Mark typed buffer APIs safe (#996) (#1027)
#1866
Conversation
| @@ -299,7 +296,7 @@ impl MutableBuffer { | |||
| /// assert_eq!(buffer.len(), 8) // u32 has 4 bytes | |||
| /// ``` | |||
| #[inline] | |||
| pub fn extend_from_slice<T: ToByteSlice>(&mut self, items: &[T]) { | |||
| pub fn extend_from_slice<T: ArrowNativeType>(&mut self, items: &[T]) { | |||
There was a problem hiding this comment.
This method was potentially unsound, as ToByteSlice is not sealed and so could theoretically be implemented for a type that is not trivially transmutable (which the implementation of this method implicitly assumes).
Edit: this is an API change
github-actions
bot
added
arrow
| @@ -576,7 +576,7 @@ macro_rules! def_get_binary_array_fn { | |||
| fn $name(array: &$ty) -> Vec<ByteArray> { | |||
| let mut byte_array = ByteArray::new(); | |||
| let ptr = crate::util::memory::ByteBufferPtr::new( | |||
| unsafe { array.value_data().typed_data::<u8>() }.to_vec(), | |||
| array.value_data().as_slice().to_vec(), | |||
There was a problem hiding this comment.
I don't know why this was ever using typed_data...
|
Clippy fix in #1868 |
arrow/src/buffer/immutable.rs
Outdated
| /// correctly for type `T`. | ||
| pub fn typed_data<T: ArrowNativeType>(&self) -> &[T] { | ||
| // SAFETY | ||
| // ArrowNativeType are trivially transmutable, and this method checks alignment |
There was a problem hiding this comment.
| // ArrowNativeType are trivially transmutable, and this method checks alignment | |
| // ArrowNativeType is sealed (can't be implemented outside the arrow crate, | |
| // trivially transmutable, and this method checks alignment |
| let (prefix, offsets, suffix) = self.as_slice().align_to::<T>(); | ||
| /// This function panics if the underlying buffer is not aligned | ||
| /// correctly for type `T`. | ||
| pub fn typed_data<T: ArrowNativeType>(&self) -> &[T] { |
There was a problem hiding this comment.
I wonder if this is truly "safe" -- is it really true that any bit pattern is a valid ArrowNativeType? I am thinking about floating point representations in particular -- I wonder if this API could potentially create invalid f32 / f64 which seems like it would thus still be unsafe 🤔
There was a problem hiding this comment.
I think https://doc.rust-lang.org/std/primitive.f32.html#method.from_bits is relevant here, the short answer is it is perfectly safe to transmute arbitrary bytes to floats, it may not be wise, but it is not UB.
In particular the standard library provides safe functions that transmute u32 -> f32, u64 -> f64, and so I think it is fair to say all bit sequences are valid.
There was a problem hiding this comment.
My understanding is that it is safe because there are no actual undefined bit patterns in any of the native types (as opposed to bool or Option<...> for example). Certain bit patterns might get canonicalized when interpreted as floating point values, but I don't think that would be considered undefined behavior. There are more details about specific behavior in the docs for f64::from_bits (which is considered safe).
Codecov Report
@@ Coverage Diff @@
## master #1866 +/- ##
==========================================
- Coverage 83.48% 83.47% -0.01%
==========================================
Files 201 201
Lines 57000 57001 +1
==========================================
- Hits 47584 47583 -1
- Misses 9416 9418 +2
Continue to review full report at Codecov.
|
alamb
changed the title
safe (#996) (#1027)
Which issue does this PR close?
Closes #996
Closes #1027
Rationale for this change
These APIs were originally marked unsafe because
ArrowNativeTypecould be implemented outside the crate. Since #1819 this is no longer the case, as a result these methods are no longer unsafe as they can't lead to UB.What changes are included in this PR?
Removes
unsafefromMutableBuffer::typed_data_mutandBuffer::typed_data.Are there any user-facing changes?
No