[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 |
| Comments |
| Comment by Githook User [ 04/Feb/18 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 04/Feb/18 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'email': 'jesse@mongodb.com', 'name': 'A. Jesse Jiryu Davis', 'username': 'ajdavis'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 28/Jan/18 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'email': 'xiangyu.yao24@gmail.com', 'name': 'Xiangyu Yao', 'username': 'xy24'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 05/Jan/18 ] | |||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Xiangyu Yao', 'username': 'xy24', 'email': 'xiangyu.yao24@gmail.com'}Message: | |||||||||||||||||||||||||||||||||||||||||||||||||
| 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:
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:
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. For BSON_INITIALIER, I added something to the bson-types.h:
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:
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. However, we do copy bson_t indirectly sometimes, unfortunately. 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
| |||||||||||||||||||||||||||||||||||||||||||||||||
| 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. |