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

regression tests for pointer invalidation in core library slice methods #1816

Merged
merged 3 commits into from
Jun 3, 2021

Conversation

Pointerbender
Copy link
Contributor

A fix for a pointer invalidation bug in <[T]>::copy_within has landed on the Rust master branch. This PR updates the rust-version file to the latest master commit hash and adds extra tests to the Miri test suite to ensure that regressions of this type of bug can be detected for various slice methods with the -Zmiri-track-raw-pointers flag.

I took the liberty of adding 2 extra #![feature] attributes at the top of slices.rs, since there already was one unstable feature. I hope this is okay 😄

One thing I noticed when running the entire Miri test suite with MIRIFLAGS="-Zmiri-track-raw-pointers" ./miri test is that there are currently failing tests on the master branch:

failures:
    [ui] run-pass/align.rs
    [ui] run-pass/box.rs
    [ui] run-pass/concurrency/simple.rs
    [ui] run-pass/libc.rs
    [ui] run-pass/ptr_int_casts.rs
    [ui] run-pass/stacked-borrows/int-to-ptr.rs

test result: FAILED. 199 passed; 6 failed; 1 ignored; 0 measured; 0 filtered out; finished in 12.95s

These failures were not fixed in this PR and already existed prior to this PR. I haven't investigated these yet, but am interested in helping out if possible!

Thanks!

@RalfJung
Copy link
Member

RalfJung commented May 27, 2021

One thing I noticed when running the entire Miri test suite with MIRIFLAGS="-Zmiri-track-raw-pointers" ./miri test is that there are currently failing tests on the master branch:

Yeah, it is expected that some tests fail. In particular, those that cast integers to pointers cannot work with this flag. That will explain some of the test failures, though possibly not all of them.

but am interested in helping out if possible!

Sure, if you want to audit these and make sure they are all due to int-to-ptr casts, that would be great. :)
Let's do that separately from this PR though.

@Pointerbender
Copy link
Contributor Author

Thanks for the PR comments @RalfJung! I will address these.

Sure, if you want to audit these and make sure they are all due to int-to-ptr casts, that would be great. :)
Let's do that separately from this PR though.

Sounds good! In case the failures are all due to int-to-ptr casts, would it be helpful if I can rewrite the test cases without casting pointers to integers, while keeping the same functionality?

Thanks!

p.s. I'm going on a short trip tomorrow, so it may take me until after the weekend to get back with more updates.

@RalfJung
Copy link
Member

Sounds good! In case the failures are all due to int-to-ptr casts, would it be helpful if I can rewrite the test cases without casting pointers to integers, while keeping the same functionality?

I think (almost) all of these tests are explicitly about testing int-to-ptr casts, so changing them would defeat their purpose. We do want to continue supporting int-to-ptr casts, and there is a reason that -Zmiri-track-raw-pointers is not the default.

p.s. I'm going on a short trip tomorrow, so it may take me until after the weekend to get back with more updates.

Sure, no worries.

@bors
Copy link
Contributor

bors commented May 28, 2021

☔ The latest upstream changes (presumably #1812) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 31, 2021
@Pointerbender
Copy link
Contributor Author

Hi @RalfJung, thank you for your patience while I was away! I have just pushed a new version which resolves the PR comments. I also rebased with the master branch upstream, due to a merge conflict in the rust-version file. I think everything should be good to go, but do let know if you encounter anything else 😃 Thanks!

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2021

Looking great, thanks a lot @Pointerbender :)
@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2021

📌 Commit e21dae7 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jun 3, 2021

⌛ Testing commit e21dae7 with merge ef99830...

@bors
Copy link
Contributor

bors commented Jun 3, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing ef99830 to master...

@bors bors merged commit ef99830 into rust-lang:master Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Waiting for the PR author to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants