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

Don't do pointer arithmetic on pointers to deallocated memory #106950

Merged
merged 2 commits into from
Jan 18, 2023

Conversation

the8472
Copy link
Member

@the8472 the8472 commented Jan 16, 2023

vec::Splice can invalidate the slice::Iter inside vec::Drain. So we replace them with dangling pointers which, unlike ones to deallocated memory, are allowed.

Fixes miri test failures.
Fixes rust-lang/miri#2759

@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 16, 2023

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@Noratrieb
Copy link
Member

@RalfJung
Copy link
Member

Thanks! Would be good to add a regression test in src/tools/miri/tests/pass/vec.rs.

fn miri_issue_2759() {
    let mut input = "1".to_string();
    input.replace_range(0..0, "0");
}

@rustbot
Copy link
Collaborator

rustbot commented Jan 17, 2023

The Miri subtree was changed

cc @rust-lang/miri

@RalfJung
Copy link
Member

LGTM, but we should probably get a T-libs review.
Cc @thomcc

// At this point draining is done and the only remaining tasks are splicing
// and moving things into the final place.
// Which means we can replace the slice::Iter with pointers that won't point to deallocated
// memory. This makes Miri happy.
Copy link
Member

Choose a reason for hiding this comment

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

It's not just about pleasing Miri, right?

Suggested change
// memory. This makes Miri happy.
// memory, and `Drain::drop` can still see that it's empty. This also makes Miri happy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, logically it already was 0-length before, just the way it calculated the way was Miri-UB but not LLVM-UB because we're not emitting any hints that llvm could exploit. But that rephrase is not quite right either, it's about Drain being allowed to calculate the length at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated it.

vec::Splice can invalidate the slice::Iter inside vec::Drain.
So we replace them with dangling pointers which, unlike ones to
deallocated memory, are allowed.
@cuviper
Copy link
Member

cuviper commented Jan 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit 2d54b7c has been approved by cuviper

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 17, 2023
compiler-errors added a commit to compiler-errors/rust that referenced this pull request Jan 18, 2023
Don't do pointer arithmetic on pointers to deallocated memory

vec::Splice can invalidate the slice::Iter inside vec::Drain. So we replace them with dangling pointers which, unlike ones to deallocated memory, are allowed.

Fixes miri test failures.
Fixes rust-lang/miri#2759
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 18, 2023
Rollup of 5 pull requests

Successful merges:

 - rust-lang#103702 (Lift `T: Sized` bounds from some `strict_provenance` pointer methods)
 - rust-lang#106441 (relax reference requirement on SocketAddrExt::from_abstract_name)
 - rust-lang#106718 (finish trait solver skeleton work)
 - rust-lang#106950 (Don't do pointer arithmetic on pointers to deallocated memory)
 - rust-lang#107014 (rustdoc: remove deprecated / unused code from main.js)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 1ff4a12 into rust-lang:master Jan 18, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pointer dereferenced after allocation was freed in String::replace_range
6 participants