SERVER-10554 Patch memory leak in aggregation

The issue was that empty documents are represented as a NULL pointer and
Value didn't ref count NULL pointers. This is fine for all public
interactions with Value, however MutableDocument can change the pointer
in-place when updating a nested field. This caused a leak when it
transitioned a Value from holding a non-refcounted NULL pointer to
holding a real pointer since MutableDocument/MutableValue didn't set the
refCount bit.

2 files changed, 28 insertions, 2 deletions

diff --git a/src/mongo/db/pipeline/document.h b/src/mongo/db/pipeline/document.h
index b51efa91497..5e7cf3f0579 100644
--- a/src/mongo/db/pipeline/document.h
+++ b/src/mongo/db/pipeline/document.h
@@ -162,6 +162,7 @@ namespace mongo {
         friend class FieldIterator;
         friend class ValueStorage;
         friend class MutableDocument;
+        friend class MutableValue;
 
         const DocumentStorage& storage() const {
             return (_storage ? *_storage : DocumentStorage::emptyDoc());
@@ -194,8 +195,17 @@ namespace mongo {
 
         /// Used by MutableDocument(MutableValue)
         const RefCountable*& getDocPtr() {
-            if (_val.getType() != Object)
-                *this = Value(Document());
+            if (_val.getType() != Object || _val._storage.genericRCPtr == NULL) {
+                // If the current value isn't an object we replace it with a Object-typed Value.
+                // Note that we can't just use Document() here because that is a NULL pointer and
+                // Value doesn't refcount NULL pointers. This led to a memory leak (SERVER-10554)
+                // because MutableDocument::newStorage() would set a non-NULL pointer into the Value
+                // without setting the refCounter bit. While allocating a DocumentStorage here could
+                // result in an allocation where none is needed, in practice this is only called
+                // when we are about to add a field to the sub-document so this just changes where
+                // the allocation is done.
+                _val = Value(Document(new DocumentStorage()));
+            }
 
             return _val._storage.genericRCPtr;
         }
diff --git a/src/mongo/dbtests/documenttests.cpp b/src/mongo/dbtests/documenttests.cpp
index 57bf7f8b55d..56e51176f1d 100644
--- a/src/mongo/dbtests/documenttests.cpp
+++ b/src/mongo/dbtests/documenttests.cpp
@@ -170,6 +170,22 @@ namespace DocumentTests {
                 ASSERT( md.peek().getValue( "c" ).missing() );
                 assertRoundTrips( md.peek() );
 
+                // Set a nested field using []
+                md["x"]["y"]["z"] = Value("nested");
+                ASSERT_EQUALS(md.peek()["x"]["y"]["z"], Value("nested"));
+
+                // Set a nested field using setNestedField
+                FieldPath xxyyzz = string("xx.yy.zz");
+                md.setNestedField(xxyyzz, Value("nested"));
+                ASSERT_EQUALS(md.peek().getNestedField(xxyyzz), Value("nested") );
+
+                // Set a nested fields through an existing empty document
+                md["xxx"] = Value(Document());
+                md["xxx"]["yyy"] = Value(Document());
+                FieldPath xxxyyyzzz = string("xxx.yyy.zzz");
+                md.setNestedField(xxxyyyzzz, Value("nested"));
+                ASSERT_EQUALS(md.peek().getNestedField(xxxyyyzzz), Value("nested") );
+
                 // Make sure nothing moved
                 ASSERT_EQUALS(apos, md.peek().positionOf("a"));
                 ASSERT_EQUALS(bpos, md.peek().positionOf("c"));