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

Fix unsound transmute_copy use #1

Closed
wants to merge 1 commit into from

Conversation

5225225
Copy link

@5225225 5225225 commented Jul 8, 2022

The transmute_copies here only are allowed to read one byte, since you're passing a &u8, so T is u8, and the docs on transmute_copy says

This function triggers undefined behavior if U is larger than T.

In rust-lang/rust#98839 , some checks are being added to enforce this with a runtime panic, and this crate is one of the ones that ran into that.

You should instead use raw pointers and read_unaligned.

Also, I'd recommend running this crate through https://github.com/rust-lang/miri , it's an excellent tool to find issues like this. There's some use of an uninit variable that it errors on.

I can't find any obvious signs of where that's coming from, but I'd be happy to help get this crate passing under miri.

@AlecGoncharow
Copy link
Owner

AlecGoncharow commented Jul 25, 2022

I managed to miss the notification for this.

This is interesting, thanks!

I am not sure how useful it is to update this code, as it shouldn't really be used by anything.

Once I have some time I will look into back-porting some changes from the spiritual successor to this and incorporating your recommendation, that lives in https://github.com/AlecGoncharow/zeus-rs/tree/main/hermes

I am curious how this got picked up in the scan, not familiar with how that process works but I'd imagine I have other code that violates this, especially in that linked hermes crate.

@5225225
Copy link
Author

5225225 commented Jul 25, 2022

I am not sure how useful it is to update this code, as it shouldn't really be used by anything.

No worries, I figured I might as well make at least a PR for it so that if you ever wanted to run this on an updated rustc, the work to fix it has already been done.

I am curious how this got picked up in the scan, not familiar with how that process works but I'd imagine I have other cod that violates this, especially in that linked hermes crate.

I'm not too familiar with the way crater finds github repos, but I honestly wouldn't be surprised if it's going "right, let's do a language:rust search across all of github and look for a Cargo.toml and stick that in a big list".

As long as it has tests and is on github I don't see why it can't be found.

The change to make this panic is already in nightly, so if you use an up to date rustc, and your code breaks that, you'll find out with a panic :P

But all in all, it was rather rare to see code that breaks this. I think there were four regressions in total, only one of which is in code people actually still use?


On a side note, I have a pending crater run to make the mem::uninitialized checks stricter, and I'm expecting a lot more regressions to go through :) Though, if I did my job right in making the PR, hopefully not so many that we can't land that change :)

That has nothing to do with this PR, but rust-lang/rust#99389 if you're curious

AlecGoncharow added a commit to AlecGoncharow/zeus-rs that referenced this pull request Mar 26, 2023
AlecGoncharow added a commit to AlecGoncharow/zeus-rs that referenced this pull request Mar 26, 2023
@5225225
Copy link
Author

5225225 commented Feb 17, 2024

Seems like this was fixed going off the backreferences

@5225225 5225225 closed this Feb 17, 2024
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.

2 participants