-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Valgrind warnings #16
Comments
Hello I've managed to reproduce this while running in Postgres. You don't see it when Valgrinding the code as-is, because I think the memory regions backing the pointers returned by palloc are quite big, which prevents Valgrind from detecting the overruns in the reads/writes. They did some Valgrind integration for Postgres 9.4, so maybe it will show up there. Anyway, add these lines to hll_add: void* buf = malloc(csz); This makes it also pack into a buffer where Valgrind can track the accesses. Run Postgres like this: sudo -u postgres valgrind --trace-children=yes --leak-check=no --gen-suppressions=all --suppressions=valgrind.supp -v postgres -D /var/lib/pgsql/data -p 5432 valgrind.supp is in src/tools in Postgres git. Then something like: psql -f valgrind.sql hll_regress valgrind.sql is here: https://gist.github.com/alberts/7950110
|
This issue also happens when packing to the calculated size after adding a first and a second 4-byte element to an hll_empty4(11, 5, 0, 0). |
Basically, I think the problem is that the bitstream_pack function ends up touching memory it shouldn't because it works with 64-bit values and enough padding isn't always allocated. |
@alberts literally just came to that conclusion myself. |
I believe that least invasive fix here is to always round up to a multiple of 8 when in the |
Makes sense. |
@alberts I'm getting Valgrind and PG set up on a non-Darwin machine, so I should be able to get working on this in just a bit. |
Bad news: simply allocating enough bytes to round up to a multiple of 8 would force me to sacrifice the rigorous input/output byte-size checking that's currently in place. I don't feel like that's the right tradeoff. I'm going to take a shot at reworking |
Hello
I have a loop that inserts 4 byte values into a HLL. It hits a Valgrind warning
With these parameters:
Log2m = 11;
Regwidth = 5;
Expthresh = -1;
Sparseon = 1;
it happens after inserting 160 elements.
It seems as if multiset_packed_size computes a too-small size for the packed buffer that has to be used.
Valgrind warnings look as follows. Line numbers are off because I've been playing with this part of the code outside Postgres.
==29406== Invalid read of size 8
==29406== at 0x40B2D2: bitstream_pack (byteswap.h:111)
==29406== by 0x40BBD8: multiset_pack (hll.c:245)
==29406== Address 0x548e387 is 311 bytes inside a block of size 317 alloc'd
==29406== Invalid write of size 8
==29406== at 0x40B2E4: bitstream_pack (hll.c:155)
==29406== by 0x40BBD8: multiset_pack (hll.c:245)
==29406== Address 0x548e387 is 311 bytes inside a block of size 317 alloc'd
which is this line: * (uint64_t *) bwcp->bwc_curp = qw;
Hopefully this is enough info to find the bug, otherwise I can dig more. I'm reasonably sure you can reproduce this with HLL running inside Postgres, but it's harder to Valgrind there.
I can also build a test against the original HLL source if that will help you.
Cheers
Albert
The text was updated successfully, but these errors were encountered: