[CDRIVER-2322] Add leak detection on bson_t Created: 11/Oct/17  Updated: 28/Oct/23  Resolved: 05/Jan/18

Status: Closed
Project: C Driver
Component/s: libbson
Affects Version/s: None
Fix Version/s: 1.10.0

Type: New Feature Priority: Major - P3
Reporter: Kevin Albertson Assignee: Xiangyu Yao (Inactive)
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Backwards Compatibility: Fully Compatible

 Description   

There have been a few recent cases seen where we have not been properly cleaning up after initializing bson_t (see CDRIVER-2308). There may be some additional cases where we aren't cleaning up a bson_t, but it isn't showing up in valgrind because it isn't spilling over to the heap. To prevent this from happening again, we should add a compile flag to enforce every bson_init and BSON_INITIALIZER allocates memory. This should help us discover misuse of bson_t's, and will prevent real memory leaks from happening when they do spill over. We should add a flag in cmake and our autoconf configuration to enable this.



 Comments   
Comment by Githook User [ 04/Feb/18 ]

Author:

{'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}

Message: CDRIVER-2322 document MONGOC_TEST_VALGRIND env var
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/5ca1a86a751d5ac32bc044022fe50dfc350b1f48

Comment by Githook User [ 04/Feb/18 ]

Author:

{'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}

Message: CDRIVER-2322 obsolete memory-leak test
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/5fca089152b9f00fb6d93c85ba75f66ad70ce6e2

Comment by Githook User [ 28/Jan/18 ]

Author:

{'email': 'xiangyu.yao24@gmail.com', 'name': 'Xiangyu Yao', 'username': 'xy24'}

Message: CDRIVER-2322 add doc on how to use BSON_MEMCHECK flag
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/47b4e1c9255dd0a03942eabaa1095d21f3b3e01e

Comment by Githook User [ 05/Jan/18 ]

Author:

{'name': 'Xiangyu Yao', 'username': 'xy24', 'email': 'xiangyu.yao24@gmail.com'}

Message: CDRIVER-2322 use canary to detect bson memory leaks
Branch: master
https://github.com/mongodb/mongo-c-driver/commit/7e6bb2a57a769e59342e0f76cebe35fb4e090a51

Comment by A. Jesse Jiryu Davis [ 05/Jan/18 ]

Kevin this turned out great - Xiangyu found a half-dozen potential leaks throughout the driver.

Comment by Xiangyu Yao (Inactive) [ 01/Jan/18 ]

Yes, we only need it for bson_impl_inline_t. I have finished the work and almost done fixing all the memory leaks. I'll create a code review tomorrow to let you see my changes.

Comment by A. Jesse Jiryu Davis [ 01/Jan/18 ]

Kevin, do we also need the canary field for bson_impl_alloc_t in order to catch leaks, or do we only need it for bson_impl_inline_t? I believe that bson_impl_alloc_t is always created with malloc'ed data, in _bson_impl_inline_grow, so if a bson_impl_alloc_t is initialized and not destroyed then valgrind will tell us.

Xiangyu, I like Kevin's idea here. I think we should add the "canary" field to bson_impl_inline_t. Something like:

#ifdef BSON_ALWAYS_ALLOC
#define BSON_INLINE_DATA_SIZE (120 - sizeof (char *))
#else
#define BSON_INLINE_DATA_SIZE 120
#endif
 
BSON_ALIGNED_BEGIN (128)
typedef struct {
   bson_flags_t flags;
   uint32_t len;
#ifdef BSON_ALWAYS_ALLOC
   char *canary;
#endif
   uint8_t data[BSON_INLINE_DATA_SIZE];
} bson_impl_inline_t BSON_ALIGNED_END (128);

Add an option to libbson's CMakeLists.txt to enable BSON_ALWAYS_ALLOC, and mention the new option in mongo-c-driver's CONTRIBUTING.md.

Do this work in the mongo-c-driver repository, since the libbson repository is now obsolete.

Comment by Xiangyu Yao (Inactive) [ 29/Dec/17 ]

Good point. That might work! I'll try that today.

Comment by Kevin Albertson [ 28/Dec/17 ]

An alternative to forcing the initializer to return a heap allocated bson_t might be to add a field to bson_t and allocate something. Maybe something along the lines of:

/* bson-private.h */
BSON_ALIGNED_BEGIN (128)
typedef struct {
   bson_flags_t flags;
   uint32_t len;
   char* canary; /* canary */
   uint8_t data[BSON_INLINE_DATA_SIZE];
 
} bson_impl_inline_t BSON_ALIGNED_END (128);
 
 
BSON_ALIGNED_BEGIN (128)
typedef struct {
   bson_flags_t flags;        /* flags describing the bson_t */
   uint32_t len;              /* length of bson document in bytes */
   char* canary;               /* canary */
   bson_t *parent;            /* parent bson if a child */
   /* ... */
} bson_impl_alloc_t BSON_ALIGNED_END (128);

And update BSON_INLINE_DATA_SIZE to remove space for the canary pointer. That canary pointer can be initialized with a malloc'ed byte in bson_init and freed in bson_destroy.

That's a good point about bson_t not being able to be memcpy-able. I wonder if we do that in other places.

Comment by Xiangyu Yao (Inactive) [ 28/Dec/17 ]

After some developing and thinking, I think it's pretty hard to force bson_init or BSON_INITIALIZER to allocate memory.
My method was pretty straightforward: Add a compile flag so that the logic can branch off for bson_init() and BSON_INITIALIZER.

For BSON_INITIALIER, I added something to the bson-types.h:

+#ifdef BSON_FORCE_HEAP_ALLOC
+#define BSON_INITIALIZER bson_initializer_with_buffer_on_heap ()
+#else
 #define BSON_INITIALIZER \
    {                     \
       3, 5,              \
       {                  \
          5               \
       }                  \
    }
+#endif

This bson_initializer_with_buffer_on_heap() function is supposed to return a bson_t which is actually a bson_impl_alloc_t with a small buffer allocated and attached. However, in bson_impl_alloc_t, there is a field called buf which stores the pointer to the buffer pointer. Specifically, this field stores the address of another field alloc which is the pointer to the allocated buffer. Therefore, it is technically impossible to create such an initializer: Without passing the original struct, we cannot know the address of the alloc field of the original struct. Thus any assignment to the buf field would be wrong. Simple struct assignment just doesn't work here.

For bson_init(), it's a little bit more complicated. The method I tried was the same: branching off the logic like this:

void
bson_init (bson_t *bson)
{
#ifdef BSON_FORCE_HEAP_ALLOC
   bson_impl_alloc_t *impl = (bson_impl_alloc_t *) bson;
 
   BSON_ASSERT (bson);
 
   impl->flags = BSON_FLAG_STATIC;
   impl->len = 5;
   impl->parent = NULL;
   impl->depth = 0;
   impl->buf = &impl->alloc;
   impl->buflen = &impl->alloclen;
   impl->offset = 0;
   impl->alloclen = BSON_INLINE_DATA_SIZE + 1;
   impl->alloc = bson_malloc (impl->alloclen);
   impl->alloc[0] = 5;
   impl->alloc[1] = 0;
   impl->alloc[2] = 0;
   impl->alloc[3] = 0;
   impl->alloc[4] = 0;
   impl->realloc = bson_realloc_ctx;
   impl->realloc_func_ctx = NULL;
#else
   bson_impl_inline_t *impl = (bson_impl_inline_t *) bson;
 
   BSON_ASSERT (bson);
 
   impl->flags = BSON_FLAG_INLINE | BSON_FLAG_STATIC;
   impl->len = 5;
   impl->data[0] = 5;
   impl->data[1] = 0;
   impl->data[2] = 0;
   impl->data[3] = 0;
   impl->data[4] = 0;
#endif
}

Since we are passing a pointer to the bson_t struct here, the thing that doesn't work out for BSON_INITIALIZER would work here. But when I tried to run the tests, some tests broke.
I realized the existence of buf field eliminates the possibility to make a direct copy of bson_impl_alloc_t without doing any further modification because the value of the buf wouldn't make sense in the new bson_impl_alloc_t.

However, we do copy bson_t indirectly sometimes, unfortunately.
For example in mongoc_bulk_operation_insert_with_opts(), we append the new command to the bulk operation's command array. This append function simply memcpy stuff to the array. Well, in this case, the command contains a bson_t field called cmd_opts. Thus a foolish copy happens. Later, when someone tries to access the buffer inside the bson_t, he will use "*b->buf" which points to the "alloc" field in the original command which got destroyed long time ago.

I don't think it's an existing bug in our system, or at least not very fatal since cmd_opts is supposed to be small. It's just my change that forces cmd_opts to own a buffer. It's also hard to change the elegant, simple copy to something like copy-and-then-recalculate-buf-field. Due to the existence of buf field, it seems to me that bson_impl_alloc_t was designed not to support copying.

Comment by A. Jesse Jiryu Davis [ 05/Dec/17 ]

Good proposal. I hope we'll do this during Xiangyu and Pavi's rotation this winter.

Comment by Kevin Albertson [ 05/Dec/17 ]

BSON_INITIALIZER could be redefined in terms of a function, ala

bson_t bson_initializer_with_canary() {
   bson_t bson;
   // (allocate canary, initialize bson)
   return bson;
}
#define BSON_INITIALIZER_WITH_CANARY bson_initializer_with_canary()

Comment by A. Jesse Jiryu Davis [ 24/Oct/17 ]

Kevin, I'm putting this in the backlog. LMK if you had started work on this.

Generated at Wed Feb 07 21:14:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.