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

Unsafe is no longer necessary as of rustc 1.34 #488

Closed
Gaelan opened this issue Aug 21, 2020 · 2 comments · Fixed by #507
Closed

Unsafe is no longer necessary as of rustc 1.34 #488

Gaelan opened this issue Aug 21, 2020 · 2 comments · Fixed by #507

Comments

@Gaelan
Copy link
Contributor

Gaelan commented Aug 21, 2020

This code (and another identical line in the same file) uses unsafe to convert a &[u8] to a &[u8; 8]:

uuid/src/lib.rs

Line 360 in f177d20

unsafe { &*(self.as_bytes()[8..16].as_ptr() as *const [u8; 8]) };

From Rust 1.34 and on (before that, TryFrom wasn't stable), the following code compiles to the exact same assembly as the code above:

self.as_bytes()[8..16].try_into().unwrap()

Tested with rustc 1.34 and 1.45.2 on godbolt.org, with -O.

The try_into version is simpler and doesn't use unsafe, so it'd probably be a good idea to switch to that once support for Rust 1.32/33 is dropped.

@kinggoesgaming
Copy link
Member

Our current MSRV is 1.32.0 😞

However I will keep the issue open so we can update the relevant code when we get to it

@KodrAus
Copy link
Member

KodrAus commented Dec 21, 2020

We've just bumped our MSRV to 1.34.0 in #501 with an update to getrandom.

Would anybody like to submit a PR for this?

Gaelan added a commit to Gaelan/uuid that referenced this issue Dec 21, 2020
Previously, to_fields and to_fields_le used unsafe to convert a
&[u8] into a &[u8; 8]. Now that we're only supporting Rust
versions where TryInto is stable, we can use try_into().unwrap()
instead, making uuid entirely safe Rust.

In release mode, the compiler detects that the slice will always
be the correct size, so try_into can never fail. Thus, the unwrap
is optimized out and we end up with the exact same assembly
as the unsafe block.

Godbolt output showing the resulting assembly:
https://godbolt.org/z/nWxT6W

Closes uuid-rs#488.
bors bot added a commit that referenced this issue Jan 17, 2021
507: Use TryInto to avoid unsafe. r=kinggoesgaming a=Gaelan

<!--
    If this PR is a breaking change, ensure that you are opening it against 
    the `breaking` branch.  If the pull request is incomplete, prepend the Title with WIP: 
-->

**I'm submitting a(n)** refactor

# Description

Previously, to_fields and to_fields_le used unsafe to convert a &[u8] into a &[u8; 8]. Now that we're only supporting Rust versions where TryInto is stable, we can use try_into().unwrap() instead, making uuid entirely safe Rust.

In release mode, the compiler detects that the slice will always be the correct size, so try_into can never fail. Thus, the unwrap
is optimized out and we end up with the exact same assembly as the unsafe block.

Godbolt output showing the resulting assembly: https://godbolt.org/z/nWxT6W

# Motivation

Makes UUID entirely safe Rust.

With this PR,

# Tests

All existing tests pass. Doesn't add any new functionality, so that should be sufficient. Assuming the Godbolt test is consistent with what happens in the context of the larger crate, this shouldn't change the resulting binary at all.

# Related Issue(s)

Closes #488.


Co-authored-by: Gaelan Steele <gbs@canishe.com>
@bors bors bot closed this as completed in 0550c21 Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants