Copying inappropriately aligned buffer in ipc reader #2883

viirya · 2022-10-16T02:09:40Z

Which issue does this PR close?

Closes #2882.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2022-10-16T03:23:26Z

I don't think this is correct, we rely on the memory within PrimitiveArray being correctly aligned?

tustvold · 2022-10-16T03:26:39Z

arrow-array/src/array/list_array.rs

+            .unwrap();
+        let array = Int32Array::from(array_data);
+        assert_eq!(array.len(), 1);
+        assert_eq!(array.value(0), 0);


This is now UB, as this violates the safety requirement in PrimitiveArray::values.

I'm not sure why MIRI isn't catching this...

Edit: array.value doesn't call array.values so this test isn't UB. If you add a call to array.values() in this test, MIRI will fail

viirya · 2022-10-16T03:30:56Z

I don't think this is correct, we rely on the memory within PrimitiveArray being correctly aligned?

As described in the ticket, for IPC reader where we share same memory and slice with offset and length for individual arrays. The alignment check applies on the entire memory allocation not slices. There will be unexpected alignment check error.

tustvold

As written this PR is unsound, as PrimitiveArray::values requires the values pointer to be correctly aligned.

tustvold · 2022-10-16T03:35:02Z

arrow-array/src/array/list_array.rs

@@ -878,6 +878,7 @@ mod tests {
        assert_eq!(array.len(), 2);
        assert_eq!(array.value(0), 0);
        assert_eq!(array.value(1), 0);
+        assert_eq!(array.values(), &[0, 0]);


You need to call array.values() on the unaligned PrimitiveArray

viirya · 2022-10-16T03:37:18Z

As written this PR is unsound, as PrimitiveArray::values requires the values pointer to be correctly aligned.

The alignment soundness in PrimitiveArray::values is guaranteed by is_aligned_and_not_null. This is how it is implemented:

pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
    !ptr.is_null() && ptr.addr() % mem::align_of::<T>() == 0
}

It only checks if the ptr address is aligned with type T.

tustvold · 2022-10-16T03:37:53Z

The alignment check applies on the entire memory allocation not slices.

The alignment of the entire memory allocation is irrelevant, we only care that that the buffers within it are correctly aligned. My memory is a bit fuzzy, but I seem to remember that the arrow specification goes to great lengths to document how data should be padded to guarantee alignment. This should mean that if the original allocation is aligned (something we should double-check we are actually guaranteeing), and the padding is correct, data can be zero-copy sliced - otherwise we have to copy.

viirya · 2022-10-16T03:38:15Z

arrow-array/src/array/list_array.rs

+        let array = Int32Array::from(array_data);
+        assert_eq!(array.len(), 1);
+        assert_eq!(array.value(0), 0);
+        assert_eq!(array.values(), &[0]);


For https://github.com/apache/arrow-rs/pull/2883/files#r996382807, I think you mean here?

viirya · 2022-10-16T03:43:29Z

The alignment of the entire memory allocation is irrelevant, we only care that that the buffers within it are correctly aligned.

align_offset actually checks the entire memory allocation. That said we allocate a memory allocation in IPC reader for all buffers data. Then we slice it for offset/length for each buffers without copying. When we construct PrimitiveArray from one (sliced) buffer, align_offset checks if the entire memory allocation aligns with type T. And it is obviously not guaranteed to be.

tustvold · 2022-10-16T03:49:04Z

And it is obviously not guaranteed to be.

It is supposed to be - https://arrow.apache.org/docs/format/Columnar.html#buffer-alignment-and-padding by ensuring the buffers are padded correctly, we can ensure that correctly aligning the entire memory allocation is sufficient to align the child allocations. When the writer has not done this correctly, we will have to copy the buffers or return an error.

The alignment soundness in PrimitiveArray::values is guaranteed by is_aligned_and_not_null.

Where is this called, I think I am being blind. It looks like this PR removes the alignment check in PrimitiveArray?

viirya · 2022-10-16T03:54:38Z

Where is this called, I think I am being blind. It looks like this PR removes the alignment check in PrimitiveArray?

Hmm? No, this PR doesn't remove it.

Let me quote what I saw for now:

pub fn values(&self) -> &[T::Native] {
        // Soundness
        //     raw_values alignment & location is ensured by fn from(ArrayDataRef)
        //     buffer bounds/offset is ensured by the ArrayData instance.
        unsafe {
            std::slice::from_raw_parts(
                self.raw_values.as_ptr().add(self.data.offset()),
                self.len(),
            )
        }
    }

