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

Fixes soundness bug 18510 by aborting on unwind from safe extern "C" functions only #64315

Open
wants to merge 3 commits into
base: master
from

Conversation

@gnzlbg
Copy link
Contributor

commented Sep 9, 2019

This fixes the soundness bug #18510 from November 2014 which allows safe Rust code to invoke undefined behavior:

extern "C" fn foo() { panic!() }
fn main() { foo() } // UB: a panic escapes an extern "C" function

by implementing the behavior defined in the reference for all extern "C" functions, which is to abort when a panic! tries to unwind out of them, for safe extern "C" functions only.

That is, unlike previous attempts at fixing this soundness hole, this PR does not make unsafe extern "C" functions abort, since that is not required for soundness, and has been shown to break a lot of code (see #52652 top comment for a brief history of the breakage).

That is, after this PR, it is still possible to invoke undefined behavior by unwinding out of an unsafe extern "C" function like here:

unsafe extern "C" fn foo() { panic!() }
fn main() { unsafe { foo() } } // UB: a panic escapes an unsafe extern "C" function

but doing so requires unsafe code, and is an implementation-bug, but not a soundness bug.

There is a PR open (#63909) that removes the nounwind attribute from unsafe extern "C" functions to try to prevent them from being optimized if they exhibit undefined behavior.

Note: this PR only closes #18510, which has been closed and and then re-opened as #52652. It was pointed out, that a lot of people do not interpret #52652 to be about a soundness bug, even though the top comment points to 18510, and the first couple of comments in the issue clarify that. It was also pointed out that people would prefer to re-purpose #52652 for discussing future language features instead.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

r? @petrochenkov

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

r? @RalfJung
(Not my area.)

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Still on vacation, sorry.
Cc #52652 #63909 rust-lang/rfcs#2753

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Closes #52652.

This seems rather inadequate. We still don't properly insert the shims for all functions then, we in particular we do not do anything about mozjpeg and rlua being UB.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I think I am of mixed mind about this change. It's a hack, but it does turn some UB into guaranteed abort which is nice, but it also doesn't really help to solve what I consider to be the main problem with the current unwind situation.

So, I am not necessarily opposed to landing this if there is general support. But I do think that we should definitely keep #52652 open (so the "closes" line should be removed from the PR description).

@gnzlbg assuming your goal here is to "agree on and fix what seems uncontroversial", there is also some other "low-hanging fruit" around unwind, I think? Such as #63883 and the fact that extern "Rust" FFI imports are marked nounwind. It might make sense to also try and fix those while the FFI story is still up in the air?

Also, this PR should come with a test (we already have a test for #[unwind(abort)], we should have the same test for a safe attribute-free extern "C" fn then).

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (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-09-09T16:49:34.2515106Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-09T16:49:34.2719726Z ##[command]git config gc.auto 0
2019-09-09T16:49:34.2795277Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-09T16:49:34.2855097Z ##[command]git config --get-all http.proxy
2019-09-09T16:49:34.3015612Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64315/merge:refs/remotes/pull/64315/merge
---
2019-09-09T17:52:54.5027823Z .................................................................................................... 1500/9003
2019-09-09T17:53:00.5974301Z .................................................................................................... 1600/9003
2019-09-09T17:53:13.9823871Z .....................................................i...............i.............................. 1700/9003
2019-09-09T17:53:22.3220824Z .................................................................................................... 1800/9003
2019-09-09T17:53:37.4936748Z ............................................iiiii................................................... 1900/9003
2019-09-09T17:53:49.0597588Z .................................................................................................... 2100/9003
2019-09-09T17:53:51.7544041Z .................................................................................................... 2200/9003
2019-09-09T17:53:55.8652033Z .................................................................................................... 2300/9003
2019-09-09T17:54:04.1199923Z .................................................................................................... 2400/9003
---
2019-09-09T17:57:10.8319400Z ...............................i...............i.................................................... 4700/9003
2019-09-09T17:57:23.1323423Z .................................................................................................... 4800/9003
2019-09-09T17:57:29.6578180Z .................................................................................................... 4900/9003
2019-09-09T17:57:40.9849553Z .................................................................................................... 5000/9003
2019-09-09T17:57:47.0396280Z .............ii.ii.................................................................................. 5100/9003
2019-09-09T17:57:58.1136921Z .................................................................................................... 5300/9003
2019-09-09T17:58:08.6500350Z ............................................................................i....................... 5400/9003
2019-09-09T17:58:16.6235277Z .................................................................................................... 5500/9003
2019-09-09T17:58:23.1208065Z .................................................................................................... 5600/9003
2019-09-09T17:58:23.1208065Z .................................................................................................... 5600/9003
2019-09-09T17:58:34.1581291Z ......................................................................ii...i..ii...........i........ 5700/9003
2019-09-09T17:59:00.4301843Z .................................................................................................... 5900/9003
2019-09-09T17:59:10.0587990Z .................................................................................................... 6000/9003
2019-09-09T17:59:10.0587990Z .................................................................................................... 6000/9003
2019-09-09T17:59:15.8076540Z ........................................................................i..ii....................... 6100/9003
2019-09-09T17:59:45.8996212Z .................................................................................................... 6300/9003
2019-09-09T17:59:47.9656291Z ...............................i.................................................................... 6400/9003
2019-09-09T17:59:50.0932348Z .................................................................................................... 6500/9003
2019-09-09T17:59:52.7333043Z ...i................................................................................................ 6600/9003
---
2019-09-09T18:04:49.4871579Z  finished in 20.795
2019-09-09T18:04:49.5048169Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T18:04:49.6694647Z 
2019-09-09T18:04:49.6694988Z running 150 tests
2019-09-09T18:04:53.1259139Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/150
2019-09-09T18:04:55.2603512Z ..iiii..............i.........iii.i.......ii......
2019-09-09T18:04:55.2607662Z 
2019-09-09T18:04:55.2609364Z  finished in 5.756
2019-09-09T18:04:55.2797014Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T18:04:55.4434312Z 
---
2019-09-09T18:04:57.7266360Z  finished in 2.447
2019-09-09T18:04:57.7514917Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T18:04:57.9194927Z 
2019-09-09T18:04:57.9196765Z running 9 tests
2019-09-09T18:04:57.9198587Z iiiiiiiii
2019-09-09T18:04:57.9199421Z 
2019-09-09T18:04:57.9202207Z  finished in 0.168
2019-09-09T18:04:57.9391299Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T18:04:58.1090873Z 
2019-09-09T18:04:58.1090873Z 
2019-09-09T18:04:58.1091968Z running 104 tests
2019-09-09T18:05:16.7787625Z ............................F...F................................................................... 100/104
2019-09-09T18:05:17.5474254Z ....
2019-09-09T18:05:17.5476638Z failures:
2019-09-09T18:05:17.5478058Z 
2019-09-09T18:05:17.5480059Z ---- [incremental] incremental/hashes/function_interfaces.rs stdout ----
2019-09-09T18:05:17.5481986Z 
2019-09-09T18:05:17.5483736Z error in revision `cfail2`: test compilation failed although it shouldn't!
2019-09-09T18:05:17.5485134Z status: exit code: 1
2019-09-09T18:05:17.5487525Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/function_interfaces.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces/function_interfaces.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces/auxiliary"
2019-09-09T18:05:17.5492137Z ------------------------------------------
2019-09-09T18:05:17.5492920Z 
2019-09-09T18:05:17.5493540Z ------------------------------------------
2019-09-09T18:05:17.5493718Z stderr:
2019-09-09T18:05:17.5493718Z stderr:
2019-09-09T18:05:17.5494018Z ------------------------------------------
2019-09-09T18:05:17.5494206Z error: `mir_built(make_extern)` should be clean but is not
2019-09-09T18:05:17.5494993Z    |
2019-09-09T18:05:17.5494993Z    |
2019-09-09T18:05:17.5495174Z LL | pub extern "C" fn make_extern() {}
2019-09-09T18:05:17.5496455Z 
2019-09-09T18:05:17.5496721Z error: aborting due to previous error
2019-09-09T18:05:17.5496920Z 
2019-09-09T18:05:17.5498145Z 
2019-09-09T18:05:17.5498145Z 
2019-09-09T18:05:17.5499195Z ------------------------------------------
2019-09-09T18:05:17.5502195Z 
2019-09-09T18:05:17.5502619Z 
2019-09-09T18:05:17.5504118Z ---- [incremental] incremental/hashes/inherent_impls.rs stdout ----
2019-09-09T18:05:17.5504470Z 
2019-09-09T18:05:17.5504887Z error in revision `cfail2`: test compilation failed although it shouldn't!
2019-09-09T18:05:17.5505079Z status: exit code: 1
2019-09-09T18:05:17.5506076Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/inherent_impls.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls/inherent_impls.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls/auxiliary"
2019-09-09T18:05:17.5507106Z ------------------------------------------
2019-09-09T18:05:17.5507289Z 
2019-09-09T18:05:17.5507610Z ------------------------------------------
2019-09-09T18:05:17.5507956Z stderr:
2019-09-09T18:05:17.5507956Z stderr:
2019-09-09T18:05:17.5508717Z ------------------------------------------
2019-09-09T18:05:17.5508984Z error: `mir_built(Foo::make_method_extern)` should be clean but is not
2019-09-09T18:05:17.5509660Z    |
2019-09-09T18:05:17.5509660Z    |
2019-09-09T18:05:17.5509832Z LL |     pub extern fn make_method_extern(&self) { }
2019-09-09T18:05:17.5510165Z 
2019-09-09T18:05:17.5510328Z error: aborting due to previous error
2019-09-09T18:05:17.5510470Z 
2019-09-09T18:05:17.5510625Z 
---
2019-09-09T18:05:17.5517188Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:536:22
2019-09-09T18:05:17.5517241Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-09-09T18:05:17.5517271Z 
2019-09-09T18:05:17.5517310Z 
2019-09-09T18:05:17.5519835Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/incremental" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "incremental" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-09-09T18:05:17.5520205Z 
2019-09-09T18:05:17.5520234Z 
2019-09-09T18:05:17.5520282Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-09-09T18:05:17.5520335Z Build completed unsuccessfully in 1:08:43
2019-09-09T18:05:17.5520335Z Build completed unsuccessfully in 1:08:43
2019-09-09T18:05:17.5571567Z == clock drift check ==
2019-09-09T18:05:17.5588125Z   local time: Mon Sep  9 18:05:17 UTC 2019
2019-09-09T18:05:17.7146754Z   network time: Mon, 09 Sep 2019 18:05:17 GMT
2019-09-09T18:05:17.7149198Z == end clock drift check ==
2019-09-09T18:05:20.5808492Z ##[error]Bash exited with code '1'.
2019-09-09T18:05:20.5851626Z ##[section]Starting: Checkout
2019-09-09T18:05:20.5853289Z ==============================================================================
2019-09-09T18:05:20.5853356Z Task         : Get sources
2019-09-09T18:05:20.5853399Z 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)

@joshtriplett joshtriplett added the T-lang label Sep 9, 2019

@gnzlbg gnzlbg force-pushed the gnzlbg:abortffi branch from f657b4d to 801e1b7 Sep 9, 2019

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@gnzlbg assuming your goal here is to "agree on and fix what seems uncontroversial", there is also some other "low-hanging fruit" around unwind, I think? Such as #63883 and the fact that extern "Rust" FFI imports are marked nounwind. It might make sense to also try and fix those while the FFI story is still up in the air?

I think your PR (#63909) fixes most of this other low-hanging fruit correctly. We should do that, maybe without the part that removes nounwind from safe extern "C" definitions. If there is interest in pursuing this approach, we could organize how to land things next week once you come back from the holidays.

Also, this PR should come with a test (we already have a test for #[unwind(abort)], we should have the same test for a safe attribute-free extern "C" fn then).

Sure, done.

we in particular we do not do anything about mozjpeg
We still don't properly insert the shims for all functions then,

Well, that's on purpose. Right now the status quo is that we have a soundness bug open on safe Rust but we can't fix it because around 2 crates are relying on undefined behavior. Until now our strategy has been to block the soundness bug solution until we figure out which problems those crates actually need solving, word RFCs, merge them, implement them, and only after these features are stabilized and these crates migrated to use them, only then can we fix the soundness bug.

This PR fixes the soundness bug without breaking nor fixing those crates. This allow us to close the soundness hole and to solve the problems these crate have without any time pressure nor need to come up with quick temporary features.

So, I am not necessarily opposed to landing this if there is general support. But I do think that we should definitely keep #52652 open (so the "closes" line should be removed from the PR description).

I've removed the "closes" but if we merge this we probably should rename / refocus that issue to reflect what remains to be done. I understood #52652 to be about safe Rust code being able to invoke undefined behavior which is what this PR fixes, but that issue has evolved into how to solve the problems these other crates have because that is believed to be a blocker for fixing the soundness hole.


and rlua being UB.

This is kind off-topic but it has reminded me that this PR might actually trigger a different unrelated bug.

First, AFAICT, rlua currently does not have any undefined behavior of this kind. How could it? It never "unwinds" as in panics or throws exceptions that require cleanup code. Instead, rlua only longjmps. There are some platforms like Windows that implement longjmp using asynchronous exceptions/SEH, but IIUC it is correct to unwind out of LLVM nounwind functions using those mechanisms (emphasis mine):

nounwind: This function attribute indicates that the function never raises an exception. If the function does raise an exception, its runtime behavior is undefined. However, functions marked nounwind may still trap or generate asynchronous exceptions. Exception handling schemes that are recognized by LLVM to handle asynchronous exceptions, such as SEH, will still provide their implementation defined semantics.

Clang has a correctness bug in which trying to longjmp out of a noexcept function on Windows MSVC targets incorrectly aborts the process. We should determine whether rustc has the same bug, because if it has, we are going to run into it sooner or later the moment we start aborting on these functions.

@kyren do the rlua bindings use any "safe" extern "C" callbacks ? If so, then rlua might be affected by such a bug and we should probably fix it before landing this, but AFAICT all the callbacks are unsafe extern "C" functions, so they should just work (and it is correct that they are nounwind).


Does anyone has any idea about what is causing the build errors?

@@ -491,6 +491,7 @@ fn should_abort_on_panic(tcx: TyCtxt<'_>, fn_def_id: DefId, abi: Abi) -> bool {

// Validate `#[unwind]` syntax regardless of platform-specific panic strategy
let attrs = &tcx.get_attrs(fn_def_id);
let unsafety = &tcx.fn_sig(fn_def_id).skip_binder().unsafety.clone();

This comment has been minimized.

Copy link
@gnzlbg

gnzlbg Sep 9, 2019

Author Contributor

@eddyb I don't know if this is the best way to do this. I'm not sure what the binder is for and just ended up playing type tetris here.

This comment has been minimized.

Copy link
@bjorn3

bjorn3 Sep 12, 2019

Contributor

If i am correct, the binder is the for<'a> part of for<'a> fn(&'a ()). This seems correct, as the safety of a function doesnt depend on lifetimes.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

As @RalfJung mentioned in one of the other issues: I don't believe that we should make code generation depend on the presence or absence of unsafe.

This is also missing an explanation of the advantages of this proposal over #[unwind]. In particular, assuming that #[unwind] goes through in a reasonably timely fashion, and that in the meantime we avoid emitting nounwind, this seems like an unnecessary additional complication.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I've removed the "closes" but if we merge this we probably should rename / refocus that issue to reflect what remains to be done. I understood #52652 to be about safe Rust code being able to invoke undefined behavior which is what this PR fixes, but that issue has evolved into how to solve the problems these other crates have because that is believed to be a blocker for fixing the soundness hole.

Then your understanding widely differs from mine. I was not even aware of the soundness bug until you raised it, which was after this recent burst of activity on this issue started.
For me, the bug is "there's something programmers want to do in Rust but they cannot", and more specifically "enabling more UB detection breaks stuff" -- the latter is always a critical bug. The fact that this can cause UB in safe code is just a sideshow. Still worth fixing, but certainly not the main issue at hand.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

@RalfJung

I was not even aware of the soundness bug until you raised it, which was after this recent burst of activity on this issue.

You might be confusing this with the recently reported miscompilation due to the soundness bug, because the soundness bug was discovered at least in 2017, with the first attempted fix landing in December 2017, and people have been trying to fix it ever since. The first comment in the #52652 explains this, and developing new language features to allow programs to unwind through FFI has been discussed pretty much since the start, but it only became "time critical" on March 2019 when it was decided by T-Lang that a fix to the soundness bug shall break no crates.


@joshtriplett

This is also missing an explanation of the advantages of this proposal over #[unwind]

This isn't a proposal over #[unwind], this is a fix for the soundness bug being tracked in #52652 - #[unwind] can still happen and everything here is forward compatible with that.

In particular, assuming that #[unwind] goes through in a reasonably timely fashion, and that in the meantime we avoid emitting nounwind, this seems like an unnecessary additional complication to introduce in the interim.

It is unnecessary to assume that #[unwind] would go through in a reasonably timely fashion, because we don't have to wait till that happens to fix the soundness bug.

and that in the meantime we avoid emitting nounwind,

It isn't clear whether we can avoid emitting nounwind yet as @alexcrichton raised here.

this seems like an unnecessary additional complication to introduce in the interim.

T-Lang agreed that the right fix for this is to both abort and emit nounwind, and that's what this PR does. The unnecessary complication is to temporarily stopping to emit nounwind until we have something like #[unwind] to only then start emitting nounwind and aborting again.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

In other words, this PR implements the solution that @rust-lang/lang agreed is the right thing to do for Rust's safe extern "C" functions (that is, to abort on unwind by default). RFC's like rust-lang/rfcs#2699 (unwind-through-FFI) or rust-lang/rfcs#2753 (simple_unwind_attribute) have to be (and are) compatible with this.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed (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-09-09T18:47:22.9198161Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-09T18:47:22.9391257Z ##[command]git config gc.auto 0
2019-09-09T18:47:22.9467951Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-09T18:47:22.9541161Z ##[command]git config --get-all http.proxy
2019-09-09T18:47:22.9667267Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64315/merge:refs/remotes/pull/64315/merge
---
2019-09-09T19:46:16.9515799Z .................................................................................................... 1500/9003
2019-09-09T19:46:22.0497635Z .................................................................................................... 1600/9003
2019-09-09T19:46:33.5801170Z .....................................................i...............i.............................. 1700/9003
2019-09-09T19:46:40.6948372Z .................................................................................................... 1800/9003
2019-09-09T19:46:53.6342572Z ............................................iiiii................................................... 1900/9003
2019-09-09T19:47:03.5188473Z .................................................................................................... 2100/9003
2019-09-09T19:47:05.8066888Z .................................................................................................... 2200/9003
2019-09-09T19:47:09.3202906Z .................................................................................................... 2300/9003
2019-09-09T19:47:16.3816055Z .................................................................................................... 2400/9003
---
2019-09-09T19:50:00.4214053Z ...............................i...............i.................................................... 4700/9003
2019-09-09T19:50:11.6926798Z .................................................................................................... 4800/9003
2019-09-09T19:50:17.4860555Z .................................................................................................... 4900/9003
2019-09-09T19:50:27.5244986Z .................................................................................................... 5000/9003
2019-09-09T19:50:33.2131440Z .............ii.ii.................................................................................. 5100/9003
2019-09-09T19:50:43.2000178Z .................................................................................................... 5300/9003
2019-09-09T19:50:52.7817267Z ............................................................................i....................... 5400/9003
2019-09-09T19:50:59.8572350Z .................................................................................................... 5500/9003
2019-09-09T19:51:05.7284285Z .................................................................................................... 5600/9003
2019-09-09T19:51:05.7284285Z .................................................................................................... 5600/9003
2019-09-09T19:51:15.7278606Z ......................................................................ii...i..ii...........i........ 5700/9003
2019-09-09T19:51:39.1532940Z .................................................................................................... 5900/9003
2019-09-09T19:51:47.9389324Z .................................................................................................... 6000/9003
2019-09-09T19:51:47.9389324Z .................................................................................................... 6000/9003
2019-09-09T19:51:53.8300438Z ........................................................................i..ii....................... 6100/9003
2019-09-09T19:52:21.2329386Z .................................................................................................... 6300/9003
2019-09-09T19:52:23.1184392Z ...............................i.................................................................... 6400/9003
2019-09-09T19:52:25.1705295Z .................................................................................................... 6500/9003
2019-09-09T19:52:27.6380488Z ...i................................................................................................ 6600/9003
---
2019-09-09T19:56:55.2154257Z  finished in 18.640
2019-09-09T19:56:55.2154704Z Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T19:56:55.2154736Z 
2019-09-09T19:56:55.2154787Z running 150 tests
2019-09-09T19:56:58.1244810Z i....iii......iii..iiii....i.............................i..i..................i....i.........ii.i.i 100/150
2019-09-09T19:56:59.8930764Z ..iiii..............i.........iii.i.......ii......
2019-09-09T19:56:59.8931579Z 
2019-09-09T19:56:59.8931637Z  finished in 4.887
2019-09-09T19:56:59.9097259Z Check compiletest suite=codegen-units mode=codegen-units (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T19:57:00.0515157Z 
---
2019-09-09T19:57:01.9322835Z  finished in 2.022
2019-09-09T19:57:01.9481583Z Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T19:57:02.0902594Z 
2019-09-09T19:57:02.0902775Z running 9 tests
2019-09-09T19:57:02.0903795Z iiiiiiiii
2019-09-09T19:57:02.0904067Z 
2019-09-09T19:57:02.0904101Z  finished in 0.142
2019-09-09T19:57:02.1053364Z Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
2019-09-09T19:57:02.2481642Z 
2019-09-09T19:57:02.2481642Z 
2019-09-09T19:57:02.2481818Z running 104 tests
2019-09-09T19:57:18.1026023Z ............................F...F................................................................... 100/104
2019-09-09T19:57:18.6948633Z ....
2019-09-09T19:57:18.6950422Z failures:
2019-09-09T19:57:18.6950816Z 
2019-09-09T19:57:18.6953572Z ---- [incremental] incremental/hashes/function_interfaces.rs stdout ----
2019-09-09T19:57:18.6955197Z 
2019-09-09T19:57:18.6955867Z error in revision `cfail2`: test compilation failed although it shouldn't!
2019-09-09T19:57:18.6956073Z status: exit code: 1
2019-09-09T19:57:18.6957072Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/function_interfaces.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces/function_interfaces.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/function_interfaces/auxiliary"
2019-09-09T19:57:18.6957893Z ------------------------------------------
2019-09-09T19:57:18.6958031Z 
2019-09-09T19:57:18.6958343Z ------------------------------------------
2019-09-09T19:57:18.6958495Z stderr:
2019-09-09T19:57:18.6958495Z stderr:
2019-09-09T19:57:18.6958768Z ------------------------------------------
2019-09-09T19:57:18.6958953Z error: `mir_built(make_extern)` should be clean but is not
2019-09-09T19:57:18.6960131Z    |
2019-09-09T19:57:18.6960131Z    |
2019-09-09T19:57:18.6960312Z LL | pub extern "C" fn make_extern() {}
2019-09-09T19:57:18.6960613Z 
2019-09-09T19:57:18.6962098Z error: aborting due to previous error
2019-09-09T19:57:18.6962262Z 
2019-09-09T19:57:18.6962394Z 
2019-09-09T19:57:18.6962394Z 
2019-09-09T19:57:18.6963418Z ------------------------------------------
2019-09-09T19:57:18.6964064Z 
2019-09-09T19:57:18.6964249Z 
2019-09-09T19:57:18.6964907Z ---- [incremental] incremental/hashes/inherent_impls.rs stdout ----
2019-09-09T19:57:18.6965161Z 
2019-09-09T19:57:18.6965503Z error in revision `cfail2`: test compilation failed although it shouldn't!
2019-09-09T19:57:18.6965659Z status: exit code: 1
2019-09-09T19:57:18.6966730Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/incremental/hashes/inherent_impls.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--cfg" "cfail2" "-C" "incremental=/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls/inherent_impls.inc" "-Z" "incremental-verify-ich" "-Z" "incremental-queries" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "query-dep-graph" "-Zincremental-ignore-spans" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental/hashes/inherent_impls/auxiliary"
2019-09-09T19:57:18.6967330Z ------------------------------------------
2019-09-09T19:57:18.6967470Z 
2019-09-09T19:57:18.6967778Z ------------------------------------------
2019-09-09T19:57:18.6967925Z stderr:
2019-09-09T19:57:18.6967925Z stderr:
2019-09-09T19:57:18.6968209Z ------------------------------------------
2019-09-09T19:57:18.6968385Z error: `mir_built(Foo::make_method_extern)` should be clean but is not
2019-09-09T19:57:18.6968864Z    |
2019-09-09T19:57:18.6968864Z    |
2019-09-09T19:57:18.6969016Z LL |     pub extern fn make_method_extern(&self) { }
2019-09-09T19:57:18.6969434Z 
2019-09-09T19:57:18.6970111Z error: aborting due to previous error
2019-09-09T19:57:18.6970244Z 
2019-09-09T19:57:18.6970373Z 
---
2019-09-09T19:57:18.6972892Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:536:22
2019-09-09T19:57:18.6973542Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
2019-09-09T19:57:18.6973700Z 
2019-09-09T19:57:18.6973830Z 
2019-09-09T19:57:18.6975138Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/incremental" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/incremental" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "incremental" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-09-09T19:57:18.6975553Z 
2019-09-09T19:57:18.6975658Z 
2019-09-09T19:57:18.6975775Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-09-09T19:57:18.6976000Z Build completed unsuccessfully in 1:02:45
2019-09-09T19:57:18.6976000Z Build completed unsuccessfully in 1:02:45
2019-09-09T19:57:18.7014381Z == clock drift check ==
2019-09-09T19:57:18.7027009Z   local time: Mon Sep  9 19:57:18 UTC 2019
2019-09-09T19:57:18.8869035Z   network time: Mon, 09 Sep 2019 19:57:18 GMT
2019-09-09T19:57:18.8875802Z == end clock drift check ==
2019-09-09T19:57:23.0235533Z ##[error]Bash exited with code '1'.
2019-09-09T19:57:23.0273068Z ##[section]Starting: Checkout
2019-09-09T19:57:23.0274954Z ==============================================================================
2019-09-09T19:57:23.0274998Z Task         : Get sources
2019-09-09T19:57:23.0275053Z 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)

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

You might be confusing this with the recently reported miscompilation due to the soundness bug, because the soundness bug was discovered at least in 2017, with the first attempted fix landing in December 2017, and people have been trying to fix it ever since. The first comment in the #52652 explains this, and developing new language features to allow programs to unwind through FFI has been discussed pretty much since the start, but it only became "time critical" on March 2019 when it was decided by T-Lang that a fix to the soundness bug shall break no crates.

All of that discussion was about turning UB into a defined abort, and how that broke stuff, but AFAIK never was it mentioned that it is possible to cause this UB from safe code only. There was never a soundness bug until 14 days ago.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

this is a fix for the soundness bug being tracked in #52652 - #[unwind] can still happen and everything here is forward compatible with that.

See #52652 (comment) for a comment explaining that that bug is not a soundness bug in the usual sense of the word. Emphasis mine:

It is indeed not rustc or std that is unsound, but the current situation is that it is very difficult to write sound FFI code.

You are misreading history here.

T-Lang agreed that the right fix for this is to both abort and emit nounwind, and that's what this PR does.

T-lang agreed that the right fix is to do this for all extern "C" (except when opting out with an attribute). That is not what this PR does.
This PR replaces "opt-out with an attribute" by "opt-out by adding unsafe". That's a cute hack. It is by no means a proper fix, and also not fixing any of the issues or implementing any of the proposals that have been floating around, except for #63943.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

All of that discussion was about turning UB into a defined abort, and how that broke stuff, but AFAIK never was it mentioned that it is possible to cause this UB from safe code only.

That's the only reason #52652 is tagged with I-Unsound:

extern "C" fn foo() { panic() } // Safe Rust invokes UB

There was never a soundness bug until 14 days ago.

That's just a miscompilation caused by that unsoundness.

See #52652 (comment) for a comment explaining that that bug is not a soundness bug in the usual sense of the word.

See these comments refuting that comment, starting at this one: #52652 (comment) and the couple that follow showing UB in safe Rust. Particularly this comment: #52652 (comment) which shows the same UB that leads to the miscompilation I filled two weeks ago.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

T-lang agreed that the right fix is to do this for all extern "C" (except when opting out with an attribute).

Sure, as I mentioned, this PR implements the part of that required to fix the soundness hole, leaving the rest for later.

This PR replaces "opt-out with an attribute" by "opt-out by adding unsafe".

On stable Rust, there is no attribute, so it is unclear to me how this PR could replace that with anything. On nightly Rust, there is an attribute, but this PR does not replace it.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Particularly this comment: #52652 (comment) which shows the same UB that leads to the miscompilation I filled two weeks ago.

Fair, I had missed that. (It's not "the first comment though" as you had claimed.)

But #52652 was marked I-unsound before that already. It is not, primarily, a soundness bug. It primarily is about letting people write FFI code like mozjpeg does. The issue description does not even mention (un)soundness.

On stable Rust, there is no attribute, so it is unclear to me how this PR could replace that with anything. On nightly Rust, there is an attribute, but this PR does not replace it.

It's conceptually replacing it, as in, it is doing the same thing.
Having both the attribute and unsafe available as a switch here doesn't even make much sense (and if I understand your comments, you also do not intend it as long-term solution).

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

(It's not "the first comment though" as you had claimed.)
But #52652 was marked I-unsound before that already.

It's linked in the first comment (in the original comment part, not the updated part at the top), which links to the PR that fixed this for the first time, where this was discussed, and the announcement of Rust 1.24.0 which mentions that we fix a case where you could invoke undefined behavior in safe Rust: https://blog.rust-lang.org/2018/02/15/Rust-1.24.html#other-good-stuff

The reason that this issue was marked with I-Unsound initially is that the first and only report of this that one can find is the PR that actually fixed it for the first time. If there were any other issues filled before that, I can't find them. That PR was merged, stuff broke, reverted, and this issue was filled at some point after.. with I-Unsound, by somebody else that wasn't the PR author. I guess this is another reason to not use PRs as bug reports =/ (EDIT: #18510)

It's conceptually replacing it, as in, it is doing the same thing.
Having both the attribute and unsafe available as a switch here doesn't even make much sense (and if I understand your comments, you also do not intend it as long-term solution).

Right now, even if we stop emitting nounwind, both safe and unsafe extern fns that panic are UB. It's UB that we might not exploit, but it is still UB. That is, right now, both the safe and unsafe extern "C" function cases are unsafe.

You might see fixing the safe function case as making the unsafe case the opt out of safety, but that's already the case.

Sure, until a proper solution lands, this might make users that are invoking undefined behavior in safe Rust code change their code to "only" invoke undefined behavior in unsafe Rust code. Not a great improvement, but IMO an improvement nevertheless. That can be seen as an opt-out on stable, but there is no need to do that on nightly because one has #[unwind] there.

@kyren

This comment has been minimized.

Copy link
Contributor

commented Sep 9, 2019

@kyren do the rlua bindings use any "safe" extern "C" callbacks ? If so, then rlua might be affected by such a bug and we should probably fix it before landing this, but AFAICT all the callbacks are unsafe extern "C" functions, so they should just work (and it is correct that they are nounwind).

No, absolutely all rust functions marked extern "C" in rlua are unsafe, so this would not break rlua AIUI.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

@gnzlbg I don't recall a distinction between safe and unsafe functions ever coming up in the language team discussions, and it certainly wasn't in the conclusion from the lang team meeting. The conclusion from the language team meeting is that the state we'd like to get to is a consistent one in which all functions (safe or unsafe) not marked as unwinding are both marked nounwind for optimization by default, and that both abort on attempted unwind. (Thanks to @RalfJung for summarizing that point while I was in the process of writing this comment.) The language team also agreed that our current inconsistent state (not aborting but still adding nounwind) is not right; we talked about merging the PR to stop adding nounwind in the interim to address the current UB as soon as possible, as the RFC for #[unwind] seems to keep getting derailed. (Ralf has said the same thing, and I would like to just merge Ralf's PR.)

Right now, even if we stop emitting nounwind, both safe and unsafe extern fns that panic are UB.

Are you referring to the lack of specification for how Rust unwinding interacts with other languages? If so: RFC 2753 is trying to address that through some minimal additions of specification/definition.

In the meantime, I personally am inclined to merge Ralf's PR at #63909 to fix potential miscompilation, and that was the lang team consensus as well, though I'm going to go confirm that again. With that PR merged, we shouldn't generate any further miscompilation. We would miss some optimization opportunities, but we can get those optimization opportunities back once we introduce #[unwind] (or some other means for developers to make rust stop emitting nounwind on extern "C" functions that might unwind). And yes, actually using the unwind behavior across languages will still be undefined, but in a well-understood way (namely that you need to use the right compiler options), and at least it'll remain functional while work continues on defining its behavior.

This PR, on the other hand, introduces some additional semantics to unsafe, without an RFC, and without any documentation. Like @RalfJung, I don't think this is a good idea.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2019

Are you referring to the lack of specification for how Rust unwinding interacts with other languages?

Per the Rust language reference, extern "C" function are not allowed to unwind, and therefore, it is ok to emit nounwind for them.

we talked about merging the PR to stop adding nounwind in the interim to address the current UB as soon as possible,

We don't have to emit nounwind, but removing nounwind does not remove the undefined behavior nor the soundness hole in the language. It disables an optimization that exploits it, but that's it, if an extern "C" function unwinds, the behavior is still undefined according to the language reference.

And yes, actually using the unwind behavior across languages will still be undefined

Not only across languages. The behavior of this safe Rust program is undefined if foo unwinds:

extern "C" fn foo() { panic!() } 
fn main() { foo() }

independently of whether we emit nounwind or not, according to the reference.

This PR, on the other hand, introduces some additional semantics to unsafe

Which additional semantics does this PR introduce? This PR does not allow unsafe code to do anything new (independently of whether @RalfJung 's PR is merged as well or not). This PR does however limit what safe Rust code can currently do, by preventing examples like the above from invoking undefined behavior.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

It's linked in the first comment (in the original comment part, not the updated part at the top), which links to the PR that fixed this for the first time, where this was discussed, and the announcement of Rust 1.24.0 which mentions that we fix a case where you could invoke undefined behavior in safe Rust: https://blog.rust-lang.org/2018/02/15/Rust-1.24.html#other-good-stuff

None of this talks about being able to trigger UB in safe code. It's all about having UB on an FFI boundary, so there's unsafe code involved.

AFAIK #52652 (comment) is the first time it was shown that safe Rust can run into this. And that comment (somewhat surprisingly I have to say) did not get much of a reaction. The discussion continued to be solely about the FFI case.

The only one talking about the soundness issue since then is you, AFAIK. I wasn't even aware I was fixing a soundness issue when writing #63909.

The reason that this issue was marked with I-Unsound initially is that the first and only report of this that one can find is the PR that actually fixed it for the first time.

That is incorrect, I quoted above the reason it is marked I-Unsound:

It is indeed not rustc or std that is unsound, but the current situation is that it is very difficult to write sound FFI code.

(from #52652 (comment), which is before the soundness issue was even raised for the first time)


Not that it matters very much what happened back then, though... the matter right now is that we have two PRs fixing the soundness issue, this one here and #63909.

This one here has the advantage of minimizing the possible performance impact, but OTOH it makes unsafe relevant for codegen which is very odd at best.

The other one goes further, risking a bigger perf/codesize impact, but OTOH it also removes "de facto" (but not "de jure") UB from mozjpeg and basically just completes what we started but didn't finish in #62603. It is true that this PR only "de facto" fixes the soundness hole @gnzlbg is worried about, but everything about this situation is temporary so that does not seem like a big problem.


Per the Rust language reference, extern "C" function are not allowed to unwind, and therefore, it is ok to emit nounwind for them.

I think @gnzlbg is talking about "de jure" UB here, while @joshtriplett was asking about "de facto" UB. As in, even with my PR, mozjpeg is still UB under the Rust spec, and only an RFC can fix that. But, with my PR, we reliably generate UB-free LLVM IR for mozjpeg.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@RalfJung See: #18510 which is the original soundness bug report, which does contain the UB in safe Rust down in the comments. That issue was closed when the first PR with a fix for this landed, but when that failed, #52652 was reopened.

The other one goes further, risking a bigger perf/codesize impact, but OTOH it also removes "de facto" (but not "de jure") UB from mozjpeg and basically just completes what we started but didn't finish in #62603.

FWIW (as mentioned above) I think we should do both. Remove "de jure" UB from safe Rust code with this PR, and remove "de facto" UB from unsafe Rust code with @RalfJung PR. That's as good as it can get without an RFC: it fixes the soundness hole in safe Rust, and it allows unsafe Rust code to not be "mis"-optimized until an RFC with a better solution lands.

@gnzlbg gnzlbg changed the title Fixes soundness bug 52652 by aborting on unwind from safe extern "C" functions only Fixes soundness bug 18510 by aborting on unwind from safe extern "C" functions only Sep 13, 2019

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2019

So I've updated the top comment to make it clear which problem this solves.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 13, 2019

Having read the discussion, this PR seems reasonable to land as a temporary crutch so that the soundness hole, which I regard as the primary bug, is resolved.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

See: #18510 which is the original soundness bug report, which does contain the UB in safe Rust down in the comments. That issue was closed when the first PR with a fix for this landed, but when that failed, #52652 was reopened.

Again that bug was not originally about soundness within Rust, it was all about FFI. Both #18510 and #52652 are (in their OP description) specifically about FFI. But you are right insofar that the Rust soundness issue was mentioned in there.

Still, #63943 I think is the first issue that is specifically about the soundness issue fixed here. I think this PR closes #63943, does it not? Could you add that to the PR description? Also I think that description mischaracterizes what #18510 is about, but whatever.

by implementing the behavior defined in the reference which requires safe extern "C" functions to abort when a panic! tries to unwind out of them.

This makes it sound like the reference says something specifically about safe extern "C" functions. It does not.

Having read the discussion, this PR seems reasonable to land as a temporary crutch so that the soundness hole, which I regard as the primary bug, is resolved.

The primary bug in what context? Note that in the lang team discussions, at least the one where I was present, the soundness hole was not even mentioned. There is a whole lot more to this unwinding discussion than this soundness hole.

IMO #63909 is the "better" clutch to fix the soundness issue (at last to fix the de-facto UB), and it does not require making codegen depend on the unsafe keyword -- a big fat warning sign IMO, and a very dangerous precedent.

@Centril

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

The primary bug in what context? Note that in the lang team discussions, at least the one where I was present, the soundness hole was not even mentioned. There is a whole lot more to this unwinding discussion than this soundness hole.

The lang team discussions started as a result of FCP-merge on #58794 (comment). I proposed FCP so that the UB in safe code would be resolved. The unwinding aspect took over once that became the blocker for closing the soundness hole.

a big fat warning sign IMO, and a very dangerous precedent.

I don't disagree and would definitely not like this PR on a permanent basis either. But when it comes down to it, I find that closing the soundness without inviting more reliance on spec-UB is more important.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

@RalfJung You are mixing things up.

The Rust language does not allow extern "C" functions to unwind.

The soundness bug is that it is possible to write safe Rust code that causes an extern "C" function to unwind and therefore it allows safe Rust to exhibit undefined behavior. This was reported in #18510.

We use the nounwind attribute on extern "C" functions. Therefore, if an extern "C" function unwinds, the behavior is not only undefined in Rust, but also in LLVM-IR. Such undefined behavior in LLVM was reported in #18510 as well.

You reported that you did not known of any actual code for which LLVM optimized its IR by using nounwind. I searched for a report of such a case and could not find any, so I filled #63943 to report that this issue was not only theoretical, but also practical. Doing this was impossible to do with Rust nightly for a long time, and only became recently possible again with #62603. Maybe I should close #63943 and post the report to #18510 instead, and reopen that one.

I think this PR closes #63943, does it not?

This PR closes the soundness hole, and therefore when the optimization shown in #63943 happens in safe Rust, it is always correct. However, after this PR that optimization can still happen in unsafe Rust code, so the problem reported in #63943 is still there (just replace fn with unsafe fn).

This makes it sound like the reference says something specifically about safe extern "C" functions. It does not.

I've clarified that to:

by implementing the behavior defined in the reference for all extern "C" functions, which is to abort when a panic! tries to unwind out of them, only for safe extern "C" functions.

@RalfJung

This comment has been minimized.

Copy link
Member

commented Sep 14, 2019

The lang team discussions started as a result of FCP-merge on #58794 (comment). I proposed FCP so that the UB in safe code would be resolved. The unwinding aspect took over once that became the blocker for closing the soundness hole.

I see. I was not aware the lang team talked about the soundness issue back then.

Sorry for my misunderstandings here.

However, after this PR that optimization can still happen in unsafe Rust code, so the problem reported in #63943 is still there (just replace fn with unsafe fn).

But here you are contradicting yourself... "The Rust language does not allow extern "C" functions to unwind", and hence there is no miscompilation in #63943 (after this PR lands, and replacing fn by unsafe fn). We can only call this a miscompilation because it is entirely safe code.

@gnzlbg

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2019

"The Rust language does not allow extern "C" functions to unwind", and hence there is no miscompilation in #63943.

@RalfJung you are completely right: #63943 is not a miscompilation.

We can only call this a miscompilation because it is entirely safe code.

I consider the soundness bug and the ""miscompilation"" to be separate issues, but the actual reason I called #63943 a miscompilation is because this optimization ""breaks"" code that has undefined behavior.

The lang team has set as a hard constraint that solutions to the soundness bug shall not break code that exhibits undefined behavior. I'm open to renaming the issue to something that clarifies that (e.g. "unsound optimization for unsound code" or something like that) since apparently this is the new normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.