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.

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>;