LLVM loop optimization can make safe programs crash

[RalfJung]

The following snippet crashes when compiled in release mode on current stable, beta and nightly:

enum Null {}

fn foo() -> Null { loop { } }

fn create_null() -> Null {
    let n = foo();

    let mut i = 0;
    while i < 100 { i += 1; }
    return n;
}

fn use_null(n: Null) -> ! {
    match n { }
}


fn main() {
    use_null(create_null());
}

https://play.rust-lang.org/?gist=1f99432e4f2dccdf7d7e&version=stable

This is based on the following example of LLVM removing a loop that I was made aware of: https://github.com/simnalamburt/snippets/blob/12e73f45f3/rust/infinite.rs.
What seems to happen is that since C allows LLVM to remove endless loops that have no side-effect, we end up executing a match that has no arms.

[ranma42]

The LLVM IR of the optimised code is

; Function Attrs: noreturn nounwind readnone uwtable
define internal void @_ZN4main20h5ec738167109b800UaaE() unnamed_addr #0 {
entry-block:
  unreachable
}

This kind of optimisation breaks the main assumption that should normally hold on uninhabited types: it should be impossible to have a value of that type.
rust-lang/rfcs#1216 proposes to explicitly handle such types in Rust. It might be effective in ensuring that LLVM never has to handle them and in injecting the appropriate code to ensure divergence when needed (IIUIC this could be achieved with appropriate attributes or intrinsic calls).
This topic has also been recently discussed in the LLVM mailing list: http://lists.llvm.org/pipermail/llvm-dev/2015-July/088095.html

[alexcrichton]

triage: I-nominated

Seems bad! If LLVM doesn't have a way to say "yes, this loop really is infinite" though then we may just have to sit-and-wait for the upstream discussion to settle.

[ranma42]

A way to prevent infinite loops from being optimised away is to add unsafe {asm!("" :::: "volatile")} inside of them. This is similar to the llvm.noop.sideeffect intrinsic that has been proposed in the LLVM mailing list, but it might prevent some optimisations.
In order to avoid the performance loss and to still guarantee that diverging functions/loops are not optimised away, I believe that it should be sufficient to insert an empty non-optimisable loop (i.e. loop { unsafe { asm!("" :::: "volatile") } }) if uninhabited values are in scope.
If LLVM optimises the code which should diverge to the point that it does not diverge anymore, such loops will ensure that the control flow is still unable to proceed.
In "lucky" case in which LLVM is unable to optimise the diverging code, such loop will be removed by DCE.

[geofft]

Is this related to #18785? That one's about infinite recursion to be UB, but it sounds like the fundamental cause might be similar: LLVM doesn't consider not halting to be a side effect, so if a function has no side effects other than not halting, it's happy to optimize it away.

[arielb1]

@geofft

It's the same issue.

[RalfJung]

Yes, looks like it's the same. Further down that issue, they show how to get undef, from which I assume it's not hard to make a (seemingly safe) program crash.

[simnalamburt]

👍

[ranma42]

Crash, or, possibly even worse heartbleed https://play.rust-lang.org/?gist=15a325a795244192bdce&version=stable

[nikomatsakis]

So I've been wondering how long until somebody reports this. :) In my opinion, the best solution would of course be if we could tell LLVM not to be so aggressive about potentially infinite loops. Otherwise, the only thing I think we can do is to do a conservative analysis in Rust itself that determines whether:

1. the loop will terminate OR
2. the loop will have side-effects (I/O operations etc, I forget precisely how this is defined in C)

Either of this should be enough to avoid undefined behavior.

[nikomatsakis]

triage: P-medium

We'd like to see what LLVM will do before we invest a lot of effort on our side, and this seems relatively unlikely to cause problems in practice (though I have personally hit this while developing the compiler as well). There are no backwards incomatibility issues to be concerned about.

[dotdash]

Quoting from the LLVM mailing list discussion:

 The implementation may assume that any thread will eventually do one of the following:
   - terminate
   - make a call to a library I/O function
   - access or modify a volatile object, or
   - perform a synchronization operation or an atomic operation

 [Note: This is intended to allow compiler transformations such as removal of empty loops, even
  when termination cannot be proven. — end note ]

[ranma42]

@dotdash The excerpt you are quoting comes from the C++ specification; it is basically the answer to "how it [having side effects] is defined in C" (also confirmed by the standard committee: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1528.htm ).

Regarding what is the expected behaviour of the LLVM IR there is some confusion. https://llvm.org/bugs/show_bug.cgi?id=24078 shows that there seems to be no accurate & explicit specification of the semantics of infinite loops in LLVM IR. It aligns with the semantics of C++, most likely for historical reasons and for convenience (I only managed to track down https://groups.google.com/forum/#!topic/llvm-dev/j2vlIECKkdE which apparently refers to a time when infinite loops were not optimised away, some time before the C/C++ specs were updated to allow it).

From the thread it is clear that there is the desire to optimise C++ code as effectively as possible (i.e. also taking into account the opportunity to remove infinite loops), but in the same thread several developers (including some that actively contribute to LLVM) have shown interest in the ability to preserve infinite loops, as they are needed for other languages.

[dotdash]

@ranma42 I'm aware of that, I just quoted that for reference, because one possibility to work-around this would be to detect such loops in rust and add one of the above to it to stop LLVM from performing this optimization.

[bstrie]

Is this a soundness issue? If so, we should tag it as such.

[bluss]

Yes, following @ranma42's example, this way shows how it readily defeats array bounds checks. playground link

[arielb1]

@bluss

