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

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

Open
dwrensha opened this issue Jul 23, 2019 · 10 comments

Comments

@dwrensha
Copy link
Contributor

commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

@estebank

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

@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 estebank added the A-macros label Jul 25, 2019

@estebank

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

cc @petrochenkov re:macros/parse_nt being panicky.

@nnethercote

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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

@estebank

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

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

@dwrensha

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

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!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.