Add ABI check for shims

[unseddd]

Resolves #1631

[RalfJung]

Thanks for the PR, @unseddd! However, we must have miscommunicated somewhere. This is not the check that I meant. In fact I think this check is redundant, since the same check is already done by the Miri core engine.

I probably should give an example for what I mean. Consider this code:

fn main() {
    extern "Rust" {
        fn malloc(size: usize) -> *mut std::ffi::c_void;
    }

    unsafe {
        let _ = malloc(0);
    };
}

This code is wrong, because it calls malloc with the "Rust" call ABI, when it should be "C". However, Miri accepts this code! #1631 is about adjusting Miri to reject this code. The problem does not just affect malloc, it affects every single extern function that Miri has a "shim" for, i.e., that is supported by Miri.

I hope it's clearer now, and sorry for not giving a concrete example earlier. :) I will add this to the issue description as well.

[unseddd]

it calls malloc with the "Rust" call ABI, when it should be "C". However, Miri accepts this code! #1631 is about adjusting Miri to reject this code.

Ok, think I understand now. Honestly wasn't sure if the check(s) I PRed were correct in this context, so thanks for clarifying.

Would it be correct to just do something simple like:

if abi != Abi::C {
    return err...;
}

[unseddd]

@RalfJung I changed the ABI check, and rebased on latest master. Let me know what you think.

With the update, looks like a test(s) is now failing. Is this expected? Should I fix the test, or did I screw something up?

[RalfJung]

Sorry for the delay; I am quite busy currently so it could take a bit until I can take a closer look at what you did. It's on my queue though and will not get lost!

Would it be correct to just do something simple like

Well, the question is where you meant to put that check. The ABI might not be the same for all the shims we have. It probably is Abi::C for most of them, but some functions will have other ABIs. Probably what makes most sense is to first handle the other ABIs, and then use the existing code for the C ABI.

[RalfJung]

With the update, looks like a test(s) is now failing. Is this expected? Should I fix the test, or did I screw something up?

That's a bit hard to tell, which function is it talking about? If it is __rust_begin_short_backtrace, then that function does have the Rust ABI, not the C ABI -- so looks like the check is wrong here.

[unseddd]

Sorry for the delay; I am quite busy currently so it could take a bit until I can take a closer look at what you did. It's on my queue though and will not get lost!

No worries, thanks for letting me know. No intention of being a nuisance 😄

If it is __rust_begin_short_backtrace, then that function does have the Rust ABI, not the C ABI -- so looks like the check is wrong here

I'll look into it more, then. Still need to figure out all the callers of the shim find_mir_or_eval_fn. From your earlier comment, I thought these would all be extern "C" calls, but maybe that's only a subset? Anyway, thanks for the feedback, will keep working on this.

[RalfJung]

Still need to figure out all the callers of the shim find_mir_or_eval_fn.

There seems to be some confusion about terminology here, specifically about the term "shim". I should explain what we mean by that. :)

find_mir_or_eval_fn is called for each function call. Usually it looks up the MIR of the function, but for some function, there's no MIR -- in particular, for extern-imported functions. Generally Miri will just error, saying that it cannot call functions for which there is no MIR available. However, for some specific of these functions, we have implemented "shims" by hand that implement the intended behavior by directly manipulating interpreter state.

The main entry point you have to worry about is this:

miri/src/shims/foreign_items.rs

Line 112 in 903bfd8

fn emulate_foreign_item(

[unseddd]

find_mir_or_eval_fn is called for each function call. ... The main entry point you have to worry about is this

Thanks for pointing me in the right direction, that solved the failing test 😄

Sorry for the notification spam in the related issue (forgot about the commit message). This PR should be ready for review when you are.

[RalfJung]

This function here should also check the ABI similar to what you did for foreign_items:

miri/src/shims/dlsym.rs

Line 29 in 6fdda8a

fn call_dlsym(

[RalfJung]

I just added another architecture-specific function here; can you rebase and also add the check_abi here?

[RalfJung]

Looking great. :) Please squash the commits together, then we can merge this.

(Heads-up, there might be conflicts with me resolving rust-lang/rust#81341... sorry for that.)

[bors]

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

[unseddd]

Looking great. :) Please squash the commits together, then we can merge this.

All done 😄

(Heads-up, there might be conflicts with me resolving rust-lang/rust#81341... sorry for that.)

No worries, was an easy fix.

[RalfJung]

@unseddd thanks a lot for the PR and for patiently taking care of all my comments. :)
@bors r+

[bors]

📌 Commit de4eea9 has been approved by RalfJung

[bors]

⌛ Testing commit de4eea9 with merge b38dc83...

[bors]

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

[unseddd]

@unseddd thanks a lot for the PR and for patiently taking care of all my comments. :)

No problem 😄

Thanks for being patient with me during the learning process ❤️