This is an automated email from the ASF dual-hosted git repository.
Jefffrey pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 14a5d7b8af docs : add saftey comments to unsafe callsites (#10839)
14a5d7b8af is described below
commit 14a5d7b8af7084a39e8516c1feb59c026e6ecf77
Author: RIchard Baah <[email protected]>
AuthorDate: Wed Aug 26 21:54:56 2026 -0400
docs : add saftey comments to unsafe callsites (#10839)
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax.
-->
- Closes #147
- Closes #149
- Closes #151
# Rationale for this change
see #147, #149, #151
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
adds doc comment to add unsafe call sites mentioned in the linked
issues.
**AI was used to help find and add doc comments**
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
# Are these changes tested?
n/a
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
If this PR claims a performance improvement, please include evidence
such as benchmark results.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
If there are any breaking changes to public APIs, please call them out.
-->
---
arrow-array/src/ffi.rs | 10 +++++++---
arrow-buffer/src/util/bit_chunk_iterator.rs | 8 ++++++++
arrow-buffer/src/util/bit_util.rs | 5 +++++
arrow-schema/src/ffi.rs | 11 +++++++++++
4 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/arrow-array/src/ffi.rs b/arrow-array/src/ffi.rs
index d09c077c65..1957031c22 100644
--- a/arrow-array/src/ffi.rs
+++ b/arrow-array/src/ffi.rs
@@ -322,7 +322,9 @@ impl ImportedArrowArray<'_> {
child_data.push(d.consume()?);
}
- // Should FFI be checking validity?
+ // Safety: all fields (length, null_count, null buffer, data buffers,
child data) were
+ // derived from the C Data Interface schema and array, which the
caller of `from_ffi`
+ // guarantees follow the spec; the constructed `ArrayData` satisfies
its invariants.
Ok(unsafe {
ArrayData::new_unchecked(
self.data_type,
@@ -482,7 +484,9 @@ impl ImportedArrowArray<'_> {
// we assume that pointer is aligned for `i32`, as Utf8 uses
`i32` offsets.
#[expect(clippy::cast_ptr_alignment)]
let offset_buffer = self.array.buffer(1).cast::<i32>();
- // get last offset
+ // Safety: `len` is the byte length of the offset buffer;
dividing by `size_of::<i32>()`
+ // gives the number of i32 elements. The `- 1` is safe because
the array is non-empty
+ // (checked above), so the offset buffer has at least one
element.
(unsafe { *offset_buffer.add(len / size_of::<i32>() - 1) }) as
usize
}
(DataType::LargeUtf8 | DataType::LargeBinary, 2) => {
@@ -496,7 +500,7 @@ impl ImportedArrowArray<'_> {
// we assume that pointer is aligned for `i64`, as Large uses
`i64` offsets.
#[expect(clippy::cast_ptr_alignment)]
let offset_buffer = self.array.buffer(1).cast::<i64>();
- // get last offset
+ // Safety: same as the i32 case above but for i64 offsets.
(unsafe { *offset_buffer.add(len / size_of::<i64>() - 1) }) as
usize
}
// View types: these have variadic buffers.
diff --git a/arrow-buffer/src/util/bit_chunk_iterator.rs
b/arrow-buffer/src/util/bit_chunk_iterator.rs
index 460349e7a6..b67309962a 100644
--- a/arrow-buffer/src/util/bit_chunk_iterator.rs
+++ b/arrow-buffer/src/util/bit_chunk_iterator.rs
@@ -88,6 +88,7 @@ impl<'a> UnalignedBitChunk<'a> {
}
// Read into prefix and suffix as needed
+ // Safety: u64 has no invalid bit patterns so reinterpreting
initialized u8 bytes as u64 is sound.
let (prefix, mut chunks, suffix) = unsafe { buffer.align_to::<u64>() };
assert!(
prefix.len() < 8 && suffix.len() < 8,
@@ -284,6 +285,8 @@ impl<'a> BitChunks<'a> {
// might be one more than sizeof(u64) if the offset is in the
middle of a byte
let byte_len = ceil(bit_len + bit_offset, 8);
// pointer to remainder bytes after all complete chunks
+ // Safety: the buffer contains `chunk_len * 8 + ceil(remainder_len
+ bit_offset, 8)`
+ // bytes, so offsetting by `chunk_len * 8` and reading `byte_len`
bytes is in-bounds.
let base = unsafe {
self.buffer
.as_ptr()
@@ -373,6 +376,9 @@ impl Iterator for BitChunkIterator<'_> {
// bit-packed buffers are stored starting with the least-significant
byte first
// so when reading as u64 on a big-endian machine, the bytes need to
be swapped
+ // Safety: `index < self.chunk_len` and the buffer is at least
`chunk_len * 8` bytes long,
+ // so `raw_data.add(index)` is a valid in-bounds pointer;
`read_unaligned` handles
+ // any pointer alignment.
let current = unsafe {
std::ptr::read_unaligned(raw_data.add(index)).to_le() };
let bit_offset = self.bit_offset;
@@ -382,6 +388,8 @@ impl Iterator for BitChunkIterator<'_> {
} else {
// the constructor ensures that bit_offset is in 0..8
// that means we need to read at most one additional byte to fill
in the high bits
+ // Safety: the buffer has at least one byte past the last chunk
(the remainder byte
+ // needed for `bit_offset > 0`), so `index + 1` is within bounds.
let next =
unsafe { std::ptr::read_unaligned(raw_data.add(index +
1).cast::<u8>()) as u64 };
diff --git a/arrow-buffer/src/util/bit_util.rs
b/arrow-buffer/src/util/bit_util.rs
index d5da713e8a..f06e03ef13 100644
--- a/arrow-buffer/src/util/bit_util.rs
+++ b/arrow-buffer/src/util/bit_util.rs
@@ -664,6 +664,8 @@ impl<'a> U64UnalignedSlice<'a> {
// make the last pointer invalid, we handle the first element outside
the loop
// and then advance the pointer at the start of the loop
// making sure that the iterator is not empty
+ // Safety: `self.len > 0` (checked above) and the pointer has not been
advanced yet,
+ // so it is valid for reads and writes.
unsafe {
// I hope the function get inlined and the compiler remove the
dead right parameter
self.apply_bin_op(0, &mut |left, _| map(left));
@@ -794,6 +796,9 @@ fn set_remainder_bits(start_remainder_mut_slice: &mut [u8],
rem: u64, remainder_
// without calling `to_byte_slice` for each element,
// which is correct for all ArrowNativeType implementations including
u64.
let src = rem.as_ptr();
+ // Safety: `rem` has length `remainder_bytes`,
`start_remainder_mut_slice` has length
+ // `remainder_bytes`, and the two slices are non-overlapping (rem is
derived from a
+ // local `to_le_bytes()` call; start_remainder_mut_slice is the
caller's mutable buffer).
unsafe {
std::ptr::copy_nonoverlapping(
src,
diff --git a/arrow-schema/src/ffi.rs b/arrow-schema/src/ffi.rs
index 867d1409b9..679ced42d5 100644
--- a/arrow-schema/src/ffi.rs
+++ b/arrow-schema/src/ffi.rs
@@ -260,6 +260,9 @@ impl FFI_ArrowSchema {
None
};
+ // Safety: `self.private_data` was allocated as
`Box<SchemaPrivateData>` by `try_new`.
+ // We take ownership temporarily with `from_raw` and put it back with
`into_raw`,
+ // so there is no double-free and no other code can access
`private_data` concurrently.
unsafe {
let mut private_data =
Box::from_raw(self.private_data.cast::<SchemaPrivateData>());
private_data.metadata = new_metadata;
@@ -403,6 +406,8 @@ impl FFI_ArrowSchema {
///
/// This must be `Some` if the schema represents a dictionary-encoded
type, `None` otherwise.
pub fn dictionary(&self) -> Option<&Self> {
+ // Safety: per the C Data Interface spec, `self.dictionary` is either
null (returns None)
+ // or a valid pointer to an `FFI_ArrowSchema` that lives at least as
long as `self`.
unsafe { self.dictionary.as_ref() }
}
@@ -429,6 +434,8 @@ impl FFI_ArrowSchema {
let buffer = self.metadata.cast::<u8>();
fn next_four_bytes(buffer: *const u8, pos: &mut isize) -> [u8; 4] {
+ // Safety: the caller advances `pos` only by the number of
bytes consumed,
+ // so `*pos..*pos+4` is always within the bounds of the
metadata buffer.
let out = unsafe {
[
*buffer.offset(*pos),
@@ -442,6 +449,7 @@ impl FFI_ArrowSchema {
}
fn next_n_bytes(buffer: *const u8, pos: &mut isize, n: i32) ->
&[u8] {
+ // Safety: same as `next_four_bytes`; `*pos..*pos+n` is within
the metadata buffer.
let out = unsafe {
std::slice::from_raw_parts(buffer.offset(*pos),
n.try_into().unwrap())
};
@@ -487,6 +495,9 @@ impl Drop for FFI_ArrowSchema {
fn drop(&mut self) {
match self.release {
None => (),
+ // Safety: the release callback was set by the schema producer and
follows the
+ // C Data Interface contract: it frees all resources associated
with the schema
+ // and sets `release` to None. `self` is a valid, non-null pointer
here.
Some(release) => unsafe { release(self) },
}
}
