spotify / sparkey

Simple constant key/value storage library, for read-heavy systems with infrequent large bulk inserts.
Apache License 2.0
1.18k stars 81 forks source link

Fix a leak when validation fails in sparkey_hash_open #48

Closed seizethedave closed 1 year ago

seizethedave commented 1 year ago

Howdy! This is an extension of #47 where sparkey_hash_close fails to deallocate some memory when sparkey_hash_open's validation checks fail.

I've added another "integration test" to the testsystem executable that exercises this problem, and running valgrind on the built program shows the issue before and after:

Memcheck, a memory error detector
Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
Command: testsystem

Conditional jump or move depends on uninitialised value(s)
   at 0x10B064: sparkey_hash_close (hashreader.c:107)
   by 0x10B289: sparkey_hash_open (hashreader.c:88)
   by 0x10AD9E: verify_files_closed (testsystem.c:250)
   by 0x10B01A: main (testsystem.c:281)

Success!

HEAP SUMMARY:
    in use at exit: 296 bytes in 1 blocks
  total heap usage: 73,548 allocs, 73,547 frees, 913,473,745 bytes allocated

LEAK SUMMARY:
   definitely lost: 296 bytes in 1 blocks
   indirectly lost: 0 bytes in 0 blocks
     possibly lost: 0 bytes in 0 blocks
   still reachable: 0 bytes in 0 blocks
        suppressed: 0 bytes in 0 blocks
Rerun with --leak-check=full to see details of leaked memory

Use --track-origins=yes to see where uninitialised values come from
For lists of detected and suppressed errors, rerun with: -s
ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 0 from 0)

And here's valgrind with the fix enabled:

Memcheck, a memory error detector
Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
Command: testsystem

Success!

HEAP SUMMARY:
    in use at exit: 0 bytes in 0 blocks
  total heap usage: 73,548 allocs, 73,548 frees, 913,473,745 bytes allocated

All heap blocks were freed -- no leaks are possible

For lists of detected and suppressed errors, rerun with: -s
ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

At Stripe we build Sparkey with Bazel and I haven't yet been able to get the OOTB autotools build working. I'll keep trying just to make sure this all works under autotools, but if you have this set up I'd appreciate a sanity check just the same!

Running valgrind on the test binaries is also a manual thing today. I could see running valgrind as part of a build test suite, but I imagine y'all are not investing much in ole Sparkey these days. If you have any pointers on how that could be accomplished in autotools we might be up for contributing that!

spkrka commented 1 year ago

Thanks, looks good to me!