-
Notifications
You must be signed in to change notification settings - Fork 456
CDRIVER-6055 Audit array allocations #2098
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
CDRIVER-6055 Audit array allocations #2098
Conversation
6cdb790
to
b4c8bb8
Compare
0342bfe
to
fcd73e6
Compare
bson_array_alloc(size_t type_size, size_t num_elems); | ||
BSON_EXPORT(void *) | ||
bson_array_alloc0(size_t type_size, size_t num_elems); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is very similar to calloc
, I recommend consolidating these into a single new function bson_calloc
that uses the calloc
function on the BSON vmemtable, rather than adding two new public APIs. This will also defer the calloc
logic to the underlying allocator, which may have more smarts based on object size.
@@ -27,7 +27,7 @@ mongoc_set_new(size_t nitems, mongoc_set_item_dtor dtor, void *dtor_ctx) | |||
mongoc_set_t *set = (mongoc_set_t *)bson_malloc(sizeof(*set)); | |||
|
|||
set->items_allocated = BSON_MAX(nitems, 1); | |||
set->items = (mongoc_set_item_t *)bson_malloc(sizeof(*set->items) * set->items_allocated); | |||
set->items = (mongoc_set_item_t *)bson_array_alloc(sizeof(*set->items), set->items_allocated); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the pattern everywhere is almost always (T*)bson_array_alloc(sizeof(T), N)
, this is a good time to use a function-like macro:
// Plain function
void* _bson_alloc_n_impl(size_t item_size, size_t count);
// Macro that does the right thing every time:
#define bson_alloc_n(Type, Count) \
((Type*)_bson_alloc_n_impl(sizeof(Type), (Count))
This also ensures the returned pointer type is correctly used, rather than allowing the implicit-cast from void*
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since most of the original function calls I changed were to bson_malloc, not bson_malloc0, would consolidating into a calloc wrapper where memory is always zeroed out be cause for any efficiency concerns?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather keep a distinct non-zero alloc for cases that are performance sensitive. For example allocations in mongoc-set.c might be arbitrarily large. Maybe add a bson_alloc_n0
?
Motivation
"There are several points in the codebase that use
bson_malloc(sizeof(T) * N)
to allocate arrays of objects. This should not do an in-situ multiplication, since a large value of N will cause integer overflow and result in either an allocation failure or a bogus allocation size. Also, N = 0 can cause issues since malloc(0) is undefined/unspecified."Summary
Array allocations of the form
bson_malloc(sizeof(T) * N)
now use a new functionbson_array_alloc()
which handles multiplication of the two terms internally. Likewise, equivalent calls to 'bson_malloc0(sizeof(T) * N)now use
bson_array_alloc0()`.