pub const unsafe fn from_raw_parts<'a, T>(data: *const T, len: usize) -> &'a [T] {
    // SAFETY: the caller must uphold the safety contract for `from_raw_parts`.
    unsafe {
        assert_unsafe_precondition!(
            is_aligned_and_not_null(data)
                && crate::mem::size_of::<T>().saturating_mul(len) <= isize::MAX as usize
        );
        &*ptr::slice_from_raw_parts(data, len)
    }
}

pub(crate) fn is_aligned_and_not_null<T>(ptr: *const T) -> bool {
    !ptr.is_null() && ptr.addr() % mem::align_of::<T>() == 0
}

BTW, I'm on a M1 Macbook so the toolchain is stable-aarch64-apple-darwin. Maybe you will see something different?

tustvold · 2022-10-16T03:58:36Z

assert_unsafe_precondition is only checked in debug builds, although something is off as I would expect this to fire with your test.

My understanding of the test is you are explicitly creating a raw_values pointer that is not aligned to T, which should then trigger the assert and MIRI to fail?

tustvold · 2022-10-16T03:59:58Z

arrow-array/src/array/list_array.rs

@@ -861,16 +861,42 @@ mod tests {
    }

    #[test]
-    #[should_panic(expected = "memory is not aligned")]
+    #[should_panic(expected = "Need at least 8 bytes in buffers[0]")]


This is why MIRI isn't failing and the debug assertion isn't firing, the test panics before it does anything interesting... 🤦

viirya · 2022-10-16T04:08:00Z

It is supposed to be - https://arrow.apache.org/docs/format/Columnar.html#buffer-alignment-and-padding by ensuring the buffers are padded correctly, we can ensure that correctly aligning the entire memory allocation is sufficient to align the child allocations. When the writer has not done this correctly, we will have to copy the buffers or return an error.

How it can be?

That's said we allocate a 80 bytes memory allocation for all buffers containing 1 Int32Array (40 bytes) and and 1 Decimal128Array (32 bytes) and 1 Int32Array (8 bytes).

When we try to create array for the Decimal128Array, it takes the slice buffer offset by 40 to 80. Then it will fails at align_offset as it is not aligned with i128 (16 bytes).

tustvold · 2022-10-16T04:12:45Z

How it can be?

Because the allocation containing all the buffers is aligned to at least 8 bytes, and all the contained buffers are padded to a multiple of 8 bytes in length, each buffer starts and ends at an 8 byte boundary.

As mentioned on the ticket, the issue appears to be that arm requires 16-byte alignment for i128 types, which isn't guaranteed by the standard which only mandates padding up to 8 bytes. As such we will need to copy the buffer to a new correctly aligned allocation in such a case. We could/should probably do this in general where the buffer is not sufficiently aligned for its type

viirya · 2022-10-16T04:36:04Z

Because the allocation containing all the buffers is aligned to at least 8 bytes, and all the contained buffers are padded to a multiple of 8 bytes in length, each buffer starts and ends at an 8 byte boundary.

Currently padding only guarantees 8 bytes alignment. Any larger alignment requirement can just fail the check.

I'm not sure if the official doc explicitly asks for a 8 bytes alignment. For example,

Implementations are recommended to allocate memory on aligned addresses (multiple of 8- or 64-bytes) and pad (overallocate) to a length that is a multiple of 8 or 64 bytes.

and for IPC,

The body, a flat sequence of memory buffers written end-to-end with appropriate padding to ensure a minimum of 8-byte alignment

It sounds like it can be any alignment larger than 8 bytes. Maybe we can change to 16 bytes alignment?

As mentioned on the ticket, the issue appears to be that arm requires 16-byte alignment for i128 types, which isn't guaranteed by the standard which only mandates padding up to 8 bytes. As such we will need to copy the buffer to a new correctly aligned allocation in such a case. We could/should probably do this in general where the allocation is not sufficiently aligned.

Hmm, for now this sounds like a special case (DecimalArray + arm). I'm okay for the copying approach. I will modify this.

tustvold · 2022-10-16T04:41:19Z

It sounds like it can be any alignment larger than 8 bytes.

Correct

Maybe we can change to 16 bytes alignment?

