panic when an interpreter error gets unintentionally discarded #130885

RalfJung · 2024-09-26T16:15:52Z

One important invariant of Miri is that when an interpreter error is raised (in particular a UB error), those must not be discarded: it's not okay to just check foo().is_err() and then continue executing.

This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that if this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to mem::forget when an error is deliberately discarded.

Fixes rust-lang/miri#3855

rustbot · 2024-09-26T16:16:00Z

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-09-26T16:16:02Z

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

RalfJung · 2024-09-26T16:18:35Z

compiler/rustc_mir_transform/src/gvn.rs

The annoying thing is that all the MIR transforms need to use discard_interp_error() instead of ok() now, and if they forget that that's a lurking ICE.

An alternative approach would be to use TLS to track whether the current interpreter has raised an error, and then on the next interpreter step complain if we are still running. But that means more global state, and we'll have to handle the situation of multiple interpreters on the same thread (they should be well-nested, at least) -- so it's not beautiful either.

Could we configure whether InterpErrors are "explosive" in the InterpCx? I'd ideally like to see MIR opts just do InterpCx::new(...).ignore_interp_errors() or something when interfacing with const eval. Or would this still be violating some invariant you'd like to see enforced in MIR opts?

RalfJung · 2024-09-26T17:18:30Z

Ah damn, a clippy failure... those are always so painful to fix due to #78717 :/

compiler-errors

I like this change in theory, but I do fear that it introduces a new class of somewhat easy to introduce ICEs with much of a "guardrail" to avoid it...

compiler-errors · 2024-09-27T04:08:32Z

compiler/rustc_mir_transform/src/dataflow_const_prop.rs

@@ -573,7 +587,7 @@ impl<'a, 'tcx> Collector<'a, 'tcx> {
        map: &Map<'tcx>,
    ) -> Option<Const<'tcx>> {
        let ty = place.ty(self.local_decls, self.patch.tcx).ty;
-        let layout = ecx.layout_of(ty).ok()?;
+        let layout = ecx.layout_of(ty).discard_interp_error()?;


I wonder if this should just call into self.tcx.layout_of -- having a layout_of query return an InterpResult that needs to be defused seems odd, imo.

We could do that but then we have to pass the ParamEnv by hand. It doesn't mak the code shorter, overall.

There's a lot of code that has TyCtx + ParamEnv but sadly no commonly used type to pack the two leading to a lot of issues like this. :/

compiler-errors · 2024-09-27T04:14:40Z

compiler/rustc_mir_transform/src/gvn.rs

Could we configure whether InterpErrors are "explosive" in the InterpCx? I'd ideally like to see MIR opts just do InterpCx::new(...).ignore_interp_errors() or something when interfacing with const eval. Or would this still be violating some invariant you'd like to see enforced in MIR opts?

compiler-errors · 2024-09-27T04:15:26Z

compiler/rustc_mir_transform/src/jump_threading.rs

-                TrackElem::Field(idx) => self.ecx.project_field(op, idx.as_usize()).ok(),
-                TrackElem::Variant(idx) => self.ecx.project_downcast(op, idx).ok(),
+                TrackElem::Field(idx) => {
+                    self.ecx.project_field(op, idx.as_usize()).discard_interp_error()


Bikeshed: If we can't get rid of all these calls in rustc_mir_transform, perhaps we could at least find a shorter name than discard_interp_error.

The name must be somewhat scary though or else people might use it in const-eval / miri and not realize what they are doing (and reviewers could easily miss it)...

compiler-errors · 2024-09-27T04:18:20Z

compiler/rustc_mir_transform/src/jump_threading.rs

@@ -557,7 +571,9 @@ impl<'a, 'tcx> TOFinder<'a, 'tcx> {
                // `SetDiscriminant` may be a no-op if the assigned variant is the untagged variant
                // of a niche encoding. If we cannot ensure that we write to the discriminant, do
                // nothing.
-                let Ok(enum_layout) = self.ecx.layout_of(enum_ty) else { return };
+                let Some(enum_layout) = self.ecx.layout_of(enum_ty).discard_interp_error() else {


This seems really easy to mix up by writing let Ok(_), as noted above.

I'm not certain the best way to avoid this, except for going way overkill and implementing some custom struct (like not a Result but a struct wrapper that makes the Result inaccessible) that implements Try and forces you to deal with the error by either doing ? (via Try) or .discard_interp_error...

Yeah that's the only type-based alternative I can think of.

RalfJung · 2024-09-27T08:27:26Z

Could we configure whether InterpErrors are "explosive" in the InterpCx?

Unfortunately not, since the code that creates InterpError has no access to the InterpCx.

(Sorry for replying out-of-thread, github as usually has a terrible UI when a review adds a new comment to an existing thread and doesn't make it clear where to reply to a message like this.)

oli-obk · 2024-09-27T09:31:53Z

I think this is just going to be a constant source of ICEs. I can already hear the humming of Matthias's fuzzers.

I don't think TLS for a sanity assertion is a problem and would prefer that solution

RalfJung · 2024-09-27T10:27:20Z

I don't think TLS for a sanity assertion is a problem and would prefer that solution

What about the "newtyped Result" option discussed above?
To me that sounds more appealing than global state.

oli-obk · 2024-09-27T10:40:36Z

I like it most but assumed from your comments that it was not to your liking.

RalfJung · 2024-09-27T12:26:31Z

Global state is also not to my liking ;)

RalfJung · 2024-09-29T07:44:52Z

Argh I wish we had implicit Ok-wrapping, that would make this so much nicer...

RalfJung · 2024-09-29T09:56:24Z

Okay, I did that now. :D That made the PR... big...

The new InterpResult type still also has a drop guard to panic on drop. This is to guard against things like

let _ = some_op_that_can_cause_ub();

Note that we panic on drop even if the operation succeeded, so while this does risk ICEs, the ICE should occur always when some code is executed, not just sometimes, making it easier to find with tests. And this has indeed found one discarded UB issue in Miri.

RalfJung · 2024-09-29T09:57:33Z

src/tools/miri/src/shims/unix/linux/foreign_items.rs

@@ -135,7 +135,7 @@ pub trait EvalContextExt<'tcx>: crate::MiriInterpCxExt<'tcx> {
                        // The only supported flags are GRND_RANDOM and GRND_NONBLOCK,
                        // neither of which have any effect on our current PRNG.
                        // See <https://github.com/rust-lang/rust/pull/79196> for a discussion of argument sizes.
-                        let _flags = this.read_scalar(&args[3])?.to_i32();
+                        let _flags = this.read_scalar(&args[3])?.to_i32()?;


This is the discarded UB issue that was now found -- the final ? was missing.

rustbot · 2024-09-29T10:25:14Z

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

compiler/rustc_const_eval/src/const_eval/dummy_machine.rs

bors · 2024-09-30T06:33:12Z

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

compiler/rustc_const_eval/src/const_eval/eval_queries.rs

compiler/rustc_const_eval/src/interpret/eval_context.rs

compiler/rustc_const_eval/src/interpret/memory.rs

oli-obk · 2024-10-01T19:38:38Z

r=me with or without any more applications of your last change if you want them.

…he error

RalfJung · 2024-10-01T19:45:51Z

@bors r=oli-obk

bors · 2024-10-01T19:45:54Z

📌 Commit c4ce8c1 has been approved by oli-obk

It is now in the queue for this repository.

…li-obk panic when an interpreter error gets unintentionally discarded One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing. This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded. Fixes rust-lang/miri#3855

…kingjubilee Rollup of 3 pull requests Successful merges: - rust-lang#130885 (panic when an interpreter error gets unintentionally discarded) - rust-lang#131108 (Revert rust-lang#131060 "Drop conditionally applied cargo `-Zon-broken-pipe=kill` flags") - rust-lang#131121 (A couple of fixes for dataflow graphviz dumps) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130885 - RalfJung:interp-error-discard, r=oli-obk panic when an interpreter error gets unintentionally discarded One important invariant of Miri is that when an interpreter error is raised (*in particular* a UB error), those must not be discarded: it's not okay to just check `foo().is_err()` and then continue executing. This seems to catch new contributors by surprise fairly regularly, so this PR tries to make it so that *if* this ever happens, we get a panic rather than a silent missed UB bug. The interpreter error type now contains a "guard" that panics on drop, and that is explicitly passed to `mem::forget` when an error is deliberately discarded. Fixes rust-lang/miri#3855

bors · 2024-10-02T09:49:38Z

⌛ Testing commit c4ce8c1 with merge 2305aad...

rustbot assigned TaKO8Ki Sep 26, 2024

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 Sep 26, 2024

RalfJung mentioned this pull request Sep 26, 2024

Make sure interpreter errors are never discarded rust-lang/miri#3855

Closed

RalfJung commented Sep 26, 2024

View reviewed changes

RalfJung force-pushed the interp-error-discard branch from 120e9f9 to b742eda Compare September 26, 2024 16:20

This comment has been minimized.

Sign in to view

RalfJung force-pushed the interp-error-discard branch from b742eda to 4557061 Compare September 26, 2024 17:25

compiler-errors reviewed Sep 27, 2024

View reviewed changes

RalfJung force-pushed the interp-error-discard branch 2 times, most recently from f07f8d3 to afa5a66 Compare September 29, 2024 09:55

RalfJung commented Sep 29, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung force-pushed the interp-error-discard branch from afa5a66 to 59c4659 Compare September 29, 2024 10:25

This comment has been minimized.

Sign in to view

oli-obk reviewed Sep 29, 2024

View reviewed changes

compiler/rustc_const_eval/src/const_eval/dummy_machine.rs Show resolved Hide resolved

RalfJung force-pushed the interp-error-discard branch 2 times, most recently from f5c5e1f to fc32fe9 Compare September 29, 2024 21:08

panic when an interpreter error gets unintentionally discarded

4b8a5bd

RalfJung force-pushed the interp-error-discard branch from fc32fe9 to dc0281f Compare September 30, 2024 06:42

This comment has been minimized.

Sign in to view

oli-obk reviewed Oct 1, 2024

View reviewed changes

compiler/rustc_const_eval/src/const_eval/eval_queries.rs Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/eval_context.rs Show resolved Hide resolved

compiler/rustc_const_eval/src/interpret/memory.rs Outdated Show resolved Hide resolved

RalfJung force-pushed the interp-error-discard branch from dc0281f to 80c5d99 Compare October 1, 2024 19:06

RalfJung force-pushed the interp-error-discard branch from 80c5d99 to d46aa2a Compare October 1, 2024 19:43

make InterpResult a dedicated type to avoid accidentally discarding t…

c4ce8c1

…he error

RalfJung force-pushed the interp-error-discard branch from d46aa2a to c4ce8c1 Compare October 1, 2024 19:45

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 Oct 1, 2024

matthiaskrgr mentioned this pull request Oct 2, 2024

Rollup of 4 pull requests #131138

Closed

workingjubilee mentioned this pull request Oct 2, 2024

Rollup of 3 pull requests #131143

Merged

bors merged commit ea453bb into rust-lang:master Oct 2, 2024
6 checks passed

rustbot added this to the 1.83.0 milestone Oct 2, 2024

RalfJung deleted the interp-error-discard branch October 3, 2024 06:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

panic when an interpreter error gets unintentionally discarded #130885

panic when an interpreter error gets unintentionally discarded #130885

RalfJung commented Sep 26, 2024

rustbot commented Sep 26, 2024

rustbot commented Sep 26, 2024

RalfJung Sep 26, 2024

compiler-errors Sep 27, 2024

This comment has been minimized.

RalfJung commented Sep 26, 2024

compiler-errors left a comment

compiler-errors Sep 27, 2024

RalfJung Sep 27, 2024

compiler-errors Sep 27, 2024

compiler-errors Sep 27, 2024

RalfJung Sep 27, 2024

compiler-errors Sep 27, 2024

RalfJung Sep 27, 2024

RalfJung commented Sep 27, 2024 •

edited

Loading

oli-obk commented Sep 27, 2024

RalfJung commented Sep 27, 2024

oli-obk commented Sep 27, 2024

RalfJung commented Sep 27, 2024

RalfJung commented Sep 29, 2024

RalfJung commented Sep 29, 2024

RalfJung Sep 29, 2024

This comment has been minimized.

rustbot commented Sep 29, 2024

This comment has been minimized.

bors commented Sep 30, 2024

This comment has been minimized.

This comment has been minimized.

oli-obk commented Oct 1, 2024

RalfJung commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 2, 2024

panic when an interpreter error gets unintentionally discarded #130885

panic when an interpreter error gets unintentionally discarded #130885

Conversation

RalfJung commented Sep 26, 2024

rustbot commented Sep 26, 2024

rustbot commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

RalfJung commented Sep 26, 2024

compiler-errors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Sep 27, 2024 • edited Loading

oli-obk commented Sep 27, 2024

RalfJung commented Sep 27, 2024

oli-obk commented Sep 27, 2024

RalfJung commented Sep 27, 2024

RalfJung commented Sep 29, 2024

RalfJung commented Sep 29, 2024

Choose a reason for hiding this comment

This comment has been minimized.

rustbot commented Sep 29, 2024

This comment has been minimized.

bors commented Sep 30, 2024

This comment has been minimized.

This comment has been minimized.

oli-obk commented Oct 1, 2024

RalfJung commented Oct 1, 2024

bors commented Oct 1, 2024

bors commented Oct 2, 2024

RalfJung commented Sep 27, 2024 •

edited

Loading