valkey icon indicating copy to clipboard operation
valkey copied to clipboard

Fix unit test issues on address sanitizer and fortify

Open madolson opened this issue 1 year ago • 2 comments

This commit does four things:

  1. 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.
  2. 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.
  3. Now that address sanitizer was correctly running, it picked up two issues. A memory leak and uninitialized value, so those were easy to fix.
  4. 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.

madolson avatar May 04 '24 19:05 madolson

Ran a more complete set of tests here: https://github.com/madolson/valkey/actions/runs/8952734788.

madolson avatar May 04 '24 19:05 madolson

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     

see 12 files with indirect coverage changes

codecov[bot] avatar May 04 '24 19:05 codecov[bot]

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 avatar May 06 '24 03:05 PingXie

@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.

madolson avatar May 06 '24 05:05 madolson