Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ARROW-7624: [Rust] Soundness issues via Buffer methods #6397

Closed
wants to merge 4 commits into from

Conversation

paddyhoran
Copy link
Contributor

No description provided.

@github-actions
Copy link

@jturner314
Copy link

Would it be helpful for me to review this? At first glance, I noticed another issue. (The data method creates a slice from a pointer which may be null, which violates the safety constraints of std::slice::from_raw_parts.)

I do have one question before reviewing in more detail. I'm confused about the ownership semantics of BufferData. At first glance, it looks similar to Box<[u8]> (i.e. an owned block of data), but the owned field suggests otherwise. What is the intended use for an instance of BufferData with owned == false? If it's supposed to be analogous to &[u8], then BufferData should have a lifetime annotation indicating the lifetime of the borrow.

@paddyhoran
Copy link
Contributor Author

Would it be helpful for me to review this? At first glance, I noticed another issue.

Absolutely, that would be great!

I do have one question before reviewing in more detail. I'm confused about the ownership semantics of BufferData. At first glance, it looks similar to Box<[u8]> (i.e. an owned block of data), but the owned field suggests otherwise.

It is an owned block of data and I would say that owned=false is very seldom used. It was introduced to support the situation where the memory was allocated by one of the other Arrow implementations, specifically via JNI from Java. @liurenjie1024 please correct me if I'm wrong?

Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liurenjie1024
Copy link
Contributor

@paddyhoran Yes, you are right.

LGTM.

@jturner314
Copy link

Reading over the code, I found a few more issues in memory.rs/buffer.rs:

  • The functions in memory need more checks and/or need to be made unsafe to avoid undefined behavior, primarily involving zero-size allocations. (See the GlobalAlloc docs for more details.)
  • Allocation errors (i.e. the allocator returning a null pointer) are not properly handled.
  • There are various issues related to null pointers and the buffer types. IMO, the best approach is for the buffer types to enforce non-nullness of their pointers (assuming that's compatible with the rest of Arrow?).
  • The derived PartialEq implementation of Buffer compares all the bytes in the underlying BufferData, including those before the offset. I assume this is incorrect, and the desired behavior is to compare only the visible bytes (those starting at offset).
  • MutableBuffer::freeze creates a BufferData directly from the pointer and length, which results in the BufferData freeing the allocation with an incorrect size when the capacity and length of the MutableBuffer were unequal. (MutableBuffer needs to deallocate the excess capacity before creating the BufferData.)

I have a work-in-progress set of changes on the from-raw-parts-unsafe branch of my fork (see diff from this PR).

@Marwes
Copy link

Marwes commented Feb 11, 2020

@jturner314 A few of those issues are also fixed in #6395. Started going through buffer a bit over the weekend.

@paddyhoran
Copy link
Contributor Author

Thanks @jturner314 and @Marwes.

I need to take a look at the issues around allocation (the first 3 points above).

  • The derived PartialEq implementation of Buffer compares all the bytes in the underlying BufferData, including those before the offset. I assume this is incorrect, and the desired behavior is to compare only the visible bytes (those starting at offset).

Actually, I raised this issue when this was merged. This is expected, the higher level array types allow for the offset and len. The thinking is that Buffer's PartialEq allows you to compare the allocated memory (i.e. it's lower level). Others thought this made more sense... at very least we should update the docs to explain this.

  • MutableBuffer::freeze creates a BufferData directly from the pointer and length, which results in the BufferData freeing the allocation with an incorrect size when the capacity and length of the MutableBuffer were unequal. (MutableBuffer needs to deallocate the excess capacity before creating the BufferData.)

So, we pad to 64 bytes in Rust. This is mentioned in the spec. This allows us to use SIMD easily, basically we allow SIMD to operate on the padded region so that's why the padded region is included in BufferData. (@Marwes this might be why your tests are failing on #6395, but I need to look at this in more detail.)

Anyone have any issues with merging this and opening follow up JIRA's for the other items?

@jturner314
Copy link

jturner314 commented Feb 11, 2020

I just noticed another issue -- the new_len > self.len case of the resize method on MutableBuffer extends the buffer with uninitialized memory. Accessing this data (e.g. via the data method) is undefined behavior, even though the buffer just deals with raw bytes. (Relevant quote from the docs for MaybeUninit: "Moreover, uninitialized memory is special in that the compiler knows that it does not have a fixed value. This makes it undefined behavior to have uninitialized data in a variable even if that variable has an integer type, which otherwise can hold any fixed bit pattern".)

Since you're depending on padding outside of the length for use in SIMD (which means the padding needs to be initialized), perhaps the simplest solution would be to always get initialized memory (replace calls to std::alloc::alloc with std::alloc::alloc_zeroed [edit: and change memory::reallocate to zero out new capacity]).

A few of those issues are also fixed in #6395. Started going through buffer a bit over the weekend.

Thanks for pointing out that PR. It looks like it fixes most of the issues related to null pointers in the buffer types. The remaining thing I see is that the reserve and resize methods on MutableBuffer call memory::reallocate with the possibly-null self.data pointer, which due to the way reallocate is currently implemented, result in calling std::alloc::realloc with a null pointer (which is undefined behavior).

The thinking is that Buffer's PartialEq allows you to compare the allocated memory (i.e. it's lower level). Others thought this made more sense... at very least we should update the docs to explain this.

Okay, that's reasonable.

So, we pad to 64 bytes in Rust. This is mentioned in the spec. This allows us to use SIMD easily, basically we allow SIMD to operate on the padded region so that's why the padded region is included in BufferData.

Padding to 64 bytes is fine. There are a couple of things to be careful about, though. The first is that if code accesses the padding, e.g. for SIMD, it needs to be initialized. The second is that when the memory is freed, it needs to be freed with the same layout it was allocated with (the "layout" is the size and alignment). The capacity field of MutableBuffer describes the size of the allocation, so creating a BufferData using len (which often is unequal to capacity) and then dropping it results in calling dealloc with the incorrect allocation size (i.e. len instead of capacity), which is undefined behavior. If the padding needs to be preserved in BufferData, then either (a) BufferData needs to store the capacity to use when calling dealloc or (b) BufferData needs to be changed to use the "length rounded up to the nearest multiple of 64" when calling dealloc and all methods of creating BufferData (including MutableBuffer's freeze method) need to ensure that the allocation size matches the "length rounded up to the nearest multiple of 64".

Anyone have any issues with merging this and opening follow up JIRA's for the other items?

Sure, that's okay with me.The remaining issues can be fixed in other PRs.

@paddyhoran paddyhoran deleted the from-raw-parts-unsafe branch February 12, 2020 03:18
@paddyhoran
Copy link
Contributor Author

@jturner314 does #4331 address the de-allocation issue?

I opened this JIRA for the initialization issue.

@jturner314
Copy link

@paddyhoran Yes, #4331 fixes the issue I was describing regarding MutableBuffer::freeze followed by deallocating the Buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants