From d236dea13d2fdb9ad596679868eb4204c1562151 Mon Sep 17 00:00:00 2001 From: Wouter van Oortmerssen Date: Wed, 28 Oct 2015 11:54:55 -0700 Subject: [PATCH] Improved C++ asserts for nesting and not finishing buffers. Change-Id: I82a392bd262b13e978df748bc54b7ac43aec1e15 Tested: on Linux. --- include/flatbuffers/flatbuffers.h | 57 +++++++++++++++++++++++++++++++++------ src/idl_parser.cpp | 5 ++-- tests/test.cpp | 2 +- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/include/flatbuffers/flatbuffers.h b/include/flatbuffers/flatbuffers.h index ed7a2aa..e7100e3 100644 --- a/include/flatbuffers/flatbuffers.h +++ b/include/flatbuffers/flatbuffers.h @@ -524,7 +524,7 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { explicit FlatBufferBuilder(uoffset_t initial_size = 1024, const simple_allocator *allocator = nullptr) : buf_(initial_size, allocator ? *allocator : default_allocator), - minalign_(1), force_defaults_(false) { + nested(false), finished(false), minalign_(1), force_defaults_(false) { offsetbuf_.reserve(16); // Avoid first few reallocs. vtables_.reserve(16); EndianCheck(); @@ -535,6 +535,8 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { void Clear() { buf_.clear(); offsetbuf_.clear(); + nested = false; + finished = false; vtables_.clear(); minalign_ = 1; } @@ -543,7 +545,13 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { uoffset_t GetSize() const { return buf_.size(); } // Get the serialized buffer (after you call Finish()). - uint8_t *GetBufferPointer() const { return buf_.data(); } + uint8_t *GetBufferPointer() const { + Finished(); + return buf_.data(); + } + + // Get a pointer to an unfinished buffer. + uint8_t *GetCurrentBufferPointer() const { return buf_.data(); } // Get the released pointer to the serialized buffer. // Don't attempt to use this FlatBufferBuilder afterwards! @@ -551,7 +559,19 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { // deallocate this pointer (since it points to the middle of an allocation). // Thus, do not mix this pointer with other unique_ptr's, or call release() / // reset() on it. - unique_ptr_t ReleaseBufferPointer() { return buf_.release(); } + unique_ptr_t ReleaseBufferPointer() { + Finished(); + return buf_.release(); + } + + void Finished() const { + // If you get this assert, you're attempting to get access a buffer + // which hasn't been finished yet. Be sure to call + // FlatBufferBuilder::Finish with your root table. + // If you really need to access an unfinished buffer, call + // GetCurrentBufferPointer instead. + assert(finished); + } void ForceDefaults(bool fd) { force_defaults_ = fd; } @@ -633,15 +653,22 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { } void NotNested() { - // If you hit this, you're trying to construct an object when another - // hasn't finished yet. - assert(!offsetbuf_.size()); + // If you hit this, you're trying to construct a Table/Vector/String + // during the construction of its parent table (between the MyTableBuilder + // and table.Finish(). + // Move the creation of these sub-objects to above the MyTableBuilder to + // not get this assert. + // Ignoring this assert may appear to work in simple cases, but the reason + // it is here is that storing objects in-line may cause vtable offsets + // to not fit anymore. It also leads to vtable duplication. + assert(!nested); } // From generated code (or from the parser), we call StartTable/EndTable // with a sequence of AddElement calls in between. uoffset_t StartTable() { NotNested(); + nested = true; return GetSize(); } @@ -649,6 +676,8 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { // table, comparing it against existing vtables, and writing the // resulting vtable offset. uoffset_t EndTable(uoffset_t start, voffset_t numfields) { + // If you get this assert, a corresponding StartTable wasn't called. + assert(nested); // Write the vtable offset, which is the start of any Table. // We fill it's value later. auto vtableoffsetloc = PushElement(0); @@ -695,6 +724,8 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { WriteScalar(buf_.data_at(vtableoffsetloc), static_cast(vt_use) - static_cast(vtableoffsetloc)); + + nested = false; return vtableoffsetloc; } @@ -751,10 +782,14 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { } uoffset_t EndVector(size_t len) { + assert(nested); // Hit if no corresponding StartVector. + nested = false; return PushElement(static_cast(len)); } void StartVector(size_t len, size_t elemsize) { + NotNested(); + nested = true; PreAlign(len * elemsize); PreAlign(len * elemsize, elemsize); // Just in case elemsize > uoffset_t. } @@ -764,7 +799,6 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { } template Offset> CreateVector(const T *v, size_t len) { - NotNested(); StartVector(len, sizeof(T)); for (auto i = len; i > 0; ) { PushElement(v[--i]); @@ -778,7 +812,6 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { template Offset> CreateVectorOfStructs( const T *v, size_t len) { - NotNested(); StartVector(len * sizeof(T) / AlignOf(), AlignOf()); PushBytes(reinterpret_cast(v), sizeof(T) * len); return Offset>(EndVector(len)); @@ -829,6 +862,7 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { // FlatBuffers file header. template void Finish(Offset root, const char *file_identifier = nullptr) { + NotNested(); // This will cause the whole buffer to be aligned. PreAlign(sizeof(uoffset_t) + (file_identifier ? kFileIdentifierLength : 0), minalign_); @@ -838,6 +872,7 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { kFileIdentifierLength); } PushElement(ReferTo(root.o)); // Location of root. + finished = true; } private: @@ -857,6 +892,12 @@ class FlatBufferBuilder FLATBUFFERS_FINAL_CLASS { // Accumulating offsets of table members while it is being built. std::vector offsetbuf_; + // Ensure objects are not nested. + bool nested; + + // Ensure the buffer is finished before it is being accessed. + bool finished; + std::vector vtables_; // todo: Could make this into a map? size_t minalign_; diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp index 4a6e198..401b689 100644 --- a/src/idl_parser.cpp +++ b/src/idl_parser.cpp @@ -674,8 +674,9 @@ uoffset_t Parser::ParseTable(const StructDef &struct_def) { // be stored in-line later in the parent object. auto off = struct_stack_.size(); struct_stack_.insert(struct_stack_.end(), - builder_.GetBufferPointer(), - builder_.GetBufferPointer() + struct_def.bytesize); + builder_.GetCurrentBufferPointer(), + builder_.GetCurrentBufferPointer() + + struct_def.bytesize); builder_.PopBytes(struct_def.bytesize); return static_cast(off); } else { diff --git a/tests/test.cpp b/tests/test.cpp index 8612d6e..f068b48 100644 --- a/tests/test.cpp +++ b/tests/test.cpp @@ -508,7 +508,7 @@ void FuzzTest1() { lcg_reset(); // Reset. - uint8_t *eob = builder.GetBufferPointer() + builder.GetSize(); + uint8_t *eob = builder.GetCurrentBufferPointer() + builder.GetSize(); // Test that all objects we generated are readable and return the // expected values. We generate random objects in the same order -- 2.7.4