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

CTFE: throw unsupported error when partially overwriting a pointer #87248

Merged
merged 3 commits into from
Aug 2, 2021

Conversation

RalfJung
Copy link
Member

Currently, during CTFE, when a write to memory would overwrite parts of a pointer, we make the remaining parts of that pointer "uninitialized". This is probably not what users expect, so if this ever happens they will be quite confused about why some of the data just vanishes for seemingly no good reason.
So I propose we change this to abort CTFE when that happens, to at last avoid silently doing the wrong thing.
Cc #87184

Our CTFE test suite still seems to pass. However, we should probably crater this, and I want to do some tests with Miri as well.

@rust-highfive
Copy link
Collaborator

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 18, 2021
@RalfJung
Copy link
Member Author

r? @oli-obk

@RalfJung RalfJung force-pushed the ctfe-partial-overwrite branch from 3b4e57e to 0624ccb Compare July 18, 2021 10:04
@RalfJung
Copy link
Member Author

Ah turns out Miri really needs this:

error: unsupported operation: unable to overwrite parts of a pointer in memory at alloc1390
   --> /home/r/src/rust/rustc/library/std/src/panicking.rs:401:13
    |
401 |             data.r = ManuallyDrop::new(f());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ unable to overwrite parts of a pointer in memory at alloc1390
    |
    = help: this is likely not a bug in the program; it indicates that the program performed an operation that the interpreter does not support

    = note: inside `std::panicking::try::do_call::<&dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe, i32>` at /home/r/src/rust/rustc/library/std/src/panicking.rs:401:13
    = note: inside `std::panicking::try::<i32, &dyn std::ops::Fn() -> i32 + std::marker::Sync + std::panic::RefUnwindSafe>` at /home/r/src/rust/rustc/library/std/src/panicking.rs:365:19

@RalfJung RalfJung force-pushed the ctfe-partial-overwrite branch from 0624ccb to 4725c85 Compare July 18, 2021 10:41
@rust-log-analyzer

This comment has been minimized.

@RalfJung
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Jul 18, 2021

⌛ Trying commit 4725c8543fc218bcafef8c60a9f397019bc5e1aa with merge 3e59a8c2f14152ccc92a6c079f592dc2dc6fa93f...

@bors
Copy link
Contributor

bors commented Jul 18, 2021

☀️ Try build successful - checks-actions
Build commit: 3e59a8c2f14152ccc92a6c079f592dc2dc6fa93f (3e59a8c2f14152ccc92a6c079f592dc2dc6fa93f)

@RalfJung
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-87248 created and queued.
🤖 Automatically detected try build 3e59a8c2f14152ccc92a6c079f592dc2dc6fa93f
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 19, 2021
@craterbot
Copy link
Collaborator

🚧 Experiment pr-87248 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-87248 is completed!
📊 10 regressed and 6 fixed (173576 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Jul 30, 2021
@oli-obk
Copy link
Contributor

oli-obk commented Jul 30, 2021

All of the regressions are spurious (some python network error and some file not found)

@RalfJung
Copy link
Member Author

RalfJung commented Jul 30, 2021

Nice, that's what I hoped for. :)

So how do we proceed here -- what are your thoughts on this PR, @oli-obk?
(I sure need to add a test before landing.)

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

I like how this allows us to experiment on miri first!

lgtm modulo some tests

compiler/rustc_middle/src/mir/interpret/error.rs Outdated Show resolved Hide resolved
Co-authored-by: Oli Scherer <[email protected]>
@RalfJung
Copy link
Member Author

Added a test, please have a look. :)

@oli-obk
Copy link
Contributor

oli-obk commented Aug 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 2, 2021

📌 Commit 2a9b44d has been approved by oli-obk

@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 Aug 2, 2021
@bors
Copy link
Contributor

bors commented Aug 2, 2021

⌛ Testing commit 2a9b44d with merge 3227e35...

@bors
Copy link
Contributor

bors commented Aug 2, 2021

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 3227e35 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 2, 2021
@bors bors merged commit 3227e35 into rust-lang:master Aug 2, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 2, 2021
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #87248!

Tested on commit 3227e35.
Direct link to PR: #87248

💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Aug 2, 2021
Tested on commit rust-lang/rust@3227e35.
Direct link to PR: <rust-lang/rust#87248>

💔 miri on windows: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
💔 miri on linux: test-pass → build-fail (cc @eddyb @RalfJung @oli-obk).
@RalfJung RalfJung deleted the ctfe-partial-overwrite branch August 2, 2021 15:59
bors added a commit to rust-lang/miri that referenced this pull request Aug 2, 2021
adjust for ERR_ON_PARTIAL_PTR_OVERWRITE

The Miri side of rust-lang/rust#87248
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants