Hacker News new | past | comments | ask | show | jobs | submit login

Sorry if this is a bit negative but from a cursory look I wouldn't use this.

An efficient Bloom Filter should only allocate on heap exactly once: when allocating the initial bitmap. This implementation seems to be not very efficient as it allocates even during a simple query.

The usage of `static` variables makes the BloomFilter code unnecessarily dangerous to use when multiple threads are used even if every thread just wants to allocate and use totally separate bloom filters. Actually it could even cause issues with two bloom filters created after each other since even `Initialize` just returns a pointer to a single static `BloomFilter` :o I can't come up with a reason why you'd consider making anything in there `static`.

There is also strange code like this loop https://github.com/hashlookup/fleur/blob/4ee2644a850381d928a... that jumped into my eye.

Oh and btw I believe bloom_path can cause memory safety issues because it is not terminated with a null after strncopy.

I would additionally suggest to use some code formatting tool as it's a bit all over the place. But the variable/function names mix various styles as well.




> There is also strange code like this loop https://github.com/hashlookup/fleur/blob/4ee2644a850381d928a... that jumped into my eye.

The line below that is worse:

strncpy(bloom_path , argv[optind], 128);

If you pass something >= 128 chars then bloom_path won't be null terminated. In general strncpy should never be used for copying strings.

For expansion on that: https://ramblings.implicit.net/c/2014/05/02/c-functions-that...


And I just realised the GP already mentioned the bloom_path issue. Sorry, too late to edit.


tl;dr: use strlcpy instead.


I noticed some other weirdness like functions taking a double pointer argument and then freeing the memory and replacing it with a newly allocated piece of memory. Seems rather dangerous to do that.

And zeroing out data by freeing the memory and callocing new memory. Why not just write zeros into the memory instead of reallocating it from scratch only to overwrite it with zeros anyway?




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: