Skip to content

Commit

Permalink
bug fix: bloomflex std::bad_alloc for large (2^32?) allocations
Browse files Browse the repository at this point in the history
when using the fastidious option, swarm crashes when it tries to
allocate the bloomfilter (at least for allocations of 5 GB, could also
be the case for smaller allocations).

Our current tests are not able to trigger this bug.

I've introduced the bug on January 28th while replacing malloc with
new and delete. Sadly, the bug is present in main branch and in the
v3.1.1 release, so I think an urgent v3.1.2 release is necessary.

Other replacements of malloc made in January may have introduced
so-far undetected bugs. I will do my best to test large datasets
before future releases.
  • Loading branch information
frederic-mahe committed Nov 10, 2022
1 parent 4d42123 commit a46bf32
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 10 deletions.
16 changes: 7 additions & 9 deletions src/bloomflex.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include <cstring>
#include "bloomflex.h"
#include "pseudo_rng.h"
#include "util.h"


auto bloomflex_patterns_generate(struct bloomflex_s * b) -> void
Expand Down Expand Up @@ -64,18 +65,18 @@ auto bloomflex_init(uint64_t size, unsigned int k) -> struct bloomflex_s *
constexpr unsigned int multiplier {16}; // multiply by 65,536
constexpr unsigned int divider {3}; // divide by 8

auto * b = new struct bloomflex_s;
auto * b = static_cast<struct bloomflex_s *>(xmalloc(sizeof(struct bloomflex_s)));
b->size = size >> divider;

b->pattern_shift = multiplier;
b->pattern_count = 1 << b->pattern_shift;
b->pattern_mask = b->pattern_count - 1;
b->pattern_k = k;

b->patterns = new uint64_t[b->pattern_count];
b->patterns = static_cast<uint64_t *>(xmalloc(b->pattern_count * sizeof(uint64_t)));
bloomflex_patterns_generate(b);

b->bitmap = new uint64_t[size];
b->bitmap = static_cast<uint64_t *>(xmalloc(size));
memset(b->bitmap, UINT8_MAX, size);

return b;
Expand All @@ -84,10 +85,7 @@ auto bloomflex_init(uint64_t size, unsigned int k) -> struct bloomflex_s *

auto bloomflex_exit(struct bloomflex_s * b) -> void
{
delete [] b->bitmap;
b->bitmap = nullptr;
delete [] b->patterns;
b->patterns = nullptr;
delete b;
b = nullptr;
xfree(b->bitmap);
xfree(b->patterns);
xfree(b);
}
22 changes: 21 additions & 1 deletion src/util.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ static const char * progress_prompt;
static uint64_t progress_next;
static uint64_t progress_size;
static uint64_t progress_chunk;
constexpr size_t memalignment = 16;


auto progress_init(const char * prompt, const uint64_t size) -> void
Expand Down Expand Up @@ -70,13 +71,32 @@ auto progress_done() -> void
}


auto xmalloc(size_t size) -> void *
{
if (size == 0) {
size = 1;
}
void * t {nullptr};
#ifdef _WIN32
t = _aligned_malloc(size, memalignment);
#else
if (posix_memalign(& t, memalignment, size) != 0) {
t = nullptr;
}
#endif
if (t == nullptr) {
fatal(error_prefix, "Unable to allocate enough memory.");
}
return t;
}


auto xrealloc(void *ptr, size_t size) -> void *
{
if (size == 0) {
size = 1;
}
#ifdef _WIN32
constexpr size_t memalignment = 16;
void * t = _aligned_realloc(ptr, size, memalignment);
#else
void * t = realloc(ptr, size);
Expand Down
1 change: 1 addition & 0 deletions src/util.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include <cstdio>


auto xmalloc(size_t size) -> void *;
auto xrealloc(void * ptr, size_t size) -> void *;
auto xfree(void * ptr) -> void;
auto xgetline(char ** linep, size_t * linecapp, FILE * stream) -> ssize_t;
Expand Down

4 comments on commit a46bf32

@frederic-mahe
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@torognes urgent bugfix! could you please make a v3.1.2 release?

@torognes
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I can release a new version, but I think the bug is on line 78/79 in bloomflex.cc:

The statement b->bitmap = new uint64_t[size]; will allocate size * 8 bytes, since an uint64_t is 8 bytes long.
While b->bitmap = static_cast<uint64_t *>(xmalloc(size)); will allocate just size bytes.

In effect, the code with new will allocate 8 times as much memory as needed, e.g. 5*8=40GB in the example, which may cause the crash if that amount of memory is not available.

Would you like to try that, or should I release it as it is?

It could also be related to the memory alignment (16 bytes), but I doubt it.

BTW, the bloompat.h and bloompat.cc files can be eliminated from the code. They are just an older version of bloomflex.*.

@frederic-mahe
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your answer. I am in favor of making a bugfix release with the current version of the code.

Then, I can try to fix b->bitmap = new uint64_t[size]; as you suggest, do extensive tests, and push to main if everything is correct. I would also like to review all the new-delete changes I made in January.

@torognes
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll go on and release it.

Please sign in to comment.