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

[EXPERIMENT] Record an allowed byte range in pointer provenance for miri #105264

Closed
wants to merge 6 commits into from

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Dec 4, 2022

This is an experiment to have miri disallow accessing one struct field using a pointer to another.
See the test src/tools/miri/tests/fail/field-access.rs

The idea is to annotate each PlaceElem::Field with a ProjectionMode. ProjectionMode has 2 possible values:

  • ProjectionMode::Weak: the projected place has the same provenance;
  • ProjectionMode::Strong: the projected place cannot be used to access memory outside of the field.

The provenance for miri is completed to have the byte range of allowed access.
(There is probably a better way to encode this than directly in Provenance::Concrete.)

The main knob is compiler/rustc_mir_build/src/build/expr/as_place.rs, to chose when we put a Strong/Weak mode.
This draft puts as much Strong as for the demonstration.

cc @RalfJung

@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2022

r? @compiler-errors

(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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 4, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 344889e963742e87181d3c023e2a1ea7d95f9468 and e7214b520e4954abadd34d6e8278a584afdcb653
Tool subtrees were updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (90/91)
.          (91/91)


/checkout/src/test/rustdoc-gui/basic-code.goml basic-code... FAILED
[ERROR] (line 3) Error: Execution context was destroyed, most likely because of a navigation.: for command `assert-count: (".src-line-numbers", 1)`
Build completed unsuccessfully in 0:02:26

@RalfJung
Copy link
Member

RalfJung commented Dec 5, 2022

May I ask what the goal is of this experiment?

Note that Stacked Borrows already has subobject provenance, and does not put a Range into provenance like this. So there is a bunch of redundancy here. Also this is probably pretty bad for performance since it makes the Pointer type a lot bigger. Furthermore, we almost certainly don't want anything like this for CTFE. So probably this should not touch rustc_const_eval at all, and only work in Miri. (EDIT: Ah the rustc_const_eval changes are solely for that new projection mode, the CTFE model of provenance remains unchanged in fact.)

So -- I appreciate the desire to perform experiment, but I think we need to discuss a bit more whether we even want something like this before considering landing the patch and doing a proper review for that. Right now I am inclined against that. Subobject provenance should be part of the aliasing model, not a separate component that risks causing surprising interactions. Stacked Borrows already has subobject provenance so this shouldn't even change anything for Miri. And the new aliasing model we are working on (Tree Borrows) deliberately does not have subobject provenance. It would not be part to add subobject provenance to Tree Borrows, and it would not look anything like this patch. Miri will support both Stacked Borrows and Tree Borrows in parallel for some time, while T-opsem figures out which aspects of which model they want to see adopted into the final model for Rust.


let bad_y = unsafe { *xpp1 }; //~ ERROR: only permits access to offsets 0..1
println!("{}", bad_y);
}
Copy link
Member

@RalfJung RalfJung Dec 5, 2022

Choose a reason for hiding this comment

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

This is all raw pointer code. There should not be any UB here, in my opinion. Our stanza generally always was -- if you use the correct offsets, you get to do what you want with the fields of a struct. And if code uses offset_of, that is guaranteed to work even with repr(Rust). (This code happens to not use offset_of, but with your patch the testcase would fail even if it did use offset_of.)

@bors
Copy link
Contributor

bors commented Dec 6, 2022

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

@compiler-errors compiler-errors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 16, 2022
@cjgillot cjgillot closed this Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants