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.

Sign up for GitHub

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

Jump to bottom

Make weak syscalls in std work. #1629

Closed
wants to merge 1 commit into from
Closed

Make weak syscalls in std work. #1629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/shims/posix/linux/dlsym.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ impl Dlsym {
pub fn from_str(name: &str) -> InterpResult<'static, Option<Dlsym>> {
Ok(match &*name {
"__pthread_get_minstack" => None,
"getrandom" => None, // std falls back to syscall(SYS_getrandom, ...) when this is NULL.
"statx" => None, // std falls back to syscall(SYS_statx, ...) when this is NULL.
_ => throw_unsup_format!("unsupported Linux dlsym: {}", name),
})
}
Expand Down
6 changes: 5 additions & 1 deletion src/shims/posix/linux/foreign_items.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,11 @@ fn getrandom<'tcx>(

// The only supported flags are GRND_RANDOM and GRND_NONBLOCK,
// neither of which have any effect on our current PRNG.
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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.

Copy link

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 •
edited
Loading

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.

flags.to_i32()?;
}

this.gen_random(ptr, len)?;
this.write_scalar(Scalar::from_machine_usize(len, this), dest)?;
Expand Down