Introducing memory policy - #838
Conversation
Dr-Emann
left a comment
Dr-Emann
left a comment
There was a problem hiding this comment.
Should we also mention in the readme about functions that can't indicate allocation failure, something like that for functions like roaring_bitmap_add_range_closed which returns void, or roaring_bitmap_add_checked which returns a bool for if the item already existed, not if the value failed to add because of allocation errors.
So presumably, the only way to be correct in the face of allocation failures is to use a custom allocator, and set a flag on allocation failure so you know if the bitmap may be in an inconsistent state.
|
We don't have to go with this PR. An easier approach is that whenever an allocation returns NULL, we call a handler, which by default just aborts.
It is most mutating functions. Once you run out of memory, the result can be incorrect. The problem is that, in the general case, if you run out of memory, you will have modified part of the data, and you won't have the copies to go back. Returning a memory allocation error code with everything would create a burdensome API given that, once the memory runs out, the only choice you typically have is to abort. Further, even if we did introduce this API, it is doubtful people would use correctly... so it would be time wasted.
An application can set a handler that will be called when the memory cannot be allocated. This can be done so when a NULL returned but also when accessing uncommitted memory. This will interrupt the thread/process where the error occurred and it will typically be halted. Under a Unix-like system, So my idea for this PR is just to choose our model. |
Given that there's no way for us to do anything about memory access failing, we must ignore the possibility: if users don't want to allow this they can disable overcommit on their systems.
I guess the idea here is that the default behavior is to abort on OOM, but if the user decides they want to do "something else" the library will make an attempt to accommodate them by handling NULL returns as best it can? Rather than having an extra handler we could just set the default roaring_init_memory_hook() allocator functions to versions that call abort() if allocation fails. If users wanted to do something else they could replace them. We don't really need an extra handler.
I understand the impetus for this, but in practice I've found it often annoying when these are added to things like the libc API. What do you think of adding a
If there are functions that can fail to perform their documented operation but not give the user an indication that they failed, that doesn't seem good. If we decide to address this using the above model (e.g. the default aborts if allocation fails, but users can do something different and the library provides "best effort" support) we should probably make this clear in the memory management discussion (e.g., if you replace the default behavior it's not enough to just return NULL: you need to provide some kind of out-of-band error notification a la errno that can be checked by methods that can't report failures), and make an effort to identify the methods that don't have any way to report failures and add a note about it to their documentation. Other: Did you notice any change in the performance, from adding various extra checks and reorgs? |
|
@madscientist Yes, I am not sure if we should do anything at all. So this PR is not something I will just 'merge'. |
…Roaring into lemire/mempolicy
Extended fuzzing of the seeded failing-allocator harness (ASan+UBSan+LSan, 1500+ seeds) surfaced three out-of-memory bugs plus one pre-existing alignment issue: - array_run_container_xor: array_container_from_run() and bitset_container_from_run() can return NULL under OOM. The temporary was used unchecked and dereferenced in array_array_container_xor (NULL deref; same family as issue #841, reachable via roaring_bitmap_xor_inplace). Return early with *dst = NULL, matching the file's OOM convention. - roaring_bitmap_andnot: the empty-first-operand branch called roaring_bitmap_create() then roaring_bitmap_set_copy_on_write() without a NULL check, dereferencing NULL when the allocation fails. NULL-check and return NULL. - roaring_bitmap_remove_range_closed: when ra_unshare_container_at_index fails under OOM the container stays shared; the old code advanced src without dst, so the trailing ra_shift_tail dropped the shared wrapper without freeing it (leak). Keep the shared container as a survivor at dst, mirroring the add-range keep_existing_container handling. - roaring_bitmap_deserialize (SERIALIZATION_ARRAY_UINT32): the element pointer was cast to uint32_t* from an unaligned buffer offset, so elems + i formed a misaligned uint32_t* (UBSan -fsanitize=alignment). Index a char pointer with a byte offset and read via memcpy. Adds regression tests (test_xor_array_run_oom, test_andnot_empty_oom, test_cow_remove_range_leak_oom); each was confirmed to fail on the unfixed code (UBSan null deref / LSan leak) and pass with the fix. Full memory_pressure_unit suite is 15/15 under ASan+UBSan+LSan; toplevel_unit 169/169.
|
@madscientist I removed the CROARING_NODISCARD from our public API. |
So, the way this works in Go is that a failure to allocate will just panic. In C++, you typically get an exception. And so forth. Memory conditions are typically treated as fatal errors. |
3 participants
Currently, CRoaring does not have a policy as to what happens when we run out of memory. The gist of the proposal is as follows.
It is not possible to perfectly test this policy, but I have used a fuzzer to do so.
The rationale for the policy is that we can have one model throughout the code base, so we know what is acceptable at any point with respect to what happens with malloc returns NULL.
The goal is not and cannot be that you can continue to use Roaring bitmaps in an allocating manner after you have run out of memory. It is possible to build such a system, but it involves making copies prior to allocating operations and monitoring constantly the memory allocations, not something we want or can push on everyone.
The goal is not even to avoid crashes when you run out of memory... because that is the likely outcome. As I wrote above, if you are under Linux, the most likely outcome is that malloc will succeed and you will crash on memory access.
So the point is more about aesthetics. We set a convention, and we hold it, and every future report is assessed per this policy.
For the practical minded engineer, the policy is still the same: don't continue using a system after you have run out of memory.
Great many changes involve the addition of the
CROARING_NODISCARDqualifier. We now systematically demand that users of some functions check the return. Further, great many changes use the(void)resultis our tests to ignore these demands.(In the README, I remove spurious spaces at the end of some lines, these lines were not otherwise changed.)
Fixes #638