For buffers allocated by arrow-rs we use larger alignments (32 bytes on arm, 128 bytes on x86) - see https://github.com/apache/arrow-rs/blob/master/arrow-buffer/src/alloc/alignment.rs. I presume this carries across to IPC files we write, but I have not verified this.

The issue is whatever wrote the test file in the ticket was only using the minimum 8 byte padding, and so we need to copy in such cases

Edit: It would appear we also only write with an alignment of 8 bytes, we should change this.

Hmm, for now this sounds like a special case (DecimalArray + arm)

I think it will also impact IntervalMonthDayNanoType which also uses i128

I'm okay for the copying approach

TBC we should only copy as a fallback for when the buffer is not sufficiently aligned. We could probably do this in the general case, it is better than panicking.

* Increase default IPC alignment to 64 (#2883) * Update test

tustvold · 2022-10-16T06:16:34Z

arrow/src/ipc/reader.rs

+
+/// Calculate byte boundary and return the number of bytes needed to pad to `align_req` bytes
+#[inline]
+fn padding(len: usize, align_req: usize) -> usize {


This is already handled for you by MutableBuffer, you shouldn't need to add explicit padding

tustvold · 2022-10-16T06:18:37Z

arrow/src/ipc/reader.rs

+        let len_in_bytes = length * std::mem::size_of::<i128>();
+        let pad_len = padding(len_in_bytes, align_req);
+        let mut aligned_buffer = MutableBuffer::with_capacity(len_in_bytes + pad_len);
+        aligned_buffer.extend_from_slice(&buffer.as_slice()[0..len_in_bytes]);
+        aligned_buffer.extend_from_slice(&vec![0u8; pad_len]);
+        aligned_buffer.into()


Suggested change

let len_in_bytes = length * std::mem::size_of::<i128>();

let pad_len = padding(len_in_bytes, align_req);

let mut aligned_buffer = MutableBuffer::with_capacity(len_in_bytes + pad_len);

aligned_buffer.extend_from_slice(&buffer.as_slice()[0..len_in_bytes]);

aligned_buffer.extend_from_slice(&vec![0u8; pad_len]);

aligned_buffer.into()

Buffer::from_slice_ref(buffer.as_slice)

viirya · 2022-10-16T06:34:19Z

arrow/src/ipc/reader.rs

+        let len_in_bytes = length * std::mem::size_of::<i128>();
+        let slice = &buffer.as_slice()[0..len_in_bytes];


We only need copying the range of current buffer (i.e., length).

tustvold · 2022-10-16T08:01:10Z

arrow/src/ipc/reader.rs

+    // e.g. 8 bytes, but on some platform (e.g. ARM) i128 requires 16 bytes alignment.
+    // We need to copy the buffer as fallback.
+    if align_offset != 0 {
+        let len_in_bytes = length * std::mem::size_of::<i128>();


I think this is incorrect for the Decimal256 case? Perhaps we could make this method generic on the native type, to ensure the correct size and alignment is used?

The alignment req for i256 is also 16. But sounds better to make it generic.

It was actually the length I was concerned about

tustvold · 2022-10-16T17:45:45Z

arrow/src/ipc/reader.rs

            .len(length)
            .add_buffer(buffers[1].clone())
            .null_bit_buffer(null_buffer)
            .build()
            .unwrap(),
-        Decimal128(_, _) | Decimal256(_, _) => {
+        Interval(IntervalUnit::MonthDayNano) | Decimal128(_, _) | Decimal256(_, _) => {
+            let buffer = if matches!(data_type, &DataType::Decimal256(_, _)) {


Why not lift into parent match?

Save a few lines? :) I lifted it now.

tustvold · 2022-10-16T17:47:54Z

arrow/src/ipc/reader.rs

+    // We need to copy the buffer as fallback.
+    if align_offset != 0 {
+        let len_in_bytes = length * std::mem::size_of::<T>();
+        let slice = &buffer.as_slice()[0..len_in_bytes];


Minor nit, this will panic for invalid data where previously we would error. Should just be a case of taking the minimum of expected and actual length

arrow/src/ipc/reader.rs

viirya · 2022-10-16T19:35:15Z

Thanks @tustvold !

ursabot · 2022-10-16T19:41:28Z

Benchmark runs are scheduled for baseline = a3effc1 and contender = bfd87bd. bfd87bd is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Fix ptr alignment error.

aba2874

github-actions bot added the arrow Changes to the arrow crate label Oct 16, 2022

Rewrite test

e184bad

viirya force-pushed the fix_align_error branch from be2d47b to e184bad Compare October 16, 2022 02:45

tustvold reviewed Oct 16, 2022

View reviewed changes

tustvold requested changes Oct 16, 2022

View reviewed changes

Add values

705aefa

tustvold reviewed Oct 16, 2022

View reviewed changes

viirya commented Oct 16, 2022

View reviewed changes

tustvold reviewed Oct 16, 2022

View reviewed changes

tustvold added a commit to tustvold/arrow-rs that referenced this pull request Oct 16, 2022

Increase default IPC alignment to 64 (apache#2883)

e364395

tustvold mentioned this pull request Oct 16, 2022

Increase default IPC alignment to 64 (#2883) #2884

Merged

viirya added 3 commits October 15, 2022 23:03

Copy buffer if it is not aligned properly

d2ccba5

Move to a function

400cf81

Cover IntervalMonthDayNanoType too

6a56057

viirya changed the title ~~Skip memory alignment check when constructing PrimitiveArray from an array data reference~~ Copying inappropriately aligned buffer in ipc reader Oct 16, 2022

tustvold added a commit that referenced this pull request Oct 16, 2022

Increase default IPC alignment to 64 (#2883) (#2884)

a3effc1

* Increase default IPC alignment to 64 (#2883) * Update test

Remove unnecessary change

30ffb5c

tustvold reviewed Oct 16, 2022

View reviewed changes

For review

5dc264a

viirya commented Oct 16, 2022

View reviewed changes

tustvold reviewed Oct 16, 2022

View reviewed changes

Make it generic for i256

1538455

tustvold approved these changes Oct 16, 2022

View reviewed changes

tustvold reviewed Oct 16, 2022

View reviewed changes

Lift into parent match and use minimum length.

96577a6

viirya force-pushed the fix_align_error branch from c85e2ad to 96577a6 Compare October 16, 2022 17:51

tustvold reviewed Oct 16, 2022

View reviewed changes

arrow/src/ipc/reader.rs Show resolved Hide resolved

viirya merged commit bfd87bd into apache:master Oct 16, 2022

alamb mentioned this pull request Oct 28, 2022

Memory alignment error in RawPtrBox::new #2882

Closed

hzuo mentioned this pull request Mar 26, 2024

IPC code writes data with insufficient alignment #5553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copying inappropriately aligned buffer in ipc reader #2883

Copying inappropriately aligned buffer in ipc reader #2883

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022

tustvold Oct 16, 2022 •

edited

Loading

viirya commented Oct 16, 2022

tustvold left a comment

tustvold Oct 16, 2022

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 •

edited

Loading

viirya Oct 16, 2022

viirya commented Oct 16, 2022 •

edited

Loading

tustvold commented Oct 16, 2022 •

edited

Loading

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022

tustvold Oct 16, 2022 •

edited

Loading

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 •

edited

Loading

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 •

edited

Loading

tustvold Oct 16, 2022

tustvold Oct 16, 2022

viirya Oct 16, 2022

tustvold Oct 16, 2022

viirya Oct 16, 2022

tustvold Oct 16, 2022

tustvold Oct 16, 2022

viirya Oct 16, 2022

tustvold Oct 16, 2022

viirya commented Oct 16, 2022

ursabot commented Oct 16, 2022

		let len_in_bytes = length * std::mem::size_of::<i128>();
		let slice = &buffer.as_slice()[0..len_in_bytes];

Copying inappropriately aligned buffer in ipc reader #2883

Copying inappropriately aligned buffer in ipc reader #2883

Conversation

viirya commented Oct 16, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold commented Oct 16, 2022

tustvold Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

viirya commented Oct 16, 2022

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

viirya commented Oct 16, 2022 • edited Loading

tustvold commented Oct 16, 2022 • edited Loading

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022

tustvold Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 • edited Loading

viirya commented Oct 16, 2022

tustvold commented Oct 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Oct 16, 2022

ursabot commented Oct 16, 2022

tustvold Oct 16, 2022 •

edited

Loading

tustvold commented Oct 16, 2022 •

edited

Loading

viirya commented Oct 16, 2022 •

edited

Loading

tustvold commented Oct 16, 2022 •

edited

Loading

tustvold Oct 16, 2022 •

edited

Loading

tustvold commented Oct 16, 2022 •

edited

Loading

tustvold commented Oct 16, 2022 •

edited

Loading