Fix a subtle uninitialized-memory-read in Buffer::for_each_value() #7330
Conversation
When we flattened dimensions in for_each_value_prep(), we would copy from one past the end, meaning the last element contained uninitialized garbage. (This wasn't noticed as an out-of-bounds read because we overallocated in structure in for_each_value_impl()). This garbage stride was later used to advance ptrs in for_each_value_helper()... but only on the final iteration, so even if the ptr was wrong, it didn't matter, as the ptr was never used again. Under certain MSAN configurations, though, the read would be (correctly) flagged as uninitialized. This fixes the MSAN bug, and also (slightly) improves the efficiency by returning the post-flattened number of dimensions, potentially reducing the number of iterations f for_each_value_helper() needed.
vitalybuka
left a comment
vitalybuka
left a comment
There was a problem hiding this comment.
Added few comments, but I know nothing about this code.
The patch fixes sanitizer issue we had.
| } | ||
|
|
||
| return innermost_strides_are_one; | ||
| return {d, innermost_strides_are_one}; |
There was a problem hiding this comment.
this d can be after d-- on 2226, is this expected?
There was a problem hiding this comment.
Yes, this is deliberate; if we 'flatten' multiple dimensions into one, we want to know the new, smaller number of dimensions.
| if (flat) { | ||
| t[i - 1].extent *= t[i].extent; | ||
| for (int j = i; j < d; j++) { | ||
| for (int j = i; j < d - 1; j++) { |
There was a problem hiding this comment.
t[d-1].extent will be set on line 2231, what is about .stride?
There was a problem hiding this comment.
No, we want to leave that alone -- @abadams to confirm.
There was a problem hiding this comment.
Do we still need to set the extent, given that we're not going to iterate over that dimension?
There was a problem hiding this comment.
Oooh, good point. We can probably elide that now. Let me do some hackery to verify.
There was a problem hiding this comment.
yes, that was definitely unnecessary.
|
I now have another thing I'm not sure is optimal; this section in for_each_value_prep(): I presume the |
…alide#7330) * Fix a subtle uninitialized-memory-read in Buffer::for_each_value() When we flattened dimensions in for_each_value_prep(), we would copy from one past the end, meaning the last element contained uninitialized garbage. (This wasn't noticed as an out-of-bounds read because we overallocated in structure in for_each_value_impl()). This garbage stride was later used to advance ptrs in for_each_value_helper()... but only on the final iteration, so even if the ptr was wrong, it didn't matter, as the ptr was never used again. Under certain MSAN configurations, though, the read would be (correctly) flagged as uninitialized. This fixes the MSAN bug, and also (slightly) improves the efficiency by returning the post-flattened number of dimensions, potentially reducing the number of iterations f for_each_value_helper() needed. * Oopsie * Update HalideBuffer.h * Update HalideBuffer.h
When we flattened dimensions in for_each_value_prep(), we would copy from one past the end, meaning the last element contained uninitialized garbage. (This wasn't noticed as an out-of-bounds read because we overallocated in structure in for_each_value_impl()). This garbage stride was later used to advance ptrs in for_each_value_helper()... but only on the final iteration, so even if the ptr was wrong, it didn't matter, as the ptr was never used again. Under certain MSAN configurations, though, the read would be (correctly) flagged as uninitialized.
This fixes the MSAN bug, and also (slightly) improves the efficiency by returning the post-flattened number of dimensions, potentially reducing the number of iterations f for_each_value_helper() needed.