Implement calls to exported symbols (#1170) #1776

ghost · 2021-04-16T09:15:37Z

Closes #1170.

src/shims/foreign_items.rs

RalfJung · 2021-04-16T09:30:16Z

Miri becomes unacceptably slow (at least on my machine): it seems stuck at mangling functions.

What you do here should not have any effect on executions that do not do such function calls -- is that currently not the case?

RalfJung · 2021-04-16T09:31:43Z

Also I don't think we should support anything relying on mangling. What if you restrict this to calling functions with an explicit link_name or no_mangle?

ghost · 2021-04-16T13:42:20Z

What you do here should not have any effect on executions that do not do such function calls -- is that currently not the case?

It's not. A custom #[no_mangle] (or #[export_name]) function can override a shim here (I hope to detect errors like https://rust-lang.zulipchat.com/#narrow/stream/122651-general/topic/Playground.20segfault/near/230231355), so the exported_symbols search is performed every time when any extern function is called.

Also I don't think we should support anything relying on mangling. What if you restrict this to calling functions with an explicit link_name or no_mangle?

I dropped mangled functions support and it seems faster now.

RalfJung · 2021-04-16T14:02:14Z

A custom #[no_mangle] (or #[export_name]) function can override a shim here

Oh... that's not what #1170 is about.^^ I would expect hard-coded shims to take precedence. That would also solve any performance concerns.

If we can overwrite shims, shouldn't we also have a way for you to still somehow call the actual underlying implementation? That seems like a separate feature, maybe we can leave it to a separate PR.

ghost · 2021-04-16T14:57:18Z

Oh... that's not what #1170 is about.^^ I would expect hard-coded shims to take precedence. That would also solve any performance concerns.

I was just hoping to detect more undefined behaviors (caused by using #[no_mangle] to override a libc function accidentally, since custom #[no_mangle] functions are taking precedence in practice).

If we can overwrite shims, shouldn't we also have a way for you to still somehow call the actual underlying implementation?

That sounds like RTLD_NEXT...
But maybe it's better to emit an error other than letting it override the shim silently? Actually I don't know what will happen in practice on non-Linux platforms or when libc is statically linked if someone tries to define a function that has the same symbol name with a libc function. If the linker is going to emit an error or make a random choice, a throw_unsup_format is fine, I think.

tests/compile-fail/non_extern_foreign_call/clashing.rs

src/shims/foreign_items.rs

src/bin/miri.rs

RalfJung · 2021-04-16T17:20:46Z

I was just hoping to detect more undefined behaviors (caused by using #[no_mangle] to override a libc function accidentally, since custom #[no_mangle] functions are taking precedence in practice).

Hm, that is fair -- but it seems like a very different usecase from #1170: there you want to actually execute that user-defined code; here it seems better to just abort execution? Jumping to the user-defined shim would only be correct if this is always guaranteed to happen, and as you said I am not sure if that is true e.g. when statically building binaries with musl.

ghost · 2021-04-16T17:46:29Z

here it seems better to just abort execution?

I agree, but does that mean I need to add the same check (to abort it) to all the arms in those long matches that I added check_abi last time?
(I really hope there's an iterable list of Miri supported shims somewhere in the code...)

RalfJung · 2021-04-16T17:49:49Z

Hm... it might be better to combine check_args, check_abi, and check_<whatever, something for link_name> into one function that does it all. If that function receives the name of the shim (which it needs for the new check) we'd also have better error messages than we do currently for arg/ABI mismatches.

But that seems entirely orthogonal to what you implemented here. So maybe first turn this PR into a solution for #1170, i.e., calling non-foreign extern functions when no shim was found -- and then in a follow-up PR add the new check?

ghost · 2021-04-16T17:51:44Z

👍

cargo-miri/bin.rs

src/shims/foreign_items.rs

tests/run-pass/non_extern_foreign_call.rs

tests/compile-fail/non_extern_foreign_call/abi_mismatch.rs

tests/compile-fail/non_extern_foreign_function_clashing.rs

bors · 2021-04-20T08:01:57Z

☔ The latest upstream changes (presumably #1779) made this pull request unmergeable. Please resolve the merge conflicts.

src/bin/miri.rs

src/shims/foreign_items.rs

test-cargo-miri/test.default.stdout.ref

RalfJung · 2021-04-23T14:13:39Z

'm currently not sure how to test or emit an error for unwinding

Every time a stack frame is popped, this function is called:

miri/src/shims/panic.rs

Line 120 in 58436e9

fn handle_stack_pop(

There you should be able to detect a situation where unwinding == true and you are inside a function with an ABI that does not allow unwinding (because its ABI is, for example, extern "C").

RalfJung · 2021-05-31T07:26:33Z

(Please let me know when this is ready for another round of review.)

src/bin/miri.rs

RalfJung · 2021-06-03T15:48:00Z

This is great, thanks a lot @hyd-dev for working through so many rounds of review. :)
@bors r+

bors · 2021-06-03T15:48:01Z

📌 Commit 57e4f1d has been approved by RalfJung

bors · 2021-06-03T16:00:01Z

⌛ Testing commit 57e4f1d with merge 28717a0...

bors · 2021-06-03T16:16:28Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 28717a0 to master...

Report an error if a `#[no_mangle]`/`#[export_name = ...]` function has the same symbol name as a built-in shim Implements #1776 (comment). The error looks like this: ``` error: found `malloc` symbol definition that clashes with a built-in shim --> tests/compile-fail/function_calls/exported_symbol_shim_clashing.rs:12:9 | 12 | malloc(0); | ^^^^^^^^^ found `malloc` symbol definition that clashes with a built-in shim | help: the `malloc` symbol is defined here --> tests/compile-fail/function_calls/exported_symbol_shim_clashing.rs:2:1 | 2 | / extern "C" fn malloc(_: usize) -> *mut std::ffi::c_void { 3 | | //~^ HELP the `malloc` symbol is defined here 4 | | unreachable!() 5 | | } | |_^ = note: inside `main` at tests/compile-fail/function_calls/exported_symbol_shim_clashing.rs:12:9 ``` This does not implement "better error messages than we do currently for arg/ABI mismatches" in #1776 (comment) -- I failed to remove all `check_arg_count()` and `check_abi()` (they are still used in `src/shims/intrinsics.rs` and `call_dlsym()`) and they don't receive the name of the shim.

Filter out items other than non-generic functions and statics in our version of `exported_symbols` [`#[no_mangle]` on a `use` item](https://docs.rs/brotli-decompressor/2.3.1/src/brotli_decompressor/ffi/mod.rs.html#3-5) can make Miri ICE when compiling a dependency (rust-lang/rust#86261): ```rs #[no_mangle] use std::{thread,panic, io, boxed, any, string}; ``` <details> ``` error: internal compiler error: compiler/rustc_middle/src/ty/mod.rs:1650:13: item_name: no name for DefPath { data: [DisambiguatedDefPathData { data: Misc, disambiguator: 14 }], krate: crate0 } thread 'rustc' panicked at 'Box<dyn Any>', compiler/rustc_errors/src/lib.rs:1007:9 stack backtrace: 0: std::panicking::begin_panic 1: std::panic::panic_any 2: rustc_errors::HandlerInner::bug 3: rustc_errors::Handler::bug 4: rustc_middle::ty::context::tls::with_opt 5: rustc_middle::util::bug::opt_span_bug_fmt 6: rustc_middle::util::bug::bug_fmt 7: rustc_middle::ty::<impl rustc_middle::ty::context::TyCtxt>::item_name 8: rustc_symbol_mangling::symbol_name_provider 9: rustc_query_impl::<impl rustc_query_system::query::config::QueryAccessors<rustc_query_impl::plumbing::QueryCtxt> for rustc_query_impl::queries::symbol_name>::compute 10: rustc_query_system::query::plumbing::get_query_impl 11: <rustc_query_impl::Queries as rustc_middle::ty::query::QueryEngine>::symbol_name 12: rustc_middle::middle::exported_symbols::ExportedSymbol::symbol_name_for_local_instance 13: rustc_codegen_ssa::back::symbol_export::symbol_name_for_instance_in_crate 14: rustc_codegen_ssa::back::linker::exported_symbols 15: <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold 16: rustc_codegen_ssa::back::linker::LinkerInfo::new 17: rustc_codegen_ssa::back::write::start_async_codegen 18: <rustc_codegen_llvm::LlvmCodegenBackend as rustc_codegen_ssa::traits::backend::CodegenBackend>::codegen_crate 19: rustc_interface::passes::QueryContext::enter 20: rustc_interface::queries::Queries::ongoing_codegen 21: rustc_interface::queries::<impl rustc_interface::interface::Compiler>::enter 22: rustc_span::with_source_map 23: rustc_interface::interface::create_compiler_and_run 24: rustc_span::with_session_globals note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace. note: the compiler unexpectedly panicked. this is a bug. note: we would appreciate a bug report: https://github.com/rust-lang/rust/issues/new?labels=C-bug%2C+I-ICE%2C+T-compiler&template=ice.md note: rustc 1.54.0-nightly (a50d72158 2021-06-08) running on x86_64-unknown-linux-gnu note: compiler flags: -C embed-bitcode=no -C debuginfo=1 --crate-type lib note: some of the compiler flags provided by cargo are hidden query stack during panic: #0 [symbol_name] computing the symbol for `{misc#14}` end of query stack ``` </details> This might be because in #1776, we override the `exported_symbols` query, and our version of `exported_symbols` can return a `use` item which don't have a name if the `use` item is tagged with `#[no_mangle]`, and then: - `rustc_codegen_ssa::back::symbol_export::symbol_name_for_instance_in_crate` is called for for every `exported_symbols`: https://github.com/rust-lang/rust/blob/fb3ea63d9b4c3e9bb90d4250b870faaffb9c8fd2/compiler/rustc_codegen_ssa/src/back/linker.rs#L1300-L1304 - it calls `rustc_middle::middle::exported_symbols::ExportedSymbol::symbol_name_for_local_instance`: https://github.com/rust-lang/rust/blob/fb3ea63d9b4c3e9bb90d4250b870faaffb9c8fd2/compiler/rustc_codegen_ssa/src/back/symbol_export.rs#L412 - which calls `rustc_symbol_mangling::symbol_name_provider`: https://github.com/rust-lang/rust/blob/fb3ea63d9b4c3e9bb90d4250b870faaffb9c8fd2/compiler/rustc_middle/src/middle/exported_symbols.rs#L37-L44 - which calls `item_name`: https://github.com/rust-lang/rust/blob/fb3ea63d9b4c3e9bb90d4250b870faaffb9c8fd2/compiler/rustc_symbol_mangling/src/lib.rs#L216, which triggers the ICE It might also be problematic for https://github.com/rust-lang/miri/blob/d39f0c64b8b369188a73a655716ab56683a6537b/src/shims/foreign_items.rs#L165 which also uses `item_name`, but Miri cannot compile the dependency, so that code can't be reached. Therefore, this PR makes `exported_symbols` filter out all items that are not functions or statics, so all items returned will have a name, which avoids the ICE (I have tested it in the https://github.com/jorgecarleitao/arrow2 repository). (This PR also includes a commit that fixes a small (unrelated) bug for `#[no_mangle]` on associated functions -- I found that because I notice `#[no_mangle]` is supported on associated functions and they should not be filtered out in `exported_symbols`.) Fixes (when the submodule is bumped) rust-lang/rust#86261.

deprecate -Zmiri-disable-abi-check This was added in #1776 but I couldn't find any discussion or motivation.

…obk,saethlin deprecate -Zmiri-disable-abi-check This was added in rust-lang/miri#1776 but I couldn't find any discussion or motivation.

ghost commented Apr 16, 2021

View reviewed changes

src/shims/foreign_items.rs Outdated Show resolved Hide resolved

ghost commented Apr 16, 2021

View reviewed changes

tests/compile-fail/non_extern_foreign_call/clashing.rs Outdated Show resolved Hide resolved

ghost commented Apr 16, 2021

View reviewed changes

src/shims/foreign_items.rs Outdated Show resolved Hide resolved

src/bin/miri.rs Outdated Show resolved Hide resolved