valkey
valkey copied to clipboard
Fix unit test issues on address sanitizer and fortify
This commit does four things:
- On various images, the linker was not able to correctly load the flto optimizations from the archive generated for unit tests, and was throwing errors. I was able to solve this by updating the plugin for the fortify test, but was unable to reproduce it on the ASAN tests or find a solution. So I decided to go with a single solution for now, which was to just disable the linker optimizations for those tests. This shouldn't weaken the protections provided by ASAN.
- The change to remove flto for some reason caused some odd inlining behavior in the intset test, that I wasn't really able to understand. The error was basically that we were doing a 4 byte write, starting at byte offset 8, for the first addition to listpack that was of size 10. Practically this has no effect, since I'm not aware of any allocator that would give us a 10 byte block as opposed to 12 (or more likely 16) bytes. The isn't the correct behavior, since an uninitialized listpack defaults to 16bit encoding, which should only be writing 2 bytes. I rabbit holed like 2 hours into this, and gave up and just ignored the warning on the file.
- Now that address sanitizer was correctly running, it picked up two issues. A memory leak and uninitialized value, so those were easy to fix.
- There is also a small change to the fortify to build the test up front instead of later, this is just to be consistent with other tests and has no functional change.
Ran a more complete set of tests here: https://github.com/madolson/valkey/actions/runs/8952734788.
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 68.43%. Comparing base (
39d4b43) to head (54b50de). Report is 1 commits behind head on unstable.
Additional details and impacted files
@@ Coverage Diff @@
## unstable #437 +/- ##
============================================
- Coverage 68.46% 68.43% -0.03%
============================================
Files 109 109
Lines 61673 61673
============================================
- Hits 42222 42206 -16
- Misses 19451 19467 +16
The error was basically that we were doing a 4 byte write, starting at byte offset 8, for the first addition to listpack that was of size 10. Practically this has no effect, since I'm not aware of any allocator that would give us a 10 byte block as opposed to 12 (or more likely 16) bytes. The isn't the correct behavior, since an uninitialized listpack defaults to 16bit encoding, which should only be writing 2 bytes.
The changes LGTM but this does sound like a bug. which function is this? I am interested in a look as well.
@PingXie https://github.com/valkey-io/valkey/blob/1b3199e0705817e2cdd8bca6ce3ab3b3567b4573/src/unit/test_intset.c#L83 was one such example. It's worth mentioning that this is only happens on the inlined versions, and not the actual usages of it. I'm not entirely clear why the compiler didn't complain before, since the same types of optimizations should have been used before. Since the unit tests used "fixed" numbers, it was making a bunch of optimizations.