ARROW-7836: [Rust] "allocate_aligned"/"reallocate" need to initialize memory to avoid UB #6422
Conversation
|
The CI failures are unrelated. @Marwes @jturner314 are you happy with this solution? |
jturner314
left a comment
jturner314
left a comment
There was a problem hiding this comment.
With the addition of a null pointer check (see the comment), this looks good to me. I think zeroing out all new memory, as in this PR, is the safest option. My understanding is that zeroing memory is very fast. At some later point, it may be worth revisiting this to see how much of a performance impact it has. (For example, it may be sufficient to just zero out the padding used for SIMD, instead of the entire allocation.) But, without a more thorough review of accesses into memory buffers, I'd prioritize safety over potential performance gains.
rust/arrow/src/memory.rs
Outdated
| ); | ||
| if new_size > old_size { | ||
| std::ptr::write_bytes( | ||
| new_ptr.offset(old_size as isize), |
There was a problem hiding this comment.
Fwiw, using the add method is equivalent and a bit more concise:
| new_ptr.offset(old_size as isize), | |
| new_ptr.add(old_size), |
rust/arrow/src/memory.rs
Outdated
| new_size, | ||
| ) | ||
| ); | ||
| if new_size > old_size { |
There was a problem hiding this comment.
There should be an additional check here in case realloc returns a null pointer (indicating an error), since otherwise the offset will be out-of-bounds. (Also, std::ptr::write_bytes requires the pointer to be non-null.)
| if new_size > old_size { | |
| if !new_ptr.is_null() && new_size > old_size { |
rust/arrow/src/memory.rs
Outdated
| ) | ||
| ); | ||
| if new_size > old_size { | ||
| std::ptr::write_bytes( |
There was a problem hiding this comment.
Fwiw, using the .write_bytes() method on the pointer would be a bit more concise.
|
I addressed all the comments. @jturner314 thank you very much for all your time and help!
Yea, definitely my thinking too. |
|
@andygrove, @nevi-me @sunchao can I get at least one approval from another committer to get this merged if you have a chance? |
3 participants
No description provided.