CRoaring
CRoaring copied to clipboard
RoaringBitmap
fix: add null checks to roaring64
Might help with https://github.com/RoaringBitmap/CRoaring/issues/630
This is likely not a perfect fix.
I agree with @SLieve : the code looks fine but I still wonder whether we've hit upon the correct approach.
For example we need to keep track of what to free in the malloc failure case.
What do you mean here? Possible temporaries aside from the roaring64 instance?
I don't know how you all feel about goto, but it's quite handy for handling cleanup in error cases in C in a foolproof way.
Just in case you mean the NULL itself, it's legal to pass it to free, so that in particular should not be a problem.
OTOH, maybe there's a way to do the allocations in one place and find the allocation error once, separate from the core logic, to make the logic itself easier to reason about?
Yes, I mean any temporaries. Of course goto will work, but doing this thoroughly throughout the codebase will add a lot of code for relatively little gain in my opinion. As with all new code, this added code can easily have bugs or obscure bugs that would be more obvious with less code.
I think it should at least be a global decision, and a documented one, given we don't always use the system allocator. If we opt to assume allocations can't fail that should be the case for the whole project and be documented for users providing custom allocators that if the provided one can fail it can trigger UB. Otherwise, the checks are necessary, and I'd rather leak if I forget to free something than trigger UB because I dereferenced NULL. I think both are bad but one is worse than the other.
@Oppen Sounds sensible. I have no strong opinion and I am eager to know what people think. I will setup an issue.
Unless we can get fuzzing set up to deterministically randomly return null from the allocator while doing operations, I won't have a ton of confidence in trying to handle nulls
In the case of the memory is full, alloc will return NULL, the application will core dump without this null checking. And there is no other way to avoid this from happening. Similar in roaring.c, there is a null check after each roaring_malloc. I am strongly suggest to merge this PR.
@qijiax The reason I am not merging this PR (which I wrote) is that we have no reason to believe that it solves the problem. That is we don't know what this PR will do when we run out of memory. We have no testing, no analysis. So we are basically working on assumptions.
@lemire I tried to set the global roaring_alloc and throw exception when running out of mem, but can not solve all the problem. Because I have no way to free the memory that is successfully allocated. For this PR, I believe that is same as 32bit roaring bitmap, when allocator return null, we can execute free function and return null. All the calling stack do the same thing, and finally return null to the client. I think the client also has way to handle this null retuning.
For this PR, I believe that is same as 32bit roaring bitmap, when allocator return null, we can execute free function and return null. All the calling stack do the same thing, and finally return null to the client.
Pull request invited.
@lemire Any updates on this discussion? Will you merge this PR soon? Thanks~
@qijiax
Any updates on this discussion?
Please read the comments on this PR (see above comments). It does not seem to get the massive support I'd want. So I am waiting for either a better solution or more agreement.
In https://github.com/ntop/nDPI project we use croaring code and we do test/fuzz everything with a custom allocator which may fail. Obviously we needed to disable it for croaring allocations. We'd like to have this PR (or something similar) merged; if there is some interest we could test this PR in our environment and report back any issues or possible improvements
@IvanNardi
One issue here is our capacity to test this internally. E.g., it is a fall fine to have checks, but how do we know that we are doing the right thing when errors happen ? A crash may be flat out better in some instances.