The policy is that wrong-code issues that are also soundness issues (i.e. most of them) should be tagged I-wrong.

[brson]

I don't see it mentioned here: LLVM now has a "mustprogress" attribute:

• added here: https://reviews.llvm.org/D85393
• described in the manual: https://www.llvm.org/docs/LangRef.html

It appears to implement the proposal from way back in 2015, wherein C/C++ must annotate their functions "mustprogress" to enable these types of optimizations. The way it reads to me is that Rust should stop miscompiling these situations in the future just by not using this attribute.

Edit: oh I see @RalfJung linked this previously in some collapsed comments.

[shepmaster]

After #77972, this is the smallest reproduction of the original case I could make. Fails in Rust 1.49.0 and 1.51.0-nightly (2021-01-17 4253153):

pub fn oops() {
    (|| loop {
        drop(42)
    })()
}

[nikic]

This should be mostly fixed in LLVM 12 by https://reviews.llvm.org/D94106. There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

[oxalica]

The upstream issue is marked as fixed now. https://bugs.llvm.org/show_bug.cgi?id=965

[bstrie]

There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

Is there an LLVM bug to track this issue? (I presume it's not something rustc can address on its own?)

Also, if this issue were fully addressed, would we want to revert #77972? If so, that would be worth tracking somewhere. Even though it appears to cause no noticeable regressions, not doing unnecessary work seems valuable.

[nikic]

There's still a minor issue remaining (side-effects can be hoisted across infinite loops).

Is there an LLVM bug to track this issue? (I presume it's not something rustc can address on its own?)

This issue is fixed by https://reviews.llvm.org/D95288. There might be more places with problematic assumptions, but at least I'm not aware of any. (In default-enabled passes that is. The attributor has known issues.)

Also, if this issue were fully addressed, would we want to revert #77972? If so, that would be worth tracking somewhere. Even though it appears to cause no noticeable regressions, not doing unnecessary work seems valuable.

We shouldn't revert that yet as Rust also supports older LLVM versions, but we can limit sideeffect insertion to older versions at least.

[oberien]

I just found the following miscompilation resulting from this bug, which simply compiles to ud2 on stable, beta and nightly:

pub fn oops() -> u32 {
    (0..).sum() // or .last().unwrap() instead of .sum()
}

It's a minified version of the following snippet where I mistyped take_while as filter:

pub fn foo(end: u32) -> u32 {
    (0..)
        .map(|i| i * i)
        .filter(|&i| i < end)
        .sum()
}

[Necior]

I just want to notice that it's a bug on which beginner Rustaceans may stumble upon when working on the final exercise from The Book (Building a Multithreaded Web Server).

When I was playing with the web server's code, for a moment I had an empty loop body:

impl Worker {
    fn new(id: usize, receiver: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
        let thread = thread::spawn(|| loop {});
        Worker { id, thread }
    }
}

instead of (Listing 20-20):

impl Worker {
    fn new(id: usize, receiver: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
        let thread = thread::spawn(move || loop {
            let job = receiver.lock().unwrap().recv().unwrap();
            println!("Worker {} got a job; executing.", id);
            job();
        });
        Worker { id, thread }
    }
}

Running the first version ends with Illegal instruction (core dumped). This example boils down to #46076 which was closed as a duplicate of #28728 (this issue). It's also similar to #28728 (comment).

I keep my fingers crossed for solving this issue 🤞

[nagisa]

Closed by #81451

[daira]

Thankyou to everyone who helped to get this fixed.

However, I would like to note that it took over five years from the original report, and four years after LLVM devs said they would fix it on their side (#28728 (comment)), for this memory-unsafe soundness bug to actually be fixed in Rust.

Is this the expected length of time for fixes to memory-unsafe soundness bugs? :-/

It could, of course, have been fixed much earlier by just adding a side-effect into infinite loops [edit: I know that this is a strong statement but I have seen nothing that contradicts it], and then removing that workaround as a performance optimization after LLVM fixed their bug.

It doesn't appear to be the only example: #52652 (which also could be triggered from safe code as noted in #52652 (comment)) took over three years to be fixed, measuring from the original attempted fix in #46833.

Based on the evidence of this bug and #52652, there appears to be a de facto policy to leave tricky unsoundness bugs unfixed for long periods, whenever fixing them in the most obvious way could cause any potential performance regression or break any (no matter how obscure) code. I would like to register my objection to that de facto policy, which I do not think well serves the majority of Rust users.

[SimonSapin]

@daira While I think you make good points, closed issues are generally considered resolved and not in need of attention, so new comments there are easy to miss. Consider starting a new discussion elsewhere (though to be honest I’m not sure which venue would be best in this case.)

[RalfJung]

https://internals.rust-lang.org/ might be a reasonable place.

(FWIW, I disagree with your assessment @daira, in particular about "just" adding a side-effect into infinite loops -- it's not that simple; as usual, one has to be careful with "why don't you just"-style questions.)

[daira]

I used "just" advisedly. It really wouldn't have been hard, as compiler bugs go, to use that approach. Specifically, my understanding is that adding @llvm.sideeffect as originally proposed in #18785 would have been sufficient. No compiler bug of this kind is trivial, but they should not take 5 years to fix. (Correction: over 6 years, counting from #18785; that bug wasn't specific to loops but it was essentially the same problem.)

[Aaron1011]

It doesn't appear to be the only example: #52652

That bug was 'fixed' very quickly, but the fix had to be reverted because it broke the rlua crate. The full fix required an RFC to add new -unwind ABIs to the language. While it's definitely a good idea to prioritize fixing soundness issues, it's not always a matter of making an invisible change to the compiler backend.