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

Weak Bindings Violate Strict Provenance in MIRI #262

Closed
nvzqz opened this issue Jun 5, 2022 · 7 comments · Fixed by #263
Closed

Weak Bindings Violate Strict Provenance in MIRI #262

nvzqz opened this issue Jun 5, 2022 · 7 comments · Fixed by #263
Assignees
@josephlr

Comments

@nvzqz
Copy link

nvzqz commented Jun 5, 2022

Because the Weak struct is defined as having LazyUsize for the address, it does not maintain pointer provenance and thus fails when run under MIRI with -Zmiri-strict-provenance.

The solution would be to define a LazyPtr type that utilizes AtomicPtr instead of AtomicUsize.
@josephlr
Copy link
Member

josephlr commented Jun 6, 2022

@nvzqz I notice that the AtomicPtr types basically wrap a *mut T while we usually have something of type unsafe extern "C" fn(*mut u8, libc::size_t, libc::c_uint) -> libc::c_int. I know that we can cast function pointers to/from *const (), but will that work with -Zmiri-strict-provenance?

@nvzqz
Copy link
Author

nvzqz commented Jun 6, 2022 •
edited
Loading

Yes, function pointers are tracked under strict provenance. The issue is that integers lose pointer provenance information. This same thing happened with libstd: rust-lang/rust#96167.

@josephlr
Copy link
Member

josephlr commented Jun 7, 2022

Awesome, I'll take a look!

@josephlr josephlr self-assigned this Jun 7, 2022
@josephlr
Copy link
Member

josephlr commented Jun 7, 2022

Would you also need a 0.1 backport?

@nvzqz
Copy link
Author

nvzqz commented Jun 7, 2022

I myself do not need this fixed in 0.1. This came up due to using rand, which I assume most people who hit this will also be.

josephlr added a commit that referenced this issue Jun 7, 2022
@josephlr
Use AtomicPtr instead of AtomicUsize for Weak
27f4f82 
This allows Strict Provenance to work properly, fixing #262. It also
now matches what `libstd` does:
https://github.com/rust-lang/rust/blob/9f7e997c8bc3cacd2ab4eb75e63cb5fa9279c7b0/library/std/src/sys/unix/weak.rs#L85-L141

Also, while reading the `libstd` code, I noticed that they use an
`Acquire` fence and `Release` store as the returned pointer should
have "consume" semantics. I changed our code to do something
slightly stronger (Acquire load and Release store) for consistancy.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr mentioned this issue Jun 7, 2022
Merged
josephlr added a commit that referenced this issue Jun 7, 2022
@josephlr
Use AtomicPtr instead of AtomicUsize for Weak
214a4a2 
This allows Strict Provenance to work properly, fixing #262. It also
now matches what `libstd` does:
https://github.com/rust-lang/rust/blob/9f7e997c8bc3cacd2ab4eb75e63cb5fa9279c7b0/library/std/src/sys/unix/weak.rs#L85-L141

Also, while reading the `libstd` code, I noticed that they use an
`Acquire` fence and `Release` store as the returned pointer should
have "consume" semantics. I changed our code to do something
slightly stronger (Acquire load and Release store) for consistancy.

Signed-off-by: Joe Richey <joerichey@google.com>
josephlr added a commit that referenced this issue Jun 7, 2022
@josephlr
Use AtomicPtr instead of AtomicUsize for Weak
0498e88 
This allows Strict Provenance to work properly, fixing #262. It also
now matches what `libstd` does:
https://github.com/rust-lang/rust/blob/9f7e997c8bc3cacd2ab4eb75e63cb5fa9279c7b0/library/std/src/sys/unix/weak.rs#L85-L141

Also, while reading the `libstd` code, I noticed that they use an
`Acquire` fence and `Release` store as the returned pointer should
have "consume" semantics. I changed our code to do something
slightly stronger (Acquire load and Release store) for consistancy.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr
Copy link
Member

josephlr commented Jun 7, 2022

It looks like we will have to use 1 as *mut c_void in our code for stuff to properly compile on 1.34. Will that work with Strict Provenance provided that we never deference the invalid pointer?

@nvzqz
Copy link
Author

nvzqz commented Jun 7, 2022 •
edited
Loading

My understanding is that's totally fine. Considering that sptr::invalid_mut does exactly this, I don't see an issue.

josephlr added a commit that referenced this issue Jun 13, 2022
@josephlr @fbstj
Use AtomicPtr instead of AtomicUsize for Weak (#263)
c82a522 
This allows Strict Provenance to work properly, fixing #262. It also
now matches what `libstd` does:
https://github.com/rust-lang/rust/blob/9f7e997c8bc3cacd2ab4eb75e63cb5fa9279c7b0/library/std/src/sys/unix/weak.rs#L85-L141

Also, while reading the `libstd` code, I noticed that they use an
`Acquire` fence and `Release` store as the returned pointer should
have "consume" semantics. As this doesn't yet exist in Rust, we
instead do exactly what `libstd` does, which means:
  - Relaxed Load
  - Release Store
  - Acquire fence when returning pointer

Signed-off-by: Joe Richey <joerichey@google.com>

Co-authored-by: Joe ST <joe@fbstj.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@josephlr josephlr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use AtomicPtr instead of AtomicUsize for Weak rust-random/getrandom
2 participants
@josephlr @nvzqz