ARROW-7813: [Rust] Remove and fix unsafe code by Marwes · Pull Request #6395 · apache/arrow

Marwes · 2020-02-10T10:21:20Z

This removes or corrects many instances of unsafe code in the rust crates. This is by no means a complete fix and some of the fixes do not entirely fix the issues with the particular unsafe (see comments), but in all instances it should put the code in a better place than before.

Based on #6256

github-actions · 2020-02-10T10:32:40Z

https://issues.apache.org/jira/browse/ARROW-7813

Marwes · 2020-02-13T16:25:12Z

Removed the commits that changed the simd implemented operators. The code is definitely doing undefined behaviour though as the padding won't be initialized ( #6397 (comment) ).

So either the code could do non-simd operations for the last partial block (if any). Or the padding should be initialized before reading it.

paddyhoran · 2020-02-13T20:39:00Z

So either the code could do non-simd operations for the last partial block (if any). Or the padding should be initialized before reading it.

I plan to initialize the padded region, ARROW-7836 if no-one beats me to it.

paddyhoran · 2020-02-13T20:39:57Z

I'll try and review this tomorrow so we can get it merged.

sunchao · 2020-02-15T18:08:33Z

Will take a look too. This needs rebase BTW. And cc @sadikovi .

paddyhoran · 2020-02-24T03:52:51Z

Ping @Marwes can you re-base this so we can review if you get a chance please?

andygrove · 2020-03-28T12:45:54Z

@Marwes Could you rebase this ?

Marwes · 2020-04-01T07:50:39Z

Rebased, CI is failing though. Not sure what to do to fix.

paddyhoran · 2020-04-04T00:21:05Z

This was fixed by #6800. Let me see if I can re-trigger CI.

paddyhoran · 2020-04-04T00:26:23Z

@kszucs how can I re-trigger CI? I tried to force-push to this branch but it has no effect.

wesm · 2020-04-04T20:54:21Z

Not sure what happened with GitHub (they have been having outage issues?) but I rebased and CI is running again

nevi-me · 2020-04-04T21:21:41Z

CI is failing with unnecessary unsafe blocks on parquet\src\arrow\array_reader.rs

paddyhoran · 2020-04-12T17:02:34Z

@Marwes looks like just some basic issues with this, want to take a quick look? I'd like to get this one merged.

bluss · 2020-04-12T18:04:44Z

(I was "invited" to look at this PR, since I noticed some issues that needed fixing. All my comments will appear a bit random and semi-related to this PR, because of this. I will only comment on important issues.)

Nice, I had named 3 things I saw in arrow 0.16.0 code (not a full review)

Issue (1) about Buffer.ptr being null is being fixed here in this PR. Another common way to solve this in Rust is to use https://doc.rust-lang.org/std/ptr/struct.NonNull.html#method.dangling - an aligned non-null dummy pointer, but I won't review this solution further, Marwes can probably judge it all as well as I can. Using NonNull<T> is certainly an idea, too.
Issue (3) about making Buffer::from_raw_partsunsafe has been merged as a fix in an older PR.

Issue (2) so remains, no check for allocation failure. This produces dangerous mismatches - null pointer and nonzero length - in various places.

This doesn't have to be fixed in this PR, just to note that the issue is not fixed here.

Sign in to view

bluss · 2020-04-15T09:17:20Z

Sign in to view

This fix has disappeared, I can't see it in the diff anymore 😕

I opened ARROW-8479

Might be other platforms this works on as well, but those could be added as they are found

Fixes undefined behaviour that could occur from safe code calling with `T == Box<i32>` etc.

This may cause panics in code using ByteArray or Int96 but no tests currently test these paths. Still better than the status quo which would be undefined behaviour (if the replaced code paths were hit).

Some types such as `&[Int96]` and `&[ByteArray]` can't be transmuted to a `&[u8]` as they aren't plain old data. `&mut bool` can't be transmuted to a `&mut u8` since that would allow writing values other than 0 or 1 to it.

Marwes · 2020-04-14T07:58:16Z

Issue (1) about Buffer.ptr being null is being fixed here in this PR. Another common way to solve this in Rust is to use https://doc.rust-lang.org/std/ptr/struct.NonNull.html#method.dangling - an aligned non-null dummy pointer, but I won't review this solution further, Marwes can probably judge it all as well as I can. Using NonNull is certainly an idea, too.

That's the better solution. I just spotted the null pointer issue late in this PR and didn't want to complicate it further.

Fixed the review comments.

bluss · 2020-04-14T11:28:12Z

Thank you @Marwes for doing big work like this for better and safer code

paddyhoran

Thank you @Marwes for doing big work like this for better and safer code

Absolutely, thanks very much @Marwes! LGTM from Arrow (crate) perspective. @sunchao can you take a quick look at the changes to the Parquet crate please?

wesm · 2020-04-14T16:03:51Z

Would be great to get this into 0.17.0, probably needs to be merged today

cc @kszucs

sunchao

+1. I only had time to skim through this but it'd be great if we have some perf numbers to show there is no regression.

kszucs · 2020-04-15T00:20:05Z

@sunchao Included in the 0.17 release. If you find a significant regression, then we can still fix it by cutting a new release candidate.

paddyhoran added the Component: Rust label Feb 10, 2020

Marwes mentioned this pull request Feb 11, 2020
ARROW-7624: [Rust] Soundness issues via Buffer methods #6397
Closed

Marwes force-pushed the unsafe_rust_2 branch from 257e947 to 5c8a373Compare February 13, 2020 14:14

Marwes force-pushed the unsafe_rust_2 branch from f85ca4a to a5479dcCompare April 1, 2020 07:08

wesm force-pushed the unsafe_rust_2 branch from a5479dc to b34f1fdCompare April 4, 2020 20:53

paddyhoran mentioned this pull request Apr 12, 2020
Adoption of the Apache Arrow memory alignment and padding? rust-ndarray/ndarray#771
Open

bluss reviewed Apr 12, 2020
View reviewed changes

Comment threadrust/arrow/src/buffer.rs Outdated
This comment was marked as resolved.
Sign in to view

bluss reviewed Apr 13, 2020
View reviewed changes

Comment threadrust/arrow/src/buffer.rs Outdated
This comment was marked as resolved.
Sign in to view

bluss reviewed Apr 13, 2020
View reviewed changes

Markus Westerlind added 7 commits April 14, 2020 09:42

Better error message for missing test data
62919d4

fix: murmur hash is only safe on platforms with unaligned loads
5a15589
Might be other platforms this works on as well, but those could be added as they are found

fix: Use safe code to read types in bit_util
5681258
Fixes undefined behaviour that could occur from safe code calling with `T == Box<i32>` etc.

Remove unnecessary unsafe in memcpy functions
44bf359

fix: Don't allow arbitrary transmutes when decoding
27b7b29
This may cause panics in code using ByteArray or Int96 but no tests currently test these paths. Still better than the status quo which would be undefined behaviour (if the replaced code paths were hit).

Remove unnecessary unsafe in Int96 construction
20e62c4

fix: Don't trust parquet data to be UTF-8
4df3f2b

Markus Westerlind added 13 commits April 14, 2020 09:42

refactor: Remove unnecessary unsafe in decoding
5a69773

fix: Restrict which types can be transmuted to byte slices
fb719eb
Some types such as `&[Int96]` and `&[ByteArray]` can't be transmuted to a `&[u8]` as they aren't plain old data. `&mut bool` can't be transmuted to a `&mut u8` since that would allow writing values other than 0 or 1 to it.

refactor: Remove unnecessary unsafe from rle encoding
c9996be

fix: Prevent aliasing of &mut with FatPtr by threading lifetimes
fc0d0d4

fix: Don't alias &mut from bit_writer
b73debc

refactor: Avoid unsafe in put
92e3205

refactor: Avoid unsafe when casting RecordReader
46a894b

refactor: Move unsafe into MutableBuffer
425ba07

fix: Don't create a slice from a null pointer when calling Buffer::data
c606440

refactor: No need for unsafe in Debug impl for Buffer
52cc391

Fix fn data
6990951

Fix non-simd compilation
cfb8496

Fix bluss's review comments
45cccc4

Marwes force-pushed the unsafe_rust_2 branch from b34f1fd to 45cccc4Compare April 14, 2020 07:52

paddyhoran approved these changes Apr 14, 2020
View reviewed changes

sunchao approved these changes Apr 14, 2020
View reviewed changes

nealrichardson closed this in 3ca2e19Apr 14, 2020

Marwes deleted the unsafe_rust_2 branch April 15, 2020 09:33

alamb mentioned this pull request Apr 26, 2021
Use "raw_data_mut" in "Buffer::typed_data_mut" apache/arrow-rs#64
Closed

asfimport mentioned this pull request Apr 16, 2020
[Rust] Fix undefined behaviour and and remove unsafe #24044
Closed

Uh oh!

Conversation

Marwes commented Feb 10, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actionsBot commented Feb 10, 2020

Uh oh!

Marwes commented Feb 13, 2020

Uh oh!

paddyhoran commented Feb 13, 2020

Uh oh!

paddyhoran commented Feb 13, 2020

Uh oh!

sunchao commented Feb 15, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paddyhoran commented Feb 24, 2020

Uh oh!

andygrove commented Mar 28, 2020

Uh oh!

Marwes commented Apr 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paddyhoran commented Apr 4, 2020

Uh oh!

paddyhoran commented Apr 4, 2020

Uh oh!

wesm commented Apr 4, 2020

Uh oh!

nevi-me commented Apr 4, 2020

Uh oh!

paddyhoran commented Apr 12, 2020

Uh oh!

bluss commented Apr 12, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

Uh oh!

blussApr 15, 2020

Choose a reason for hiding this comment

Uh oh!

paddyhoranApr 16, 2020

Choose a reason for hiding this comment

Uh oh!

Marwes commented Apr 14, 2020

Uh oh!

bluss commented Apr 14, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paddyhoran left a comment

Choose a reason for hiding this comment

Uh oh!

wesm commented Apr 14, 2020

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

kszucs commented Apr 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Marwes commented Feb 10, 2020•
edited
Loading

sunchao commented Feb 15, 2020•
edited
Loading

Marwes commented Apr 1, 2020•
edited
Loading

bluss commented Apr 12, 2020•
edited
Loading

bluss commented Apr 14, 2020•
edited
Loading