diff options
author | ADAM David Alan Martin <adam.martin@10gen.com> | 2019-07-26 15:04:05 -0400 |
---|---|---|
committer | ADAM David Alan Martin <adam.martin@10gen.com> | 2019-07-26 15:04:05 -0400 |
commit | 5090e4efc24b88d28fa83d30457e1d097f2fc273 (patch) | |
tree | 5b7f54a544121446b0b012f9b0f6c5443aae859b /src/mongo/bson | |
parent | c44cf6f5f299991ec6819b5933b081476dc65a27 (diff) | |
download | mongo-5090e4efc24b88d28fa83d30457e1d097f2fc273.tar.gz |
SERVER-41989 Fix exception safety in `BSONObjBuilder::asTempObj`.
The setting of `_doneCalled` too early allows for it to remain set
after an exception is thrown. This will cause invariant violations
under some conditions.
Diffstat (limited to 'src/mongo/bson')
-rw-r--r-- | src/mongo/bson/bsonobjbuilder.cpp | 6 | ||||
-rw-r--r-- | src/mongo/bson/bsonobjbuilder.h | 55 | ||||
-rw-r--r-- | src/mongo/bson/util/builder.h | 24 |
3 files changed, 42 insertions, 43 deletions
diff --git a/src/mongo/bson/bsonobjbuilder.cpp b/src/mongo/bson/bsonobjbuilder.cpp index e2d56cb540e..369f307beb4 100644 --- a/src/mongo/bson/bsonobjbuilder.cpp +++ b/src/mongo/bson/bsonobjbuilder.cpp @@ -239,7 +239,7 @@ BSONObjBuilder::~BSONObjBuilder() { } template <typename Alloc> -void _BufBuilder<Alloc>::grow_reallocate(int minSize) { +void BasicBufBuilder<Alloc>::grow_reallocate(int minSize) { if (minSize > BufferMaxSize) { std::stringstream ss; ss << "BufBuilder attempted to grow() to " << minSize << " bytes, past the 64MB limit."; @@ -254,8 +254,8 @@ void _BufBuilder<Alloc>::grow_reallocate(int minSize) { size = a; } -template class _BufBuilder<SharedBufferAllocator>; -template class _BufBuilder<StackAllocator>; +template class BasicBufBuilder<SharedBufferAllocator>; +template class BasicBufBuilder<StackAllocator>; template class StringBuilderImpl<SharedBufferAllocator>; template class StringBuilderImpl<StackAllocator>; diff --git a/src/mongo/bson/bsonobjbuilder.h b/src/mongo/bson/bsonobjbuilder.h index edd3c04ed77..08720e7e18e 100644 --- a/src/mongo/bson/bsonobjbuilder.h +++ b/src/mongo/bson/bsonobjbuilder.h @@ -51,6 +51,7 @@ #include "mongo/platform/decimal128.h" #include "mongo/util/decimal_counter.h" #include "mongo/util/if_constexpr.h" +#include "mongo/util/scopeguard.h" namespace mongo { @@ -68,8 +69,7 @@ class BSONObjBuilder { public: /** @param initsize this is just a hint as to the final size of the object */ - BSONObjBuilder(int initsize = 512) - : _b(_buf), _buf(initsize), _offset(0), _s(this), _tracker(nullptr), _doneCalled(false) { + BSONObjBuilder(int initsize = 512) : _b(_buf), _buf(initsize), _s(this) { // Skip over space for the object length. The length is filled in by _done. _b.skip(sizeof(int)); @@ -82,12 +82,7 @@ public: * example. */ BSONObjBuilder(BufBuilder& baseBuilder) - : _b(baseBuilder), - _buf(0), - _offset(baseBuilder.len()), - _s(this), - _tracker(nullptr), - _doneCalled(false) { + : _b(baseBuilder), _buf(0), _offset(baseBuilder.len()), _s(this) { // Skip over space for the object length, which is filled in by _done. We don't need a // holder since we are a sub-builder, and some parent builder has already made the // reservation. @@ -102,12 +97,7 @@ public: struct ResumeBuildingTag {}; BSONObjBuilder(ResumeBuildingTag, BufBuilder& existingBuilder, std::size_t offset = 0) - : _b(existingBuilder), - _buf(0), - _offset(offset), - _s(this), - _tracker(nullptr), - _doneCalled(false) { + : _b(existingBuilder), _buf(0), _offset(offset), _s(this) { invariant(_b.len() - offset >= BSONObj::kMinBSONLength); _b.setlen(_b.len() - 1); // get rid of the previous EOO. // Reserve space for our EOO. @@ -117,10 +107,8 @@ public: BSONObjBuilder(const BSONSizeTracker& tracker) : _b(_buf), _buf(tracker.getSize()), - _offset(0), _s(this), - _tracker(const_cast<BSONSizeTracker*>(&tracker)), - _doneCalled(false) { + _tracker(const_cast<BSONSizeTracker*>(&tracker)) { // See the comments in the first constructor for details. _b.skip(sizeof(int)); @@ -135,8 +123,7 @@ public: * able to avoid copying and will just reuse the buffer. Therefore, you should try to std::move * into this constructor where possible. */ - BSONObjBuilder(BSONObj prefix) - : _b(_buf), _buf(0), _offset(0), _s(this), _tracker(nullptr), _doneCalled(false) { + BSONObjBuilder(BSONObj prefix) : _b(_buf), _buf(0), _s(this) { // If prefix wasn't owned or we don't have exclusive access to it, we must copy. if (!prefix.isOwned() || prefix.sharedBuffer().isShared()) { _b.grow(prefix.objsize()); // Make sure we won't need to realloc(). @@ -656,11 +643,20 @@ public: Intended use case: append a field if not already there. */ BSONObj asTempObj() { - BSONObj temp(_done(), BSONObj::LargeSizeTrait{}); - _b.setlen(_b.len() - 1); // next append should overwrite the EOO - _b.reserveBytes(1); // Rereserve room for the real EOO - _doneCalled = false; - return temp; + const char* const buffer = _done(); + + // None of the code which resets this builder to the not-done state is expected to throw. + // If it does, that would be a violation of our expectations. + auto resetObjectState = makeGuard([this]() noexcept { + // Immediately after the buffer for the ephemeral space created by the call to `_done()` + // is ready, reset our state to not-done. + _doneCalled = false; + + _b.setlen(_b.len() - 1); // next append should overwrite the EOO + _b.reserveBytes(1); // Rereserve room for the real EOO + }); + + return BSONObj(buffer, BSONObj::LargeSizeTrait()); } /** Make it look as if "done" has been called, so that our destructor is a no-op. Do @@ -734,8 +730,6 @@ private: if (_doneCalled) return _b.buf() + _offset; - _doneCalled = true; - // TODO remove this or find some way to prevent it from failing. Since this is intended // for use with BSON() literal queries, it is less likely to result in oversized BSON. _s.endField(); @@ -748,15 +742,18 @@ private: DataView(data).write(tagLittleEndian(size)); if (_tracker) _tracker->got(size); + + // Only set `_doneCalled` to true when all functions which could throw haven't thrown. + _doneCalled = true; return data; } BufBuilder& _b; BufBuilder _buf; - int _offset; + int _offset = 0; BSONObjBuilderValueStream _s; - BSONSizeTracker* _tracker; - bool _doneCalled; + BSONSizeTracker* _tracker = nullptr; + bool _doneCalled = false; }; class BSONArrayBuilder { diff --git a/src/mongo/bson/util/builder.h b/src/mongo/bson/util/builder.h index d1cd0d53963..ce6c9b9779b 100644 --- a/src/mongo/bson/util/builder.h +++ b/src/mongo/bson/util/builder.h @@ -36,6 +36,7 @@ #include <cstring> #include <sstream> #include <string> +#include <type_traits> #include <boost/optional.hpp> @@ -155,9 +156,9 @@ private: }; template <class BufferAllocator> -class _BufBuilder { +class BasicBufBuilder { public: - _BufBuilder(int initsize = 512) : size(initsize) { + BasicBufBuilder(int initsize = 512) : size(initsize) { if (size > 0) { _buf.malloc(size); } @@ -350,8 +351,8 @@ private: friend class StringBuilderImpl<BufferAllocator>; }; -typedef _BufBuilder<SharedBufferAllocator> BufBuilder; -MONGO_STATIC_ASSERT(std::is_move_constructible<BufBuilder>::value); +using BufBuilder = BasicBufBuilder<SharedBufferAllocator>; +MONGO_STATIC_ASSERT(std::is_move_constructible_v<BufBuilder>); /** The StackBufBuilder builds smaller datasets on the stack instead of using malloc. this can be significantly faster for small bufs. However, you can not release() the @@ -360,9 +361,9 @@ MONGO_STATIC_ASSERT(std::is_move_constructible<BufBuilder>::value); nothing bad would happen. In fact in some circumstances this might make sense, say, embedded in some other object. */ -class StackBufBuilder : public _BufBuilder<StackAllocator> { +class StackBufBuilder : public BasicBufBuilder<StackAllocator> { public: - StackBufBuilder() : _BufBuilder<StackAllocator>(StackAllocator::SZ) {} + StackBufBuilder() : BasicBufBuilder<StackAllocator>(StackAllocator::SZ) {} void release() = delete; // not allowed. not implemented. }; MONGO_STATIC_ASSERT(!std::is_move_constructible<StackBufBuilder>::value); @@ -495,7 +496,6 @@ public: } private: - _BufBuilder<Allocator> _buf; template <typename T> StringBuilderImpl& appendIntegral(T val, int maxSize) { MONGO_STATIC_ASSERT(!std::is_same<T, char>()); // char shouldn't append as number. @@ -520,13 +520,15 @@ private: _buf.l = prev + z; return *this; } + + BasicBufBuilder<Allocator> _buf; }; -typedef StringBuilderImpl<SharedBufferAllocator> StringBuilder; -typedef StringBuilderImpl<StackAllocator> StackStringBuilder; +using StringBuilder = StringBuilderImpl<SharedBufferAllocator>; +using StackStringBuilder = StringBuilderImpl<StackAllocator>; -extern template class _BufBuilder<SharedBufferAllocator>; -extern template class _BufBuilder<StackAllocator>; +extern template class BasicBufBuilder<SharedBufferAllocator>; +extern template class BasicBufBuilder<StackAllocator>; extern template class StringBuilderImpl<SharedBufferAllocator>; extern template class StringBuilderImpl<StackAllocator>; |