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

Improve error message of calling unsupported non-"C"/"system"-ABI foreign function #1745

Merged
merged 5 commits into from Mar 17, 2021
Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 17, 2021

Miri currently reports the following foo() call has ABI-mismatch UB:

#[cfg(unix)]
extern "Rust" { // or any non-"C" ABI
    fn foo();
}

#[cfg(windows)]
extern "C" { // or any non-"system" ABI
    fn foo();
}

fn main() {
    unsafe {
        foo();
    }
}

Output when targeting Linux (and maybe also macOS):

error: Undefined Behavior: calling a function with ABI C using caller ABI Rust
  --> src/main.rs:13:9
   |
13 |         foo();
   |         ^^^^^ calling a function with ABI C using caller ABI Rust
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavio

Output when targeting Windows:

error: Undefined Behavior: calling a function with ABI system using caller ABI C
  --> <anon>:13:9
   |
13 |         foo();
   |         ^^^^^ calling a function with ABI system using caller ABI C
   |
   = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior

However, to my knowledge, that's not UB -- it's just unsupported by Miri (and Miri can't assume the function has "C" or "system" ABI since Miri doesn't know about it). I believe that is because of the overzealous check_abi() call before the long match in src/shims/{posix,windows}/foreign_items.rs. The ABI is checked to match the system one ("system" on Windows, "C" otherwise) no matter the callee is recognized as a shim or an unsupported foreign function.

Therefore, this PR removes the check_abi() call before the match and inserts a check_abi() call to each non-wildcard match.

src/helpers.rs Outdated Show resolved Hide resolved
src/shims/foreign_items.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

Thanks a lot, looking good. :-)
@bors r+

@bors
Copy link
Contributor

bors commented Mar 17, 2021

📌 Commit 7ec919d has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Mar 17, 2021

⌛ Testing commit 7ec919d with merge 80d6b56...

@bors
Copy link
Contributor

bors commented Mar 17, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 80d6b56 to master...

@bors bors merged commit 80d6b56 into rust-lang:master Mar 17, 2021
@ghost ghost deleted the unsup-foreign-calls-are-not-ub branch March 17, 2021 18:10
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2021
Use `C-unwind` ABI for `__rust_start_panic` in `panic_abort`

The function originally has `C` ABI but is called using `C-unwind` ABI in `std`:
https://github.com/rust-lang/rust/blob/d4ad1cfc63ba5824196bfb2370451ddb5af2e020/library/std/src/panicking.rs#L49-L54
Which might be [problematic](rust-lang/miri#1745 (comment)) and triggers this [Miri error](rust-lang#87778 (comment)):
```
error: Undefined Behavior: calling a function with ABI C using caller ABI C-unwind
   --> /home/hyd-dev/.rustup/toolchains/miri/lib/rustlib/src/rust/library/std/src/panicking.rs:672:9
    |
672 |         __rust_start_panic(obj)
    |         ^^^^^^^^^^^^^^^^^^^^^^^ calling a function with ABI C using caller ABI C-unwind
    |
    = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
    = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
```
Changing the ABI of the function to `C-unwind` fixes the error above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants