Dereference pointers in shims as correct types

[beepster4096]

Currently, shims will dereference pointers as the type written by the user. This can cause false positives, incorrect behavior such as #2136, and even ICEs if a field is not present.

This PR fixes this by having shims dereference pointers with types from std or libc that we can rely on the layout and field names of instead of with whatever the user passed in.

Fixes #1123

[oli-obk]

Do you have some concrete examples that failed before this PR? I have a hard time coming envisioning what this PR is actually doing.

[RalfJung]

I tens to agree that if we can avoid trusting the user types, that is better. I was just too lazy to manually define the type each time... But I like the idea of taking the type from libc or std! Did you just solve #1123 as well in this? Nice!

[oli-obk]

Instead of switching to using hardcoded types in miri, could we just verify the layout matches what we expect and keep the existing logic? Making wrong types an error instead of assuming types

[RalfJung]

For raw ptr functions, wrong types are not really an error. Conceptually we should just tell Miri to read/write things at a given fixed offset, but that quickly gets annoying -- so this PR essentially uses the libc/std types to determine that offset.

[oli-obk]

Hmm I guess that makes sense, but still sad. Would it maybe be good to nudge people with a warning?

[RalfJung]

At least on Windows people can't use the exact std type as those are private (and indeed #2136 was caused by two different call sites using two different but equally valid types).

[beepster4096]

Do you have some concrete examples that failed before this PR? I have a hard time coming envisioning what this PR is actually doing.

Here's one for windows. While I don't think anybody would actually do this, it still should work.

extern "system" {
    // this is completely legal, FILETIME has the same size and lower alignment than u64
    fn GetSystemTimeAsFileTime(out: *mut u64);
}

fn main() {
    unsafe {
        GetSystemTimeAsFileTime(&mut 0);
    }
}

[bors]

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

[RalfJung]

@drmeepster could you rebase? Now check_mplace is a public function which you can call to avoid most of the code duplication.

[beepster4096]

I noticed that almost every use of deref_operand is problematic, not just field accesses. While just reading from a pointer won't ice, it can cause false positives if the pointee type is wrong. I've changed these to use deref_operand_as too.

[bors]

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

[RalfJung]

We unfortunately had to force-push to Miri. Could you rebase this to new master? Let me know if you need help wrestling with git.

[beepster4096]

Rebase done, review comments not addressed yet.

[RalfJung]

@rustbot author

[RalfJung]

FYI I have cherry-picked the first commit from this PR into #2696, to avoid conflicts. Please rebase once #2696 lands.

[bors]

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

[beepster4096]

Its been a while, sorry. Rebased and comments addressed.

[RalfJung]

Just trying something here, please ignore.
@bors delegate+

[RalfJung]

@bors delegate-

[oli-obk]

@bors r+

[bors]

📌 Commit d537ab9 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit d537ab9 with merge 71312d6...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 71312d6 to master...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 71312d6 to master...