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

Use zerocopy to replace some unsafe code #1349

Merged
merged 1 commit into from
Nov 2, 2023

Conversation

joshlf
Copy link
Contributor

@joshlf joshlf commented Oct 29, 2023

No description provided.

Comment on lines 134 to +137
let mut buf = [0_u8; mem::size_of::<$intrinsic>()];
rng.fill_bytes(&mut buf);
// x86 is little endian so no need for conversion
// SAFETY: we know [u8; N] and $intrinsic have the same size
unsafe { mem::transmute_copy(&buf) }
zerocopy::transmute!(buf)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's another option if this would be preferred. I opted not to go this route since it messes with code that's been verified to compile to SIMD instructions (although I expect the same would work here):

let mut val = $intrinsic::new_zeroed();
// On proper hardware, this should compile to SIMD instructions
// Verified on x86 Haswell with __m128i, __m256i
//
// x86 is little endian so no need for conversion
rng.fill_bytes(&mut val.as_bytes_mut());
val

@dhardy dhardy changed the title Remove some unsafe code Use zerocopy to replace some unsafe code Oct 30, 2023
@dhardy
Copy link
Member

dhardy commented Oct 30, 2023

The key point here is the adoption of zerocopy as a dependency.

@vks
Copy link
Collaborator

vks commented Oct 30, 2023

zerocopy looks good, it's used by some quite popular crates (bitflags, ahash, uuid). It does not pull in any additional dependencies, and does not seem to affect build time significantly.

The main contender would be bytemuck, which seems a bit more popular. @joshlf Since you seem to be the author, how would you compare zerocopy to bytemuck?

@joshlf
Copy link
Contributor Author

joshlf commented Oct 30, 2023

zerocopy looks good, it's used by some quite popular crates (bitflags, ahash, uuid). It does not pull in any additional dependencies, and does not seem to affect build time significantly.

The main contender would be bytemuck, which seems a bit more popular. @joshlf Since you seem to be the author, how would you compare zerocopy to bytemuck?

With the obvious caveat that I'm biased as the author, and focusing specifically on this use case (rather than comparing the crates across all use cases), I would advocate for zerocopy in this case for a few reasons:

  • bytemuck is no longer under active development by its author (although it receives active contributions from folks in the community)
  • IIUC, bytemuck's popularity is primarily a function of its age (it's been around a lot longer than zerocopy has)
  • Zerocopy is my full-time job at Google, and I maintain it on behalf of a large number of internal Google projects
  • Zerocopy has a very high bar for soundness and correctness, as it's used in a number of high-trust projects implementing things like cryptography, security chip firmware, etc
  • For the particular use of transmute! in this PR, the bytemuck equivalents would be cast or try_cast, which are fallible and either panic or return Err at runtime on size mismatch. transmute!, by contrast, fails compilation on size mismatch.

That said, the uses introduced in this PR are pretty small, the panic paths in bytemuck's solutions might be optimized out, and there's probably not a huge difference either way.

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

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

Thanks for the discussion. Then if @vks agrees, I think we should merge this and can also close #957.

@newpavlov
Copy link
Member

In future it also may be worth to (optionally) use zerocopy in getrandom, as mentioned in this discussion.

@vks vks merged commit 870c679 into rust-random:master Nov 2, 2023
12 checks passed
@vks vks mentioned this pull request Nov 2, 2023
@joshlf joshlf deleted the remove-unsafe branch March 2, 2024 22:49
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