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

Make weak syscalls in std work. #1629

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Nov 18, 2020

Since rust-lang/rust#78785, std looks up getrandom and statx with dlsym before attempting to use syscall(SYS_.., ..).

It also now passes all arguments as a machine-sized word, instead of their original types.

std now looks up `getrandom` and `statx` with `dlsym` before attempting
to use `syscall(SYS_.., ..)`.

It also now passes all arguments as a machine-sized word, instead of
their original types.
@RalfJung
Copy link
Member

Fixes rust-lang/rust#79176.

I removed that. That issue should only be closed when the submodule got bumped.

this.read_scalar(flags)?.to_i32()?;
let flags = this.read_scalar(flags)?;
// Either `i32` or `isize` is fine.
if flags.to_machine_isize(this).is_err() {
Copy link
Member

@RalfJung RalfJung Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is extremely suspicious, I will have to think about what to do here. is_err is never okay; Miri should not catch "exceptions" (that leads to all sots of problems).

Copy link
Contributor

@oli-obk oli-obk Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, I think the right fix is to check flags.ty.kind() and then choose a different to_* method

edit: ah, saw your other PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I need to become convinced that this is not UB.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variadic functions like this, especially syscall, are pretty annoying. For the ABI it doesn't matter if you pass a char or int or long or a pointer, those just all count as 'one argument' or 'one register'. But unless miri is going to emulate the actual processor registers to simulate the ABI, this is always going to be a bit hacky. :( (The futex implementation had the problem with accepting an integer for the pointer argument.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the C standard defines this behaviour, but the syscall wrapper isn't a C function. It's written in assembly and fully depends on knowing everything about the ABI.

Copy link
Member

@RalfJung RalfJung Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The futex implementation had the problem with accepting an integer for the pointer argument.)

Those are the same size, so they are the same ABI even as far as Rust's Layout/ABI code is concerned.

This is different. I spent a lot of time making sure Miri detects ABI mismatches during function calls, and I'd consider different sizes to be ABI mismatches. You can't even transmute these!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the C standard defines this behaviour, but the syscall wrapper isn't a C function. It's written in assembly and fully depends on knowing everything about the ABI.

It is called with the "C" ABI though, so its rules apply.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also note that Miri's getrandom function is also used to shim the getrandom libc function, not just direct calls to the syscall.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if the C standard defines this behaviour, but the syscall wrapper isn't a C function. It's written in assembly and fully depends on knowing everything about the ABI.

It is called with the "C" ABI though, so its rules apply.

@RalfJung From what little I know of the x86_64 "C" ABI, you can call an extern "C" fn(u64) as if it was an extern "C" fn(u8) because in both cases the argument is passed in the same 64-bit register[0]. I think that is what @m-ou-se meant by "fully depends on knowing everything about the ABI."

[0]: See e.g. https://godbolt.org/z/a35MYb, keeping in mind that edi (resp. eax) is an alias to the low 32 bits of rdi (resp. rax).

Copy link
Member

@RalfJung RalfJung Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can call an extern "C" fn(u64) as if it was an extern "C" fn(u8) because in both cases the argument is passed in the same 64-bit register[0]

And which 64-bit value will the function see then? Maybe the compiler passes some junk in the higher bytes?

Also, the code we are looking at in rustc runs on many targets. So just looking at the x86_64 ABI does not help.

@m-ou-se
Copy link
Member Author

m-ou-se commented Nov 19, 2020

Oh, right. Submodules. :)

Thanks.

@RalfJung RalfJung mentioned this pull request Nov 19, 2020
@RalfJung
Copy link
Member

Two things broke at once, so I had to fix them together in #1630. I made your PR part of that. Thanks for helping fixing Miri!

@RalfJung RalfJung closed this Nov 19, 2020
bors added a commit that referenced this pull request Nov 19, 2020
@m-ou-se m-ou-se deleted the fix-std-weak-syscall branch November 19, 2020 17:08
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