Fix MIR lowering evaluation order and soundness bug #65608

matthewjasper · 2019-10-19T20:08:11Z

Fixes a soundness issue with built-in index operations
Ensures correct evaluation order of assignment expressions where the RHS is a FRU or is a use of a local of reference type.
Removes an unnecessary symbol to string conversion

closes #65909
closes #65910

rust-highfive · 2019-10-19T20:08:14Z

r? @estebank

(rust_highfive has picked a reviewer for you, use r? to override)

src/librustc/mir/mod.rs

src/librustc_mir/borrow_check/conflict_errors.rs

src/librustc_mir/build/expr/as_place.rs

src/librustc_mir/build/expr/into.rs

Centril · 2019-10-20T20:49:44Z

src/librustc_mir/build/expr/as_place.rs

+            len: Operand::Move(len),
+            index: Operand::Copy(Place::from(index)),
+        };
+        // assert!(lt, "...")


Suggested change

// assert!(lt, "...")

// assert!(lt, "...") -> bb_success

matthewjasper · 2019-10-20T21:05:20Z

@bors try @rust-timer queue

rust-timer · 2019-10-20T21:05:22Z

Awaiting bors try build completion

bors · 2019-10-20T21:05:31Z

⌛ Trying commit 683d113 with merge 5c1eaf2...

Fix MIR lowering evaluation order and soundness bug * Fixes a soundness issue with built-in index operations * Ensures correct evaluation order of assignment expressions where the RHS is a FRU or is a use of a local of reference type. * Removes an unnecessary symbol to string conversion

bors · 2019-10-21T00:07:57Z

☀️ Try build successful - checks-azure
Build commit: 5c1eaf2 (5c1eaf255c65c812d8b1a117a7ace2e0af367218)

rust-timer · 2019-10-21T00:07:58Z

Queued 5c1eaf2 with parent 89e645a, future comparison URL.

rust-timer · 2019-10-21T04:11:18Z

Finished benchmarking try commit 5c1eaf2, comparison URL.

matthewjasper · 2019-10-21T19:27:07Z

Perf looks fine (almost no change). Does someone else want to review? cc @oli-obk @eddyb

eddyb · 2019-10-21T19:38:48Z

r? @pnkfelix cc @nikomatsakis (MIR building / soundness changes seem pretty important)

estebank · 2019-10-21T23:48:16Z

cc @rust-lang/compiler just to make sure everyone sees it.

src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs

pnkfelix · 2019-10-25T15:37:45Z

I've skimmed over the changes. I want another chance to go over them more carefully (hopefully Monday), especially 683d113 where it uses into() instead of as_rvalue for borrows and ~~... well, you say FRU, but its really for all struct-literal expressions, no?~~ FRU. (I was wrong, FRU is indeed special cased in the code.)

pnkfelix · 2019-10-25T15:40:04Z

(also, are there separate issues filed for the bugs being fixed here? It might be useful to have a separate place to track that...)

JohnCSimon · 2019-11-02T04:37:40Z

Ping from triage.
@pnkfelix Can you please review this PR?
cc: @matthewjasper @Centril

Thank you!

pnkfelix · 2019-11-04T02:10:17Z

src/librustc_mir/build/expr/as_place.rs

