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.

1 files changed, 26 insertions, 29 deletions

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 {