Revert "fix potential UB in ByteBool constructor"

This reverts commit 655082c.

Author: a10y

encodings/bytebool/public-api.lock

@@ -10,10 +10,6 @@ pub fn vortex_bytebool::ByteBool::from_vec<V: core::convert::Into<vortex_array::
 
 pub fn vortex_bytebool::ByteBool::new(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> vortex_bytebool::ByteBoolArray
 
-pub unsafe fn vortex_bytebool::ByteBool::new_unchecked(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> vortex_bytebool::ByteBoolArray
-
-pub fn vortex_bytebool::ByteBool::try_new(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> vortex_error::VortexResult<vortex_bytebool::ByteBoolArray>
-
 impl core::clone::Clone for vortex_bytebool::ByteBool
 
 pub fn vortex_bytebool::ByteBool::clone(&self) -> vortex_bytebool::ByteBool
@@ -80,8 +76,6 @@ pub struct vortex_bytebool::ByteBoolData
 
 impl vortex_bytebool::ByteBoolData
 
-pub fn vortex_bytebool::ByteBoolData::as_slice(&self) -> &[bool]
-
 pub fn vortex_bytebool::ByteBoolData::buffer(&self) -> &vortex_array::buffer::BufferHandle
 
 pub fn vortex_bytebool::ByteBoolData::from_vec<V: core::convert::Into<vortex_array::validity::Validity>>(data: alloc::vec::Vec<bool>, validity: V) -> Self
@@ -92,14 +86,10 @@ pub fn vortex_bytebool::ByteBoolData::len(&self) -> usize
 
 pub fn vortex_bytebool::ByteBoolData::new(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> Self
 
-pub unsafe fn vortex_bytebool::ByteBoolData::new_unchecked(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> Self
-
-pub fn vortex_bytebool::ByteBoolData::try_new(buffer: vortex_array::buffer::BufferHandle, validity: vortex_array::validity::Validity) -> vortex_error::VortexResult<Self>
+pub fn vortex_bytebool::ByteBoolData::truthy_bytes(&self) -> &[u8]
 
 pub fn vortex_bytebool::ByteBoolData::validate(buffer: &vortex_array::buffer::BufferHandle, validity: &vortex_array::validity::Validity, dtype: &vortex_array::dtype::DType, len: usize) -> vortex_error::VortexResult<()>
 
-pub fn vortex_bytebool::ByteBoolData::validate_bytes(buffer: &vortex_array::buffer::BufferHandle) -> vortex_error::VortexResult<()>
-
 impl core::clone::Clone for vortex_bytebool::ByteBoolData
 
 pub fn vortex_bytebool::ByteBoolData::clone(&self) -> vortex_bytebool::ByteBoolData

encodings/bytebool/src/array.rs

@@ -29,13 +29,11 @@ use vortex_array::vtable::VTable;
 use vortex_array::vtable::ValidityVTable;
 use vortex_array::vtable::child_to_validity;
 use vortex_array::vtable::validity_to_child;
+use vortex_buffer::BitBufferMut;
 use vortex_buffer::ByteBuffer;
-use vortex_buffer::{BitBuffer, BitBufferMut};
-use vortex_error::VortexExpect as _;
 use vortex_error::VortexResult;
 use vortex_error::vortex_bail;
 use vortex_error::vortex_ensure;
-use vortex_error::vortex_ensure_eq;
 use vortex_error::vortex_panic;
 use vortex_session::VortexSession;
 use vortex_session::registry::CachedId;
@@ -133,7 +131,7 @@ impl VTable for ByteBool {
         }
         let buffer = buffers[0].clone();
 
-        let data = ByteBoolData::try_new(buffer, validity.clone())?;
+        let data = ByteBoolData::new(buffer, validity.clone());
         let slots = ByteBoolData::make_slots(&validity, len);
         Ok(ArrayParts::new(self.clone(), dtype.clone(), len, data).with_slots(slots))
     }
@@ -200,41 +198,12 @@ impl<T: TypedArrayRef<ByteBool>> ByteBoolArrayExt for T {}
 pub struct ByteBool;
 
 impl ByteBool {
-    /// Construct a [`ByteBoolArray`] from a raw bytes buffer and validity.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the provided components do not satisfy the invariants documented in
-    /// [`ByteBool::new_unchecked`].
     pub fn new(buffer: BufferHandle, validity: Validity) -> ByteBoolArray {
-        Self::try_new(buffer, validity).vortex_expect("ByteBoolArray construction failed")
-    }
-
-    /// Construct a [`ByteBoolArray`] from a raw bytes buffer and validity, returning
-    /// an error if the provided components do not satisfy the invariants documented
-    /// in [`ByteBool::new_unchecked`].
-    pub fn try_new(buffer: BufferHandle, validity: Validity) -> VortexResult<ByteBoolArray> {
         let dtype = DType::Bool(validity.nullability());
-        let len = buffer.len();
-        let slots = ByteBoolData::make_slots(&validity, len);
-        let data = ByteBoolData::try_new(buffer, validity)?;
-        Array::try_from_parts(ArrayParts::new(ByteBool, dtype, len, data).with_slots(slots))
-    }
 
-    /// Construct a [`ByteBoolArray`] without validating the buffer contents.
-    ///
-    /// # Safety
-    ///
-    /// Every byte of `buffer` must be `0x00` or `0x01`. Any other byte value is
-    /// Undefined Behavior because [`ByteBoolData::truthy_bytes`] reinterprets the buffer
-    /// as `&[bool]`, and a `bool` with any bit pattern other than 0 or 1 is UB.
-    /// If `validity` is [`Validity::Array`], its length must equal `buffer.len()`.
-    pub unsafe fn new_unchecked(buffer: BufferHandle, validity: Validity) -> ByteBoolArray {
-        let dtype = DType::Bool(validity.nullability());
-        let len = buffer.len();
-        let slots = ByteBoolData::make_slots(&validity, len);
-        // SAFETY: caller guarantees every buffer byte is 0 or 1.
-        let data = unsafe { ByteBoolData::new_unchecked(buffer, validity) };
+        let slots = ByteBoolData::make_slots(&validity, buffer.len());
+        let data = ByteBoolData::new(buffer, validity);
+        let len = data.len();
         unsafe {
             Array::from_parts_unchecked(
                 ArrayParts::new(ByteBool, dtype, len, data).with_slots(slots),
@@ -294,81 +263,22 @@ impl ByteBoolData {
         Ok(())
     }
 
-    /// Validate that every byte of `buffer` is `0x00` or `0x01`.
-    ///
-    /// [`ByteBoolData::truthy_bytes`] transmutes the buffer's bytes to `&[bool]`; any byte
-    /// other than `0x00` or `0x01` would produce a `bool` with an invalid bit pattern,
-    /// which is Undefined Behavior per the Rust reference.
-    ///
-    /// Device-resident buffers are not host-readable and are skipped.
-    pub fn validate_bytes(buffer: &BufferHandle) -> VortexResult<()> {
-        let Some(bytes) = buffer.as_host_opt() else {
-            return Ok(());
-        };
-        // Count over a flat `&[u8]` vectorizes to pcmpgtb/pmovmskb on x86 and
-        // cmhi/addv on aarch64, so the fast path runs at ~16 bytes/cycle.
-        // See https://godbolt.org/z/z797nT1c8
-        let invalid = bytes.as_slice().iter().filter(|&&b| b > 1).count();
-        vortex_ensure_eq!(
-            invalid,
-            0,
-            "ByteBoolArray buffer contains {invalid} bytes that are not 0 or 1",
-        );
-        Ok(())
-    }
-
     fn make_slots(validity: &Validity, len: usize) -> Vec<Option<ArrayRef>> {
         vec![validity_to_child(validity, len)]
     }
 
-    /// Construct [`ByteBoolData`] from a raw bytes buffer and validity.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the provided components do not satisfy the invariants documented in
-    /// [`ByteBoolData::new_unchecked`].
     pub fn new(buffer: BufferHandle, validity: Validity) -> Self {
-        Self::try_new(buffer, validity).vortex_expect("ByteBoolData construction failed")
-    }
-
-    /// Construct [`ByteBoolData`] from a raw bytes buffer and validity, returning an
-    /// error if the provided components do not satisfy the invariants documented in
-    /// [`ByteBoolData::new_unchecked`].
-    pub fn try_new(buffer: BufferHandle, validity: Validity) -> VortexResult<Self> {
-        Self::check_validity_len(&buffer, &validity)?;
-        Self::validate_bytes(&buffer)?;
-        // SAFETY: buffer bytes and validity length validated above.
-        Ok(unsafe { Self::new_unchecked(buffer, validity) })
-    }
-
-    /// Construct [`ByteBoolData`] without validating the buffer contents.
-    ///
-    /// # Safety
-    ///
-    /// Every byte of `buffer` must be `0x00` or `0x01`. Any other byte value is
-    /// Undefined Behavior because [`ByteBoolData::truthy_bytes`] reinterprets the buffer
-    /// as `&[bool]`, and a `bool` with any bit pattern other than 0 or 1 is UB.
-    /// If `validity` is [`Validity::Array`], its length must equal `buffer.len()`.
-    pub unsafe fn new_unchecked(buffer: BufferHandle, validity: Validity) -> Self {
-        debug_assert!(
-            Self::validate_bytes(&buffer).is_ok(),
-            "ByteBoolData::new_unchecked called with non-boolean bytes",
-        );
-        Self::check_validity_len(&buffer, &validity)
-            .vortex_expect("ByteBoolData::new_unchecked called with mismatched validity length");
-        Self { buffer }
-    }
-
-    fn check_validity_len(buffer: &BufferHandle, validity: &Validity) -> VortexResult<()> {
-        if let Some(vlen) = validity.maybe_len() {
-            vortex_ensure!(
-                buffer.len() == vlen,
+        let length = buffer.len();
+        if let Some(vlen) = validity.maybe_len()
+            && length != vlen
+        {
+            vortex_panic!(
                 "Buffer length ({}) does not match validity length ({})",
-                buffer.len(),
+                length,
                 vlen
             );
         }
-        Ok(())
+        Self { buffer }
     }
 
     /// Returns the number of elements in the array.
@@ -386,9 +296,7 @@ impl ByteBoolData {
         let validity = validity.into();
         // SAFETY: we are transmuting a Vec<bool> into a Vec<u8>
         let data: Vec<u8> = unsafe { std::mem::transmute(data) };
-        let buffer = BufferHandle::new_host(ByteBuffer::from(data));
-        // SAFETY: bytes came from `Vec<bool>`, which guarantees values of 0 or 1.
-        unsafe { Self::new_unchecked(buffer, validity) }
+        Self::new(BufferHandle::new_host(ByteBuffer::from(data)), validity)
     }
 
     pub fn buffer(&self) -> &BufferHandle {
@@ -489,47 +397,6 @@ mod tests {
     }
 
     #[test]
-    fn try_new_rejects_invalid_byte() {
-        // `ByteBoolData::as_slice` transmutes the underlying bytes into `&[bool]`.
-        // A bool with any bit pattern other than 0 or 1 is Undefined Behavior per
-        // the Rust reference, so `try_new` must reject these buffers.
-        let raw = ByteBuffer::from(vec![0x02u8, 0x01, 0xFFu8]);
-        let handle = BufferHandle::new_host(raw);
-        let err = ByteBool::try_new(handle, Validity::NonNullable).unwrap_err();
-        assert!(
-            err.to_string().contains("bytes that are not 0 or 1"),
-            "unexpected error: {err}",
-        );
-    }
-
-    #[test]
-    #[should_panic(expected = "bytes that are not 0 or 1")]
-    fn new_panics_on_invalid_byte() {
-        let raw = ByteBuffer::from(vec![0x02u8]);
-        let handle = BufferHandle::new_host(raw);
-        drop(ByteBool::new(handle, Validity::NonNullable));
-    }
-
-    #[test]
-    fn new_unchecked_accepts_valid_bytes() {
-        let raw = ByteBuffer::from(vec![0x00u8, 0x01, 0x01, 0x00]);
-        let handle = BufferHandle::new_host(raw);
-        // SAFETY: all bytes are 0 or 1.
-        let arr = unsafe { ByteBool::new_unchecked(handle, Validity::NonNullable) };
-        assert_eq!(arr.len(), 4);
-        assert_eq!(arr.truthy_bytes(), &[false, true, true, false]);
-    }
-
-    #[test]
-    #[cfg(debug_assertions)]
-    #[should_panic(expected = "non-boolean bytes")]
-    fn new_unchecked_debug_asserts_invalid_bytes() {
-        let raw = ByteBuffer::from(vec![0x02u8]);
-        let handle = BufferHandle::new_host(raw);
-        // SAFETY: intentionally violated to exercise the debug assertion.
-        drop(unsafe { ByteBool::new_unchecked(handle, Validity::NonNullable) });
-    }
-
     #[test]
     fn test_nullable_bytebool_serde_roundtrip() {
         let array = ByteBool::from_option_vec(vec![Some(true), None, Some(false), None]);

encodings/bytebool/src/compute.rs

@@ -76,12 +76,9 @@ impl TakeExecute for ByteBool {
                 .collect::<ByteBuffer>()
         });
 
-        // SAFETY:
-        unsafe {
-            Ok(Some(
-                ByteBool::new_unchecked(BufferHandle::new_host(taken), validity).into_array(),
-            ))
-        }
+        Ok(Some(
+            ByteBool::new(BufferHandle::new_host(taken), validity).into_array(),
+        ))
     }
 }
 

vortex-buffer/public-api.lock

@@ -504,6 +504,10 @@ impl core::convert::From<&[bool]> for vortex_buffer::BitBufferMut
 
 pub fn vortex_buffer::BitBufferMut::from(value: &[bool]) -> Self
 
+impl core::convert::From<&[u8]> for vortex_buffer::BitBufferMut
+
+pub fn vortex_buffer::BitBufferMut::from(value: &[u8]) -> Self
+
 impl core::convert::From<alloc::vec::Vec<bool>> for vortex_buffer::BitBufferMut
 
 pub fn vortex_buffer::BitBufferMut::from(value: alloc::vec::Vec<bool>) -> Self