-                // the index changing later: Nothing will ever change this temporary.
-                // The "retagging" transformation (for Stacked Borrows) relies on this.
-                let idx = unpack!(block = this.as_temp(
+                this.lower_index_expression(


This commit contains a number of refactorings (such as the rewrite of the match expression above, and the out-of-lining of the expression into its own method here).

I like refactorings, and think these ones make sense, given the state of the code.

But I also think my review would have been much easier (and perhaps would have had a quicker turn around time) if the refactorings were isolated into their own commit that was tagged as a refactoring with no expected semantic effect).

It’s not something that I’m going to demand of this Pull Request. It’s more of a note for the future: I think that isolating refactorings into their own commit(s) is a good habit for PR authors in general.

Centril · 2019-11-08T23:22:44Z

@bors p=4

bors · 2019-11-09T04:42:57Z

⌛ Testing commit 4fc8287f9525a9f522e17e5735db57b30778e05c with merge 43bc89a5827b1333476df6a373aac987d3b8de6c...

rust-highfive · 2019-11-09T05:56:53Z

The job i686-mingw-2 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-11-09T04:44:23.0141699Z environment variables:
2019-11-09T04:44:23.1105441Z 
2019-11-09T04:44:23.1106152Z 
2019-11-09T04:44:23.1106333Z 
2019-11-09T04:44:23.1106630Z * Ensures correct evaluation order of assignment expressions where the RHS is a FRU or is a use of a local of reference type.
2019-11-09T04:44:23.1106859Z * Fixes a soundness issue with built-in index operations
2019-11-09T04:44:23.1107116Z * Removes an unnecessary symbol to string conversion
2019-11-09T04:44:23.1107810Z AGENT_DISABLELOGPLUGIN_TESTFILEPUBLISHERPLUGIN=true
2019-11-09T04:44:23.1108781Z AGENT_DISABLELOGPLUGIN_TESTRESULTLOGPLUGIN=true
2019-11-09T04:44:23.1111217Z AGENT_HOMEDIRECTORY=C:\agents\2.160.0
2019-11-09T04:44:23.1111426Z AGENT_ID=512
---
2019-11-09T05:53:36.3184078Z [RUSTC-TIMING] memoffset test:false 0.173
2019-11-09T05:53:39.0131461Z [RUSTC-TIMING] parking_lot_core test:false 2.659
2019-11-09T05:53:39.0316738Z    Compiling synstructure v0.12.1
2019-11-09T05:54:30.8192134Z 
2019-11-09T05:54:30.8192700Z This application has requested the Runtime to terminate it in an unusual way.
2019-11-09T05:54:30.8193298Z Please contact the application's support team for more information.
2019-11-09T05:54:32.5147218Z Assertion failed!
2019-11-09T05:54:32.5147489Z Program: D:\a\1\s\build\i686-pc-windows-gnu\stage1\bin\rustc.exe
2019-11-09T05:54:32.5147489Z Program: D:\a\1\s\build\i686-pc-windows-gnu\stage1\bin\rustc.exe
2019-11-09T05:54:32.5147634Z File: D:/a/1/s/src/llvm-project/llvm/include/llvm/CodeGen/MachineOperand.h, Line 527
2019-11-09T05:54:32.5147703Z 
2019-11-09T05:54:32.5148010Z Expression: isImm() && "Wrong MachineOperand accessor"
2019-11-09T05:54:32.5148240Z error: could not compile `syn`.
2019-11-09T05:54:32.5148344Z warning: build failed, waiting for other jobs to finish...
2019-11-09T05:54:35.6572595Z [RUSTC-TIMING] synstructure test:false 56.599
2019-11-09T05:54:35.6681995Z error: build failed
2019-11-09T05:54:35.6681995Z error: build failed
2019-11-09T05:54:35.6738092Z command did not execute successfully: "D:\\a\\1\\s\\build\\i686-pc-windows-gnu\\stage0\\bin\\cargo.exe" "build" "-Zconfig-profile" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "2" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "D:\\a\\1\\s\\src/rustc/Cargo.toml" "--message-format" "json-render-diagnostics"
2019-11-09T05:54:35.6738651Z expected success, got: exit code: 101
2019-11-09T05:54:35.7687414Z failed to run: D:\a\1\s\build\bootstrap\debug\bootstrap test src/test/ui src/test/compile-fail
2019-11-09T05:54:35.7687742Z Build completed unsuccessfully in 0:59:56
2019-11-09T05:54:35.8262584Z make: *** [Makefile:91: ci-mingw-subset-2] Error 1
2019-11-09T05:54:35.8855194Z   local time: Sat Nov  9 05:54:35 CUT 2019
2019-11-09T05:54:36.4998331Z   network time: Sat, 09 Nov 2019 05:54:36 GMT
2019-11-09T05:54:36.5014168Z == end clock drift check ==
2019-11-09T05:54:36.5876936Z 
2019-11-09T05:54:36.5876936Z 
2019-11-09T05:54:36.9151238Z ##[error]Bash exited with code '2'.
2019-11-09T05:54:36.9588668Z ##[section]Starting: Checkout
2019-11-09T05:54:37.0440331Z ==============================================================================
2019-11-09T05:54:37.0440466Z Task         : Get sources
2019-11-09T05:54:37.0440552Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

bors · 2019-11-09T05:57:08Z

💔 Test failed - checks-azure

matthewjasper · 2019-11-10T16:38:56Z

This appears to be a pre-existing bug. I've minimized the issue to the following this requires a rustc with llvm assertions enabled and I suspect all i686 toolchains are affected:

// Only breaks at opt-level=2 or higher.
#![crate_type="lib"]

pub fn check_overlap(ext: &i32) -> usize {
    let src_usize = &1 as *const _ as usize;
    let dst_usize = ext as *const _ as usize;
    let mut diff = src_usize.wrapping_sub(dst_usize);
    if src_usize > dst_usize {
        diff = diff.wrapping_neg();
    };

    diff
}

An expression like `x[1][{ x = y; 2}]` would perform the bounds check for the inner index operation before evaluating the outer index. This would allow out of bounds memory accesses.

This fixes some ordering problems around assignment expressions.

matthewjasper · 2019-11-11T22:16:12Z

I've pushed a work around and have opened #66315 for the CI failure.

[WIP] Llvm update Asking for a `@bors try`, includes #65608 to verify if it fixes the issue.

matthewjasper · 2019-11-12T16:02:25Z

@bors r=pnkfelix p=0 rollup=never

bors · 2019-11-12T16:02:27Z

📌 Commit 4bf0685 has been approved by pnkfelix

bors · 2019-11-12T18:03:00Z

⌛ Testing commit 4bf0685 with merge 4f03f4a...

Fix MIR lowering evaluation order and soundness bug * Fixes a soundness issue with built-in index operations * Ensures correct evaluation order of assignment expressions where the RHS is a FRU or is a use of a local of reference type. * Removes an unnecessary symbol to string conversion closes #65909 closes #65910

bors · 2019-11-12T21:12:28Z

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing 4f03f4a to master...

rust-highfive · 2019-11-12T21:13:49Z

📣 Toolstate changed by #65608!

Tested on commit 4f03f4a.
Direct link to PR: #65608

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

@Xanewok

Tested on commit rust-lang/rust@4f03f4a. Direct link to PR: <rust-lang/rust#65608> 🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive assigned estebank Oct 19, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2019

matthewjasper force-pushed the mir-eval-order branch from ce395ab to 232f060 Compare October 19, 2019 20:19

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the mir-eval-order branch from 232f060 to 0d3a777 Compare October 19, 2019 22:01

This comment has been minimized.

Sign in to view

matthewjasper force-pushed the mir-eval-order branch from 0d3a777 to 94b1389 Compare October 20, 2019 10:34

Centril reviewed Oct 20, 2019

View reviewed changes

matthewjasper force-pushed the mir-eval-order branch from 94b1389 to 683d113 Compare October 20, 2019 19:10

Centril reviewed Oct 20, 2019

View reviewed changes

rust-highfive assigned pnkfelix and unassigned estebank Oct 21, 2019

pnkfelix reviewed Oct 25, 2019

View reviewed changes

src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs Outdated Show resolved Hide resolved

pnkfelix reviewed Oct 25, 2019

View reviewed changes

src/test/ui/borrowck/slice-index-bounds-check-invalidation.rs Show resolved Hide resolved

matthewjasper force-pushed the mir-eval-order branch from 683d113 to 4c0cd34 Compare October 28, 2019 21:29

pnkfelix reviewed Nov 4, 2019

View reviewed changes

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2019

This comment has been minimized.

Sign in to view

matthewjasper added 3 commits November 11, 2019 21:47

Avoid a string comparison in MIR construction

2ff89c5

Fix soundness issue with index bounds checks

7320818

An expression like `x[1][{ x = y; 2}]` would perform the bounds check for the inner index operation before evaluating the outer index. This would allow out of bounds memory accesses.

Evaluate borrow and struct expressions in into

4bf0685

This fixes some ordering problems around assignment expressions.

matthewjasper force-pushed the mir-eval-order branch from 3d3b0f9 to 698b347 Compare November 11, 2019 22:07

mati865 mentioned this pull request Nov 11, 2019

Update LLVM submodule #66318

Merged

bors added a commit that referenced this pull request Nov 12, 2019

Auto merge of #66318 - mati865:llvm-update, r=<try>

d283c32

[WIP] Llvm update Asking for a `@bors try`, includes #65608 to verify if it fixes the issue.

matthewjasper force-pushed the mir-eval-order branch from 698b347 to 4bf0685 Compare November 12, 2019 12:31

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 Nov 12, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 12, 2019

bors merged commit 4bf0685 into rust-lang:master Nov 12, 2019

bors mentioned this pull request Nov 12, 2019

Add a raw "address of" operator #64588

Merged

matthewjasper deleted the mir-eval-order branch November 12, 2019 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix MIR lowering evaluation order and soundness bug #65608

Fix MIR lowering evaluation order and soundness bug #65608

matthewjasper commented Oct 19, 2019 •

edited

Loading

rust-highfive commented Oct 19, 2019

This comment has been minimized.

This comment has been minimized.

Centril Oct 20, 2019

matthewjasper commented Oct 20, 2019

rust-timer commented Oct 20, 2019

bors commented Oct 20, 2019

bors commented Oct 21, 2019

rust-timer commented Oct 21, 2019

rust-timer commented Oct 21, 2019

matthewjasper commented Oct 21, 2019

eddyb commented Oct 21, 2019

estebank commented Oct 21, 2019

pnkfelix commented Oct 25, 2019 •

edited

Loading

pnkfelix commented Oct 25, 2019

JohnCSimon commented Nov 2, 2019

pnkfelix Nov 4, 2019

Centril commented Nov 8, 2019

bors commented Nov 9, 2019

rust-highfive commented Nov 9, 2019

bors commented Nov 9, 2019

matthewjasper commented Nov 10, 2019

This comment has been minimized.

matthewjasper commented Nov 11, 2019

matthewjasper commented Nov 12, 2019

bors commented Nov 12, 2019

bors commented Nov 12, 2019

bors commented Nov 12, 2019

rust-highfive commented Nov 12, 2019

Fix MIR lowering evaluation order and soundness bug #65608

Fix MIR lowering evaluation order and soundness bug #65608

Conversation

matthewjasper commented Oct 19, 2019 • edited Loading

rust-highfive commented Oct 19, 2019

This comment has been minimized.

This comment has been minimized.

Centril Oct 20, 2019

Choose a reason for hiding this comment

matthewjasper commented Oct 20, 2019

rust-timer commented Oct 20, 2019

bors commented Oct 20, 2019

bors commented Oct 21, 2019

rust-timer commented Oct 21, 2019

rust-timer commented Oct 21, 2019

matthewjasper commented Oct 21, 2019

eddyb commented Oct 21, 2019

estebank commented Oct 21, 2019

pnkfelix commented Oct 25, 2019 • edited Loading

pnkfelix commented Oct 25, 2019

JohnCSimon commented Nov 2, 2019

pnkfelix Nov 4, 2019

Choose a reason for hiding this comment

Centril commented Nov 8, 2019

bors commented Nov 9, 2019

rust-highfive commented Nov 9, 2019

bors commented Nov 9, 2019

matthewjasper commented Nov 10, 2019

This comment has been minimized.

matthewjasper commented Nov 11, 2019

matthewjasper commented Nov 12, 2019

bors commented Nov 12, 2019

bors commented Nov 12, 2019

bors commented Nov 12, 2019

rust-highfive commented Nov 12, 2019

matthewjasper commented Oct 19, 2019 •

edited

Loading

pnkfelix commented Oct 25, 2019 •

edited

Loading