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

SimplifyArmIdentity can cause use-after-moves in MIR #72800

Closed
jonas-schievink opened this issue May 31, 2020 · 0 comments · Fixed by #107256
Closed

SimplifyArmIdentity can cause use-after-moves in MIR #72800

jonas-schievink opened this issue May 31, 2020 · 0 comments · Fixed by #107256
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jonas-schievink
Copy link
Contributor

Reduced example diff from <core::slice::Iter<'a, T> as Iterator>::find_map:

-// MIR for `slice::<impl at src/libcore/slice/mod.rs:3293:9: 3475:10>::find_map` before SimplifyArmIdentity
+// MIR for `slice::<impl at src/libcore/slice/mod.rs:3293:9: 3475:10>::find_map` after SimplifyArmIdentity
 
 fn slice::<impl at src/libcore/slice/mod.rs:3293:9: 3475:10>::find_map(_1: &mut slice::Iter<T>, _2: F) -> option::Option<B> {
     debug self => _1;                    // in scope 0 at src/libcore/slice/mod.rs:3420:31: 3420:40
     debug f => _2;                       // in scope 0 at src/libcore/slice/mod.rs:3420:42: 3420:47
     let mut _0: option::Option<B>;       // return place in scope 0 at src/libcore/slice/mod.rs:3420:55: 3420:64
     let _3: ();                          // in scope 0 at src/libcore/slice/mod.rs:3425:17: 3429:18
     let mut _4: ();                      // in scope 0 at src/libcore/slice/mod.rs:3420:13: 3431:14
     let mut _5: option::Option<&T>;      // in scope 0 at src/libcore/slice/mod.rs:3425:37: 3425:48
     let mut _6: &mut slice::Iter<T>;     // in scope 0 at src/libcore/slice/mod.rs:3425:37: 3425:41
     let mut _7: isize;                   // in scope 0 at src/libcore/slice/mod.rs:3425:27: 3425:34
     let _8: &T;                          // in scope 0 at src/libcore/slice/mod.rs:3425:32: 3425:33
     let mut _9: option::Option<B>;       // in scope 0 at src/libcore/slice/mod.rs:3426:38: 3426:42
     let mut _10: &mut F;                 // in scope 0 at src/libcore/slice/mod.rs:3426:38: 3426:39
     let mut _11: (&T,);                  // in scope 0 at src/libcore/slice/mod.rs:3426:38: 3426:42
     let mut _12: &T;                     // in scope 0 at src/libcore/slice/mod.rs:3426:40: 3426:41
     let mut _13: isize;                  // in scope 0 at src/libcore/slice/mod.rs:3426:28: 3426:35
     let mut _15: !;                      // in scope 0 at src/libcore/slice/mod.rs:3426:43: 3428:22
     let mut _16: B;                      // in scope 0 at src/libcore/slice/mod.rs:3427:37: 3427:38
     let mut _17: !;                      // in scope 0 at src/libcore/slice/mod.rs:3425:17: 3429:18
     let mut _18: isize;                  // in scope 0 at src/libcore/slice/mod.rs:3429:17: 3429:18
     let mut _19: isize;                  // in scope 0 at src/libcore/slice/mod.rs:3429:17: 3429:18
     scope 1 {
         debug x => _8;                   // in scope 1 at src/libcore/slice/mod.rs:3425:32: 3425:33
         let _14: B;                      // in scope 1 at src/libcore/slice/mod.rs:3426:33: 3426:34
         scope 2 {
             debug y => _14;              // in scope 2 at src/libcore/slice/mod.rs:3426:33: 3426:34
         }
     }
 
     bb0: {
         StorageLive(_3);                 // scope 0 at src/libcore/slice/mod.rs:3425:17: 3429:18
         goto -> bb1;                     // scope 0 at src/libcore/slice/mod.rs:3425:17: 3429:18
     }

     // <...>
 
     bb7: {
-        StorageLive(_14);                // scope 1 at src/libcore/slice/mod.rs:3426:33: 3426:34
-        _14 = move ((_9 as Some).0: B);  // scope 1 at src/libcore/slice/mod.rs:3426:33: 3426:34
-        StorageLive(_16);                // scope 2 at src/libcore/slice/mod.rs:3427:37: 3427:38
-        _16 = move _14;                  // scope 2 at src/libcore/slice/mod.rs:3427:37: 3427:38
-        ((_0 as Some).0: B) = move _16;  // scope 2 at src/libcore/slice/mod.rs:3427:32: 3427:39
-        discriminant(_0) = 1;            // scope 2 at src/libcore/slice/mod.rs:3427:32: 3427:39
-        StorageDead(_16);                // scope 2 at src/libcore/slice/mod.rs:3427:38: 3427:39
-        StorageDead(_14);                // scope 1 at src/libcore/slice/mod.rs:3428:21: 3428:22
+        _0 = move _9;                    // scope 2 at src/libcore/slice/mod.rs:3427:32: 3427:39
         _18 = discriminant(_9);          // scope 1 at src/libcore/slice/mod.rs:3429:17: 3429:18
         StorageDead(_9);                 // scope 1 at src/libcore/slice/mod.rs:3429:17: 3429:18
         StorageDead(_8);                 // scope 0 at src/libcore/slice/mod.rs:3429:17: 3429:18
         StorageDead(_5);                 // scope 0 at src/libcore/slice/mod.rs:3429:17: 3429:18
         StorageDead(_3);                 // scope 0 at src/libcore/slice/mod.rs:3429:17: 3429:18
         goto -> bb9;                     // scope 0 at src/libcore/slice/mod.rs:3431:13: 3431:14
     }
 
     bb9: {
         return;                          // scope 0 at src/libcore/slice/mod.rs:3431:14: 3431:14
     }
 }

Here, the move _9 makes _9 uninitialized (in MaybeInitializedLocals), but the next statement immediately uses it again via discriminant(_9).

I do not believe that this is currently a soundness issue, as no analyses other than the generator transform (which runs earlier) should rely on this data. It is a problem for #72632 though, since that runs after SimplifyArmIdentity. Granted, the example above is unlikely to be problematic even for that, but other cases might be.

@jonas-schievink jonas-schievink added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. A-mir-opt Area: MIR optimizations labels May 31, 2020
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2023
Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts

I had attempted to fix the first of these opts in rust-lang#94177 . However, despite that PR already being a full re-write, it still did not fix some of the core soundness issues. The optimizations that are attempted here are likely to be desirable, but I do not expect any of the currently written code to survive into a sound implementation. Deleting the code keeps us from having to maintain the passes in the meantime.

Closes rust-lang#77359 , closes rust-lang#72800 , closes rust-lang#78628

r? `@cjgillot`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 25, 2023
Delete `SimplifyArmIdentity` and `SimplifyBranchSame` mir opts

I had attempted to fix the first of these opts in rust-lang#94177 . However, despite that PR already being a full re-write, it still did not fix some of the core soundness issues. The optimizations that are attempted here are likely to be desirable, but I do not expect any of the currently written code to survive into a sound implementation. Deleting the code keeps us from having to maintain the passes in the meantime.

Closes rust-lang#77359 , closes rust-lang#72800 , closes rust-lang#78628

r? ``@cjgillot``
@bors bors closed this as completed in c2f46df Jan 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
1 participant