fix: add null checks to roaring64

[lemire]

Might help with #630

This is likely not a perfect fix.

[lemire]

@madscientist Can you help review?

[SLieve]

Code looks good, though I wonder about the cost/benefit of adding additional code to handle a subset of OOM cases. As mentioned, this will not catch everything but on the other hand it does make the code harder to reason about. For example we need to keep track of what to free in the malloc failure case.

[madscientist]

I agree with @SLieve : the code looks fine but I still wonder whether we've hit upon the correct approach.

[lemire]

I will leave this open. I am not at all pushing to get this merged.

[Oppen]

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?

[SLieve]

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.

[Oppen]

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.

[lemire]

@Oppen Sounds sensible. I have no strong opinion and I am eager to know what people think. I will setup an issue.

[Dr-Emann]

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

[qijiax]

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.

[lemire]

@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.

[qijiax]

@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.

[lemire]

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.

[qijiax]

@lemire Any updates on this discussion? Will you merge this PR soon? Thanks~

[lemire]

@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.