"Aborted (core dumped)" with asm! after "error: expected expression, found `<eof>`"
dwrensha opened this issue · 12 comments
rustc crashes ungracefully when given the following input (found by fuzz-rustc):
asm!(f(assert_eq!(f/$ rustc -vV
rustc 1.38.0-nightly (273f42b59 2019-07-21)
binary: rustc
commit-hash: 273f42b5964c29dda2c5a349dd4655529767b07f
commit-date: 2019-07-21
host: x86_64-unknown-linux-gnu
release: 1.38.0-nightly
LLVM version: 9.0
$ cat crash.rs
asm!(f(assert_eq!(f/
$ rustc crash.rs
error: this file contains an un-closed delimiter
--> crash.rs:1:22
|
1 | asm!(f(assert_eq!(f/
| - - - ^
| | | |
| | | un-closed delimiter
| | un-closed delimiter
| un-closed delimiter
error: macros that expand to items must be delimited with braces or followed by a semicolon
--> crash.rs:1:5
|
1 | asm!(f(assert_eq!(f/
| ^^^^^^^^^^^^^^^^^
help: change the delimiters to curly braces
|
1 | asm! {f(assert_eq!(f/}
| ^ ^
help: add a semicolon
|
1 | asm!(f(assert_eq!(f/;
| ^
error[E0658]: use of unstable library feature 'asm': inline assembly is not stable enough for use and is subject to change
--> crash.rs:1:1
|
1 | asm!(f(assert_eq!(f/
| ^^^
|
= note: for more information, see https://github.com/rust-lang/rust/issues/29722
= help: add `#![feature(asm)]` to the crate attributes to enable
error: expected expression, found end of macro arguments
--> crash.rs:1:21
|
1 | asm!(f(assert_eq!(f/
| ^ expected expression
Aborted (core dumped)
I'm seeing this behavior on stable, beta, and nightly.
Interesting:
__pthread_kill (@7fff778c22bc..7fff778c22d1:6)
pthread_kill (@7fff7797dad5..7fff7797dc45:72)
abort (@7fff7782c627..7fff7782c6b5:34)
std::sys::unix::abort_internal::hbaad8c3f22b3ec7d (/Users/ekuber/personal/rust/src/libstd/sys/unix/mod.rs:155)
std::process::abort::h37e30bbec9288051 (/Users/ekuber/personal/rust/src/libstd/process.rs:1575)
syntax::mut_visit::visit_clobber::_$u7b$$u7b$closure$u7d$$u7d$::h4970299f5550d0e2 (/Users/ekuber/personal/rust/src/libsyntax/mut_visit.rs:313)
core::result::Result$LT$T$C$E$GT$::unwrap_or_else::hbd2dbc27337f3b0c (/Users/ekuber/personal/rust/src/libcore/result.rs:818)
syntax::mut_visit::visit_clobber::h14cca7e5ec9958ef (/Users/ekuber/personal/rust/src/libsyntax/mut_visit.rs:312)
_$LT$syntax..ext..expand..MacroExpander$u20$as$u20$syntax..mut_visit..MutVisitor$GT$::visit_expr::h38398b7469c62fd3 (/Users/ekuber/personal/rust/src/libsyntax/ext/expand.rs:123)
syntax::ext::base::expr_to_spanned_string::h6b6583cd2a58ef7f (/Users/ekuber/personal/rust/src/libsyntax/ext/base.rs:906)
syntax::ext::base::expr_to_string::h90aad5a452d41508 (/Users/ekuber/personal/rust/src/libsyntax/ext/base.rs:920)
syntax_ext::asm::parse_inline_asm::hbc1fc40c893dab17 (/Users/ekuber/personal/rust/src/libsyntax_ext/asm.rs:124)
syntax_ext::asm::expand_asm::h546034de8fea0994 (/Users/ekuber/personal/rust/src/libsyntax_ext/asm.rs:48)
core::ops::function::Fn::call::h6c6b3d82f4a95de8 (/Users/ekuber/personal/rust/src/libcore/ops/function.rs:69)
_$LT$F$u20$as$u20$syntax..ext..base..TTMacroExpander$GT$::expand::h9bfa730e070ce269 (/Users/ekuber/personal/rust/src/libsyntax/ext/base.rs:262)
syntax::ext::expand::MacroExpander::expand_invoc::hc8f97540ca305bf3 (/Users/ekuber/personal/rust/src/libsyntax/ext/expand.rs:520)
syntax::ext::expand::MacroExpander::expand_fragment::hbd32ab45d6342f65 (/Users/ekuber/personal/rust/src/libsyntax/ext/expand.rs:338)
syntax::ext::expand::MacroExpander::expand_crate::h163e2199cfabbe00 (/Users/ekuber/personal/rust/src/libsyntax/ext/expand.rs:268)
rustc_interface::passes::configure_and_expand_inner::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::hfe2f494ac3082816 (/Users/ekuber/personal/rust/src/librustc_interface/passes.rs:425)
rustc::util::common::time_ext::h20692c834a177b56 (/Users/ekuber/personal/rust/src/librustc/util/common.rs:151)
I'm not familiar with visit_cobbler but that's where we are aborting:
/// Use a map-style function (`FnOnce(T) -> T`) to overwrite a `&mut T`. Useful
/// when using a `flat_map_*` or `filter_map_*` method within a `visit_`
/// method. Abort the program if the closure panics.
//
// No `noop_` prefix because there isn't a corresponding method in `MutVisitor`.
pub fn visit_clobber<T, F>(t: &mut T, f: F) where F: FnOnce(T) -> T {
unsafe {
// Safe because `t` is used in a read-only fashion by `read()` before
// being overwritten by `write()`.
let old_t = ptr::read(t);
let new_t = panic::catch_unwind(panic::AssertUnwindSafe(|| f(old_t)))
.unwrap_or_else(|_| process::abort());
ptr::write(t, new_t);
}
}cc @nnethercote
If I change the body of visit_clobber to this:
*t = f(t.clone());
then the compiler aborts how you would expect.
I caught the panic in gdb (with break rust_panic). It is coming from here. Here's the stack trace:
#0 rust_panic () at src/libstd/panicking.rs:526
#1 0x00007ffff52a5fb7 in std::panicking::update_count_then_panic ()
at src/libstd/panicking.rs:516
#2 0x00007ffff52adf86 in std::panic::resume_unwind () at src/libstd/panic.rs:427
#3 0x00007ffff74b45b3 in rustc_errors::FatalError::raise () at src/librustc_errors/lib.rs:267
#4 0x00007ffff73a7142 in syntax::ext::tt::macro_parser::parse_nt ()
at src/libsyntax/ext/tt/macro_parser.rs:927
#5 syntax::ext::tt::macro_parser::parse () at src/libsyntax/ext/tt/macro_parser.rs:791
#6 0x00007ffff73c8ba2 in syntax::tokenstream::TokenTree::parse ()
at src/libsyntax/tokenstream.rs:71
#7 syntax::ext::tt::macro_rules::generic_extension () at src/libsyntax/ext/tt/macro_rules.rs:140
#8 <syntax::ext::tt::macro_rules::MacroRulesMacroExpander as syntax::ext::base::TTMacroExpander>::e
xpand () at src/libsyntax/ext/tt/macro_rules.rs:107
#9 0x00007ffff72a4867 in syntax::ext::expand::MacroExpander::expand_invoc ()
at src/libsyntax/ext/expand.rs:520
#10 syntax::ext::expand::MacroExpander::expand_fragment () at src/libsyntax/ext/expand.rs:338
#11 0x00007ffff72750b9 in <syntax::ext::expand::MacroExpander as syntax::mut_visit::MutVisitor>::vis
it_expr::{{closure}} () at src/libsyntax/ext/expand.rs:123
#12 syntax::mut_visit::visit_clobber::{{closure}} () at src/libsyntax/mut_visit.rs:312
#13 <std::panic::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once ()
at /home/njn/moz/rust1/src/libstd/panic.rs:315
#14 std::panicking::try::do_call () at /home/njn/moz/rust1/src/libstd/panicking.rs:296
#15 0x00007ffff52d699a in __rust_maybe_catch_panic () at src/libpanic_unwind/lib.rs:82
#16 0x00007ffff727371d in std::panicking::try ()
at /home/njn/moz/rust1/src/libstd/panicking.rs:275
#17 0x00007ffff72cfb8e in std::panic::catch_unwind ()
at /home/njn/moz/rust1/src/libstd/panic.rs:394
#18 syntax::mut_visit::visit_clobber () at src/libsyntax/mut_visit.rs:312
#19 <syntax::ext::expand::MacroExpander as syntax::mut_visit::MutVisitor>::visit_expr ()
at src/libsyntax/ext/expand.rs:123
#20 syntax::ext::base::expr_to_spanned_string () at src/libsyntax/ext/base.rs:906
#21 0x00007ffff72cfd22 in syntax::ext::base::expr_to_string () at src/libsyntax/ext/base.rs:920
#22 0x00007ffff68de07a in syntax_ext::asm::parse_inline_asm () at src/libsyntax_ext/asm.rs:124
#23 syntax_ext::asm::expand_asm () at src/libsyntax_ext/asm.rs:48
So it seems like this is a "legitimate" panic from the parser that the compiler would normally catch gracefully somewhere... except that visit_clobber assumes the closure it calls never panics and just aborts immediately if so. visit_clobber's current implementation arose out of this discussion. @matklad, any thoughts on how visit_clobber could let the panic pass through in a memory-safe manner? Or is that not possible?
@nnethercote I think there’s no magic trick here. Afaik the only safe way to do this is to mem::replace t with a dummy value, apply f, and swap the result back, but that requires that t has a dummy value. Does the code break if you add T: Default bound?
Also, when suggesting that change I was 80% sure that it’s pure pedantry because, clearly, we don’t panic anywhere :) I am fascinated by how conspicuous and shallow such “benign” problems become in rust!
cc @petrochenkov re:macros/parse_nt being panicky.
Does the code break if you add T: Default bound?
Yes. Unfortunately, lots of the involved types lack a Default bound, and adding one doesn't make sense.
@eddyb wants the commit that introduced visit_clobber to be reverted anyway. Perhaps that's the best path forward, even though it'll be a perf hit.
Also, when suggesting that change I was 80% sure that it’s pure pedantry because, clearly, we don’t panic anywhere :) I am fascinated by how conspicuous and shallow such “benign” problems become in rust!
If I'd stuck with the original code that didn't handle panics it would have been a double-free without warning. One of those "unsafe is trickier than you might think" moments.
Or we just accept the current behaviour, because it's a benign abort on a very obscure case, and panictry! is being phased out anyway...
What's the plan post-panictry!?
it's a benign abort on a very obscure case
Benign to rustc, but it will kill RLS or any other long lived tool. (Not that I disagree that we can live with this in the medium term.)
Just in case it's useful: here is a variation that causes (what I assume is) the same abort with only the assert_eq! macro:
fn f() { assert_eq!(f(), (), assert_eq!(assert_eq!I am unable to reproduce this crash on the latest rustc releases.
I was curious what fixed it, so I tried bisecting with nightlies. The crash still happens on rustup override set nightly-2019-08-15 but no longer happens on rustup override set nightly-2019-08-21. Unfortunately, it looks like nightlies are not available in between those two.
In any case, as far as I'm concerned, this issue may be closed now.
Let's close this with a PR containing these repro cases as regression tests.