Fix an out-of-bounds read in validity copying in contiguous_split. #9842
Conversation
…opy if we don't need the slack bits. Prevents a potential read of unallocated memory.
nvdbaranec
added
bug
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #9842 +/- ##
===============================================
- Coverage 10.49% 9.94% -0.55%
===============================================
Files 119 119
Lines 20305 21452 +1147
===============================================
+ Hits 2130 2134 +4
- Misses 18175 19318 +1143
Continue to review full report at Codecov.
|
| uint32_t const val = (v >> bit_shift) | (next << (32 - bit_shift)); | ||
| // if we're at the very last word of a validity copy, we do not always need to read the next | ||
| // word to get the final trailing bits. | ||
| auto const read_trailing_bits = bit_shift > 0 && remainder == 4 && have_trailing_bits; |
There was a problem hiding this comment.
Is the 4 here sizeof(uint32_t)? Not requesting a change, just trying to understand the logic.
There was a problem hiding this comment.
Correct. We're reading (up to) the trailing 15 bytes of the buffer to be copied. In the case where the buffer happens to be validity, the elements are all bitmask_type words.
The fundamental issue is that if we're copying from some arbitrary row, we have to shift the bits of any validity around. So imagine we're reading 1 bit starting at row 31. That's all within the "final" word - we just need to shift that bit up by 31 for the output. But let's say we want to read 2 bits starting at row 31. The first bit comes from the word we just read, but the 2nd bit comes from the next word (idx + 1) - so we have to read it.
The initial bug here was that we were doing this when we shouldn't have been.
There was a problem hiding this comment.
Apologies in advance for the drive-by review comment. Is there any reason not to define this variable? I see multiple magic numbers throughout this code (lots of 4s and 32s) and it seems like a constexpr auto uint32_size = sizeof(uint32_t); would help avoid questions like this.
There was a problem hiding this comment.
drive-by review comment
😂
I think it would be good, if not a bit out of scope of the PR. Up to the author IMO.
There was a problem hiding this comment.
I think something like this definitely adds clarity.
constexpr size_type rows_per_element = 32;
auto const have_trailing_bits = ((num_elements * rows_per_element) - num_rows) < bit_shift;
But I also think this makes more sense as is, since this reads as pretty standard bit-shifting stuff.
uint32_t const val = (v >> bit_shift) | (next << (32 - bit_shift));
Changing it to this would obfuscate I think.
uint32_t const val = (v >> bit_shift) | (next << (rows_per_bitmask - bit_shift));
|
rerun tests |
1 similar comment
|
rerun tests |
|
@gpucibot merge |
Fixes #9504
The bug was pretty straightforward: when copying validity bits, we (potentially) perform a bit shift on the data so that we can read aligned 4 bytes at a time. Under certain circumstances, we were reading 1 word past the end of the input incorrectly.
Adding a do not merge tag - waiting to get a full run of TPC-DS with this as an extra safety check.