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

regression: transmute_copy validates sizing #100545

Closed
Mark-Simulacrum opened this issue Aug 14, 2022 · 6 comments
Closed

regression: transmute_copy validates sizing #100545

Mark-Simulacrum opened this issue Aug 14, 2022 · 6 comments
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

transmute_copy now validates reasonable sizes and panics if not as of #98839, this causes some breakage (detected at the time in #98839 (comment)).

Mostly filing this issue to make sure we add a release notes compat note about this, but I don't expect us to do anything further. Nominating for T-libs-api, though, mostly just for awareness.

cc @tmandry / @joshtriplett -- let's make sure this makes it into 1.64 release notes

@Mark-Simulacrum Mark-Simulacrum added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. I-libs-api-nominated Nominated for discussion during a libs-api team meeting. labels Aug 14, 2022
@Mark-Simulacrum Mark-Simulacrum added this to the 1.64.0 milestone Aug 14, 2022
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 14, 2022
@Mark-Simulacrum Mark-Simulacrum removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Aug 14, 2022
@estebank
Copy link
Contributor

estebank commented Aug 15, 2022

I believe this is the same as #99923

Edit: I meant "the same category as".

@Mark-Simulacrum
Copy link
Member Author

Seems like a much easier issue to explain, at least - not sure I would quite call it "same as" pointer/integer transmutes :) Though the overall effect of breaking unsafe code seems quite similar.

@RalfJung
Copy link
Member

I believe this is the same as #99923

The issues seem entirely unrelated to me, in which sense are they "the same"?

@RalfJung
Copy link
Member

Or did you mean they are broadly the same category, being related to UB detection? But even then, #99923 is about compile-time UB, and this here is primarily about run-time UB, and those are very different in how we treat them.

@5225225
Copy link
Contributor

5225225 commented Aug 15, 2022

Seems like a much easier issue to explain, at least - not sure I would quite call it "same as" pointer/integer transmutes :)

The easiest justification for the panic is "transmute_copy has a safety precondition that U, the return type, is not larger than T, the input type, this can be checked with zero runtime cost, so a panic was inserted in the UB case". As in, "it's UB because we wrote that it was UB" (it would be an out of bounds read internally, is the main reason it's marked as UB)

Maybe also mention RUST_BACKTRACE=1 since the panic location will point to the stdlib internals, not the caller. As I understand it, track_caller would make it non-zero cost for a function that's UB to panic anyways, so I didn't add it. (It does mean if you don't have gdb or backtraces, you're going to have a fun time finding the panic. We do indicate what the caller did wrong, but not where the caller is).

@RalfJung
Copy link
Member

Yeah, we could have a cfg_attr(debug_assertions, track_caller) but that would only help once we have a good system for letting people use a debug build of the standard library...

@m-ou-se m-ou-se removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Sep 13, 2022
@dtolnay dtolnay added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 13, 2022
@m-ou-se m-ou-se closed this as completed Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-from-stable-to-beta Performance or correctness regression from stable to beta. relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants