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

Fix invalid aliasing and uninint memory in streams #592

Closed
wants to merge 2 commits into from

Conversation

saethlin
Copy link
Contributor

@saethlin saethlin commented Jan 3, 2022

In its current state, running cargo miri test on this codebase flags a number of problems. This PR fixes all of those. It unfortunately does not make MIRIFLAGS="-Zmiri-tag-raw-pointers" cargo miri test pass completely, because bytes has a soundness problem which I've also put up a PR to address, and is in another aspect unanalyzable (currently?) for miri.

Using remove_lifetime_mut to construct then hold on to both a Vec and a mutable slice into it violates the existing Rust aliasing rules in this particular usage pattern, because both &mut and Unique<T> (which is used to implement Vec) assert unique access to their contents. Though at the moment Stacked Borrows is experimental, the aforementioned properties are already relied upon by rustc, because it emits the LLVM noalias attribute for these types.

The only sound way to get write access beyond the end of a Vec is to offset a pointer from the Vec, and access the data either through a raw pointer or as a reference to MaybeUninit<T> because slices require that their contents must be properly initialized. Additionally, this access cannot be done through get_unchecked_mut because that method comes from the Deref impl on Vec, so it is a precondition violation to use that method to access between len and capacity.

I've tried to be somewhat surgical in what I'm changing here. Sometimes unsound constructs are used because they look simpler, so please do let me know if I should adapt the code to accomodate the valid implementation technique(s).

@stepancheg
Copy link
Owner

What you are saying makes sense, and thank you for detailed write up.

The issue is that I cannot reproduce it (probably I don't completely understand it).

I've set up CI job which runs cargo miri test -p protobuf --lib. Is it possible to modify this PR with repro test case?

Using remove_lifetime_mut to construct then hold on to both a Vec and a
mutable slice into it violates the existing Rust aliasing rules,
because both &mut and Unique<T> (which is used to implement Vec) assert
unique access to their contents. This problem is flagged by Miri running
cargo miri test. Though at the moment Stacked Borrows is experimental,
the aforementioned properties are already relied upon by rustc.

Getting access to the uninitialized region of a Vec as a &mut[T] is not
permitted by passing invalid indexes to get_unchecked_mut, or by any other
mechanism because slice::from_raw_parts documents that it requires
properly initialized values of T. Though any bit pattern is a valid u8,
and we never do a read, it is still invalid to construct the slice.
@saethlin saethlin force-pushed the fix-output-stream-ub branch from 675da0e to 28426bd Compare January 31, 2022 05:54
@stepancheg
Copy link
Owner

I clicked approve CI, github seems to not run it by default now for first-time contributors.

Also, AFAIU, if can trigger the same actions in your repository.

@saethlin saethlin force-pushed the fix-output-stream-ub branch from 28426bd to 13ef41a Compare January 31, 2022 05:58
@saethlin
Copy link
Contributor Author

saethlin commented Jan 31, 2022

Oh! Doing stuff on my own repo sounds good. I'm on my way to bed but if this latest push doesn't get thing rolling I'm sure I'll be in for a learning experience tomorrow.

One test which can demonstrate the above is buf_read_iter::test_bytes::read_exact_bytes_from_bytes. I don't know how to select the package/library that test is part of. But cargo miri test buf_read_iter::test_bytes::read_exact_bytes_from_bytes demonstrates the issue. I have basically no understanding of workspaces. And I know about this test, because I just do the naive thing and run cargo miri test, then manually whittle away tests which are problematic to run under miri.

If you're interested in running miri generally, you can use #[cfg(miri)] to swap in smaller values for big tests (because miri is slow) or just disable tests entirely which need things like filesystem access (doesn't work on Windows yet) or any number of things that aren't pure-Rust code. You shouldn't need to many adjustments to make it viable to just run all your tests with cargo miri test in CI.

@stepancheg
Copy link
Owner

If you're interested in running miri generally, you can use #[cfg(miri)] to swap in smaller values for big tests

Already do that!

@stepancheg
Copy link
Owner

cargo miri test -p protobuf --lib

does not select the test you mentioned, but:

cargo miri test -p protobuf --lib --features=with-bytes

does.

Is bug specific to bytes crate/feature?

@saethlin saethlin force-pushed the fix-output-stream-ub branch from 13ef41a to f739004 Compare February 1, 2022 00:39
@saethlin
Copy link
Contributor Author

saethlin commented Feb 1, 2022

Okay, I think I've sorted things out now.

I was wrong to point out that test case. All tests that use bytes will not pass under Miri, because of tokio-rs/bytes#522 (I also submitted a PR to bytes which fixes the issue, but they haven't gotten to it yet). So I've cfg'd out all the tests that use bytes under Miri.

As I mentioned in my original comment (then forgot, apparently) to detect these issues you need -Zmiri-tag-raw-pointers. By default, Miri uses an incomplete implementation of Stacked Borrows (which is the name of the pointer-tagging model), which is a real shame because pointers are finicky and also other problems such as creating references to uninint memory and indexing out of bounds with get_unchecked are accidentally detectd by Miri's Stacked Borrows checker, when -Zmiri-tag-raw-pointers is set.

So the invocation is

MIRIFLAGS=-Zmiri-tag-raw-pointers cargo miri test -p protobuf --lib

which I've amended the CI config to do. Without this PR, using that command you should see a failure in the test coded_input_stream::test::test_input_stream_limits.

stepancheg added a commit that referenced this pull request Feb 1, 2022
stepancheg pushed a commit that referenced this pull request Feb 1, 2022
@stepancheg
Copy link
Owner

OK, trying to unfold it.

First, I took CodedOutputStream part of the PR.

The issue with this PR is that .buffer() call in the PR is much more expensive with this PR: it does branches instead of computing offset.

And .buffer() need to be very fast, because it is called in virtually any operation, like write_raw_varint32.

So I tried to implement it preserving buffer, but making it raw pointer instead of borrowed pointer.

Can you please review #597?

stepancheg pushed a commit that referenced this pull request Feb 1, 2022
stepancheg pushed a commit that referenced this pull request Feb 3, 2022
stepancheg pushed a commit that referenced this pull request Feb 3, 2022
stepancheg added a commit that referenced this pull request Feb 3, 2022
Based on PR #592

Thanks to Ben Kimock for reproducing the issue.
@stepancheg
Copy link
Owner

OK, now I patched CodedInputStream: 7c70db2.

In your PR you zero filled the vec, which is not necessary: BufRead can be read into [MaybeUninit<u8>], and the latter can be obtained using vec_spare_capacity_mut.

Test pass now (except disable slow tests and tokio-bytes tests).

Keeping the issue open because this code need to be ported to stable branch.

Thank you for the the bugreport, prototype, review and knowledge!

@stepancheg
Copy link
Owner

Version 2.27.0 contains the fixes.

@stepancheg stepancheg closed this Feb 3, 2022
akhramov pushed a commit to cognitedata/rust-protobuf that referenced this pull request Jan 5, 2024
akhramov pushed a commit to cognitedata/rust-protobuf that referenced this pull request Jan 5, 2024
Based on PR stepancheg#592

Thanks to Ben Kimock for reproducing the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants