Panic in nightly 1.83.0 and 1.84.0 with opt-level >= 1 when unwrapping Some variant

[Specy]

When calling an unwrap on a value that should be Some, i instead get an unwrap on None error. Attaching a debugger seems to show an invalid memory error.

This issue happens only when opt-level is set to at least 1, (aka in dev profile, no panic happens, and in release it does), and only happens in rust nightly 1.83.0 and 1.84.0, it does not happen on stable and nightly 1.82.0.

I'm not sure if the issue is in my code or in the sprs crate, i've filed an issue there and also here just to make sure.
EDIT: managed to narrow down the bug by removing the sprs crate, this is only pure rust

Here is the repo for minimum example or in the playground, there is an even smaller reproduction in the comments

To reproduce, run cargo run --release which will panic, while if running cargo run it won't panic

I've tried to run this with miri, nothing there. Also tried to run the release mode with bounds checking turned on, but nothing changed

Meta

rustc --version --verbose:

rustc 1.84.0-nightly (3f1be1ec7 2024-10-28)
binary: rustc
commit-hash: 3f1be1ec7ec3d8e80beb381ee82164a0aa3ca777
commit-date: 2024-10-28
host: x86_64-unknown-linux-gnu
release: 1.84.0-nightly
LLVM version: 19.1.1

Backtrace

thread 'main' panicked at src/main.rs:122:27:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0: rust_begin_unwind
             at /rustc/3f1be1ec7ec3d8e80beb381ee82164a0aa3ca777/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/3f1be1ec7ec3d8e80beb381ee82164a0aa3ca777/library/core/src/panicking.rs:75:14
   2: core::panicking::panic
             at /rustc/3f1be1ec7ec3d8e80beb381ee82164a0aa3ca777/library/core/src/panicking.rs:152:5
   3: core::option::unwrap_failed
             at /rustc/3f1be1ec7ec3d8e80beb381ee82164a0aa3ca777/library/core/src/option.rs:2008:5
   4: microlp::order_simple
   5: microlp::main
             at ./src/main.rs:152:5
   6: core::ops::function::FnOnce::call_once
             at /home/specy/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5

[tmiasko]

cargo-bisect-rustc suggest that regression is in e7386b3 (cc @DianQK)

[theemathas]

Minimized:

fn pop_min(mut score2head: Vec<Option<usize>>) -> Option<usize> {
    loop {
        if let Some(col) = score2head[0] {
            score2head[0] = None;
            return Some(col);
        }
    }
}

fn main() {
    let min = pop_min(vec![Some(1)]);
    println!("min: {:?}", min);
    // panic happens here on beta in release mode
    min.unwrap();
}

[jieyouxu]

Working on a revert for e7386b3, we can reland that mir-opt with this fixed.

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-critical

[jieyouxu]

(The revert will probably need to be beta-backported, but I'll nominate that in the PR)

[DianQK]

Working on a revert for e7386b3, we can reland that mir-opt with this fixed.

If you want to revert it, can you use unsound_mir_opts instead? Just like #124156. This can make subsequent reviews easier.

[jieyouxu]

Oh cool, yes I can try to use that.

[jieyouxu]

If you want to revert it, can you use unsound_mir_opts instead? Just like #124156. This can make subsequent reviews easier.

(BTW is this written down in the dev-guide or even forge somewhere, if we can make mir-opt reland reviews easier we definitely should)

[DianQK]

If you want to revert it, can you use unsound_mir_opts instead? Just like #124156. This can make subsequent reviews easier.

(BTW is this written down in the dev-guide or even forge somewhere, if we can make mir-opt reland reviews easier we definitely should)

Looks like makes sense, though I'm still not sure which chapter it should go in.

[jieyouxu]

I opened #132356 which marks the mir-opt as unsound (hence disabling it by default, but doesn't remove the implementation to make reland reviews easier). Test changes are fully reverted because I didn't feel like revisioning all of them with -Z unsound-mir-opt (or whatever that flag is called), although I could be convinced to do that if that is desirable.

EDIT: updated tests that are added by #132356 to use -Zunsound-mir-opts.

[DianQK]

I opened #132356 which marks the mir-opt as unsound (hence disabling it by default, but doesn't remove the implementation to make reland reviews easier). Test changes are fully reverted because I didn't feel like revisioning all of them with -Z unsound-mir-opt (or whatever that flag is called), although I could be convinced to do that if that is desirable.

Subsequent PRs won’t affect existing test cases, so I prefer -Zunsound-mir-opt.

[DianQK]

#132468 has been merged.

@rustbot label +P-medium -P-critical

[matthieu-m]

@DianQK While fixing this particular MIR optimization soundness is great, I was wondering if there's any more systemic initiative to try and catch such optimization issues, such as -- for example -- differential fuzzing between running miri on both unoptimized and optimized MIR, both of which should yield the same result.

[DianQK]

@DianQK While fixing this particular MIR optimization soundness is great, I was wondering if there's any more systemic initiative to try and catch such optimization issues, such as -- for example -- differential fuzzing between running miri on both unoptimized and optimized MIR, both of which should yield the same result.

@matthiaskrgr maybe have some insight on fuzzing.

[nikic]

I believe that @cbeuw did that kind of fuzzing, but I don't think its still running? And @shao-hua-li also does correctness fuzzing as well, but I don't think their methodology is MIR-oriented.

[matthiaskrgr]

differential fuzzing between running miri on both unoptimized and optimized MIR, both of which should yield the same result.

What exactly would be the result we are trying to find catch? differences in executables, program output?

I already implemented running kani on a file and looking if increase in mir opts increases the number of reachable panics.
Miri is also running with mir-opt-level 0 and mir-opt level 5, but mostly for finding crashes in miri, checking if the running executable behaves exhibits the same output should be trivial but it'll be very slow im afraid 😅

[matthieu-m]

differential fuzzing between running miri on both unoptimized and optimized MIR, both of which should yield the same result.

What exactly would be the result we are trying to find catch? differences in executables, program output?

I already implemented running kani on a file and looking if increase in mir opts increases the number of reachable panics. Miri is also running with mir-opt-level 0 and mir-opt level 5, but mostly for finding crashes in miri, checking if the running executable behaves exhibits the same output should be trivial but it'll be very slow im afraid 😅

First of all, what I am afraid of is that optimizations may accidentally introduce differences in behavior: panics, as observed here, is probably the mildest issue, changes in the results of calculations or introduction of UB would be much more concerning.

I do think optimizations on MIR are good -- they apply pre-monomorphization, for one -- but code generation bugs are a pain, and thus I am wondering if any safeguard could be put in place which would stand a good chance to catch the issue early on, before it reaches users. In particular, I note that while this issue was detected on nightly, it had already "escaped" to beta, and would have been stabilized 2 weeks later.

So, I was thinking of the potential ways to catch such issues, systematically, and early on:

• Formal verification: like CompCert, the holy grail, is probably much too expensive and stiffling.
• Symbolic verification: like Cranelift (on register allocation), in which the input & output MIRs are proven equivalent, would be fairly awesome. It's very useful for users -- I'd happily spend 2x the compile time on Release artifacts, if it means they're proven safely optimized -- and it's very useful for fuzzing -- throw random programs at the optimizer, check the optimized one is equivalent to the non-optimized one. It's probably a longer-term project, I'd guess, though.
• Differential Fuzzing: like Alive2 (John Regher), compile programs in N different ways (optimization levels, here), run them, expect the same output. Of interest, it can be applied at different stages: on MIR, or on machine code (I don't know of an LLVM IR interpreter). It should be the easiest to spin up.

Thus, as a stop-gap measure, my suggestion of perhaps using Differential Fuzzing on the output of interpreting unoptimized and optimized MIR with miri, which could be extended to the output of running the generated machine code.

I don't have much experience with miri performance wise -- I'm happy to run cargo miri test on a subset of tests on my codebase, but that's "human-scale" performance -- so I can't say whether it'd be fast enough or not for fuzzing. Perhaps sticking to small enough programs would be enough: after all the reproducer above ended up fairly small.

Still, it may be worth the time, especially as unlike native binaries, miri does catch quite a few different kinds of UB, notably borrow-checking violations.

The biggest blocker, though, would be having a good corpus + fuzzer. Absent that, it's no longer easy to spin up...

[DianQK]

In particular, I note that while this issue was detected on nightly, it had already "escaped" to beta, and would have been stabilized 2 weeks later.

Although I also deeply dislike miscompile (especially those caused by me :p), I often see cases where some miscompile in LLVM hidden for years.

Alive2 is very useful, but we currently only run it on pre-commit prove and mostly test cases in LLVM.

IMO, we can start some fuzzer with different pipeline and compare those running results. I’ll consider this, although I don’t have any experience yet.

[matthiaskrgr]

I have seen that cranelift is sometimes more strict than rustc and it ICEs on code that exhibits unsoundess which rustc+llvm would just wave through and compile.

I already implemented something like "run miri, check if there is UB in the absence of unsafe" and I guess we could also just compare the program output of runs with and without mir optimisations. I'm afraid this will blow up though when we have any kind of randomness in the tested programs.

I also have a check that runs kani https://model-checking.github.io/kani/ and checks if mir opts would cause panics and it would have detected the problem above it seems 🎉 (although I very rarely run this tbh 🙈 )

/tmp/im/a.rs MIR OPT CAUSES PANICS: 0 to 1

SUMMARY:
 ** 1 of 124 failed (3 unreachable)
Failed Checks: called `Option::unwrap()` on a `None` value
 File: "/github/home/.rustup/toolchains/nightly-2024-10-03-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs", line 2004, in std::option::unwrap_failed

VERIFICATION:- FAILED
Verification Time: 0.1597398s

[matthiaskrgr]

about the corpus, the rust repo and all its history are around 25 gb of code already, seems like a good start :)

for i in `git rev-list --objects --all  | grep "\.rs"   | cut -d' ' -f1` ; do ; git cat-file -p $i > ${i}.rs ; done

[saethlin]

This is now a discussion of how to fuzz rustc, which is much better had elsewhere.

I believe that @cbeuw did that kind of fuzzing, but I don't think its still running?

His dissertation https://dl.acm.org/doi/pdf/10.1145/3689780 says it took 10 CPU-years of fuzzing to uncover 22 new bugs. That's low enough yield that this needs to run on a server somewhere, not in someone's dev environment (much less a shared dev environment).

Infra may be willing to help run it, but someone would need to do the work to maintain the codebase.

[RalfJung]

Sadly, for mir-opt bugs like this, the best defense we have is still to be really extra super careful when writing and reviewing them. Fuzzing and other tooling can catch some bugs, but not nearly enough.

Infra may be willing to help run it, but someone would need to do the work to maintain the codebase.

More importantly, someone needs to sift through the reports generated by the tool, and turn them into useful bug reports.