[SERVER-60910] Make BSONArrayBuilder/BSONObjBuilder behavior crash on common mis-uses Created: 21/Oct/21  Updated: 06/Dec/22

Status: Backlog
Project: Core Server
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Ian Boros Assignee: Backlog - Storage Execution Team
Resolution: Unresolved Votes: 0
Labels: BSON, techdebt
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
is related to SERVER-60909 Incorrect use of BSONObjBuilder in mo... Closed
Assigned Teams:
Storage Execution
Participants:

 Description   

It is possible to "abuse" the BSONArrayBuilder/BSONObjBuilder interface by creating BSONArrayBuilder 'a', appending to it, calling doneFast(), creating builder 'b', and then using 'a' again. This misuse results in an object being created that looks very different from what the caller intended:

 

TEST(BSONObjBuilderTest, BadUseTest) {                                                                                                                                                           
    BSONObjBuilder root;                                                                                                                                                                      
                                                                                                                                                                                              
    BSONArrayBuilder array1(root.subobjStart("arr1"));                                                                                                                                        
                                                                                                                                                                                              
    array1.append(3);                                                                                                                                                                                                                                                                                                                                                 
    array1.doneFast();                                                                                                                                                                        
                                                                                                                                                                                              
    BSONArrayBuilder array2(root.subobjStart("arr2"));                                                                                                                                        
                                                                                                                                                                                              
    // Notice how we're using 'array1' here. It'd be nice if this tripped an invariant or something.                                                                                            
    array1.append(4);                                                                                                                                                                         
                                                                                                                                                                                              
    array2.doneFast();                                                                                                                                                                        
                                                                                                                                                                                              
    // This "just works" even though it doesn't produce the object you'd expect.                                                                                                                       
    auto obj = root.done();                                                                                                                                                                                                                                                                                             
                                                                                                                                                                                              
    // ... later someone tries to use the object.                                                                                                                                             
                                                                                                                                                                                              
    std::cout << obj << std::endl;                                                                                                                                                            
    // Kind of a strange object, and clearly not what was intended:                                                                                                                                    
    // { arr1: { 0: 3 }, arr2: { 1: 4 } }                                                                                                                                                     
}         

It'd be nice if we could make some "best effort" to invariant/fail when BSONObjBuilder is misused in this way.

 

An example of such mis-use in our actual codebase is SERVER-60909.



 Comments   
Comment by Ian Boros [ 26/Oct/21 ]

That's true, and would be much more convenient. Perhaps I was too "prescriptive" in the ticket description+title by assuming a particular fix rather than describing the problem. Did you have something specific in mind, or another C++ library we could take inspiration from?

Comment by Andrew Morrow (Inactive) [ 26/Oct/21 ]

While triggering an invariant is one way to do this, another is to produce an API which makes writing code with mismanaged lifetimes like this impossible at compile time.

Comment by Connie Chen [ 26/Oct/21 ]

We should re-triage this, if it takes longer than a sprint to complete.

Generated at Thu Feb 08 05:51:03 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.