"Aborted (core dumped)" with asm! after "error: expected expression, found `<eof>`"

[dwrensha]

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.

[Centril]

cc @estebank

[estebank]

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);
    }
}

[dwrensha]

cc @nnethercote

[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?

[matklad]

@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!

[estebank]

cc @petrochenkov re:macros/parse_nt being panicky.

[nnethercote]

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.

[nnethercote]

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...