Consider Vec::from_iter in Buffer::from_iter

[shinmao]

The source of unsoundness

Hi, we are the Rust researchers from SunLab. Recently, we are trying to test our static analysis tool and find the potential unsound function in the package of arrow-buffer:

arrow-rs/arrow-buffer/src/buffer/immutable.rs

Lines 417 to 432 in 90bc5ec

	impl<T: ArrowNativeType> FromIterator<T> for Buffer {
	fn from_iter<I: IntoIterator<Item = T>>(iter: I) -> Self {
	let mut iterator = iter.into_iter();
	let size = std::mem::size_of::<T>();
	// first iteration, which will likely reserve sufficient space for the buffer.
	let mut buffer = match iterator.next() {
	None => MutableBuffer::new(0),
	Some(element) => {
	let (lower, _) = iterator.size_hint();
	let mut buffer = MutableBuffer::new(lower.saturating_add(1) * size);
	unsafe {
	std::ptr::write(buffer.as_mut_ptr() as *mut T, element);
	buffer.set_len(size);
	}
	buffer

In line 429, when the mutable pointer is cast to generic type, it is actually casting from u8 to any-aligned type which could create a misaligned pointer. However, it seems that the programmers are also aware of the bugs in the test case:

arrow-rs/arrow-buffer/src/buffer/scalar.rs

Lines 224 to 231 in 90bc5ec

	#[should_panic(
	expected = "Memory pointer is not aligned with the specified scalar type"
	)]
	fn test_unaligned() {
	let expected = [0_i32, 1, 2];
	let buffer = Buffer::from_iter(expected.iter().cloned());
	let buffer = buffer.slice(1);
	ScalarBuffer::<i32>::new(buffer, 0, 2);

Here, u8 is cast to i32 and create a misaligned pointer and panic occurs.
We consider that it should not happen in safe API anyway based on the safety requirement of ptr.write(). The problem could be fixed by passing the layout of element to allocate the memory.
We would love to have more discussion, and please correct if we are wrong.

[tustvold]

it is actually casting from u8 to any-aligned type which could create a misaligned pointer

MutableBuffer uses an aligned allocator, with an alignment exceeding that of any ArrowNativeType, so this should be fine.

That being said we can probably eliminate this unsafe, without regressing performance

[shinmao]

@tustvold thanks for the response. Then, why does it still panic?

[tustvold]

Because the Buffer::slice which comes after in the test is shifting it so that Buffer is no longer aligned, but unlike MutableBuffer, Buffer has no alignment restrictions so this is safe

For context these APIs exist to accommodate zero-copy data exchange over FFI and IPC, along with custom allocators and over-alignment for SIMD, they're definitely on the funky end and it'd be great to get more eyes on what they are doing

[shinmao]

Fair enough. Thanks!

[shinmao]

Yes. I just realized these two lines after discussion with you 😂

arrow-rs/arrow-buffer/src/buffer/mutable.rs

Lines 72 to 73 in 90bc5ec

	let capacity = bit_util::round_upto_multiple_of_64(capacity);
	let layout = Layout::from_size_align(capacity, ALIGNMENT).unwrap();

[tustvold]

I've taken the liberty of updating the issue title to rephrase this into an enhancement proposal. I think this should be relatively straightforward, and less unsafe is always a good thing, the major task would be making sure it doesn't regress anything