transmute: caution against int2ptr transmutation #122379

RalfJung · 2024-03-12T12:30:00Z

This came up in #121282.
Cc @saethlin @scottmcm

Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

rustbot · 2024-03-12T12:30:08Z

r? @m-ou-se

rustbot has assigned @m-ou-se.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

library/core/src/intrinsics.rs

m-ou-se

r=me

RalfJung · 2024-03-12T13:07:50Z

@scottmcm should this go through t-lang FCP?

saethlin · 2024-03-12T14:18:05Z

I would prefer to see something here that indicates that it is specifically not possible to round-trip pointers through transmuting to an integer type and back, because that is the pattern that people do in the wild.

workingjubilee · 2024-03-12T16:14:39Z

I would consider this an inevitable conclusion from the Provenance RFC's acceptance (i.e. not requiring FCP).

RalfJung · 2024-03-12T16:23:48Z

I would prefer to see something here that indicates that it is specifically not possible to round-trip pointers through transmuting to an integer type and back, because that is the pattern that people do in the wild.

This is currently implied by:

Doing non-zero-sized memory accesses with a pointer constructed this way [i.e., via transmutation] is currently considered undefined behavior.

Are you saying that roundtrips should be mentioned more explicitly? Sure, I can do that.

RalfJung · 2024-03-12T16:28:39Z

I would consider this an inevitable conclusion from the Provenance RFC's acceptance.

We could declare that ptr2int transmute "exposes" the pointer and int2ptr transmute makes it pick up an exposed provenance similar to as casts. This isn't really practical for union field accesses or raw pointer loads, but for transmute we know pretty well what happens and could add a special hack to the semantics.

I'd prefer if we didn't do that, though.

workingjubilee · 2024-03-12T16:36:21Z

If "transmute is semantically equivalent to a bitwise move of one type into another", then how would we rationalize the operation being different from a pointer load without changing the definition of transmute?

RalfJung · 2024-03-12T16:51:35Z

We'd rationalize this as a hack in the transmute intrinsic that is intended to keep old code working -- code that made wrong assumptions about the Rust memory model at a time when there were no reliable docs about which assumptions one could make.

transmute() would not be equivalent to "do a raw ptr store at one type and then raw ptr load at a different type" any more.

workingjubilee · 2024-03-12T16:54:28Z

Ah, we'd burn down the performance of Rust programs because they did undefined behavior! I see, I see.

Wait, what?

RalfJung · 2024-03-12T17:15:28Z

I don't think this would impact the performance of any "good" programs. After all with transmute we know exactly from which type to which type we are promoting.

I didn't say it's an option I like, I just said it's a possible option and the docs added here are not an inevitable conclusion of Rust having provenance on pointers but not integers.

workingjubilee · 2024-03-12T17:25:52Z

I suppose I can concede that much, and will simply say that I would strongly prefer we not seriously entertain any ideas that are technically valid but involve deeply undermining an understanding of how programs will be compiled.

saethlin · 2024-03-12T17:33:18Z

I believe Ralf is referring to reverting #121282. Before that PR, usize as ptr and transmute::<usize, ptr> have the same lowering. Such a change would primarily impact the codegen of programs using ptr::without_provenance, but we could always recoup that by resurrecting #121242 (which special-cases that function instead of giving it the naive transmute implementation).

scottmcm · 2024-03-13T15:10:30Z

should this go through t-lang FCP?

I brought this up briefly in lang triage today, @RalfJung. Meeting consensus was that we don't need checkboxes for it. If you/opsem are happy with the description, we're happy to land stuff since the RFC about having provenance was approved.

RalfJung · 2024-03-13T15:39:00Z

Okay let's do t-opsem FCP then.

@rustbot fcp merge

RalfJung · 2024-03-13T15:39:18Z

Oh, wrong bot.
@rfcbot fcp merge

rfcbot · 2024-03-13T15:39:20Z

Team member @RalfJung has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot · 2024-03-23T16:00:59Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

RalfJung · 2024-03-23T16:04:30Z

@bors r=m-ou-se rollup
(source)

bors · 2024-03-23T16:04:33Z

📌 Commit f4adb1e has been approved by m-ou-se

It is now in the queue for this repository.

transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc `@saethlin` `@scottmcm` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122895 (add some ice tests 5xxxx to 9xxxx) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122943 (add a couple more ice tests) - rust-lang#122952 (Miri subtree update) r? `@ghost` `@rustbot` modify labels: rollup

transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc ``@saethlin`` ``@scottmcm`` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

…iaskrgr Rollup of 8 pull requests Successful merges: - rust-lang#116016 (Soft-destabilize `RustcEncodable` & `RustcDecodable`, remove from prelude in next edition) - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#122168 (Fix validation on substituted callee bodies in MIR inliner) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122943 (add a couple more ice tests) r? `@ghost` `@rustbot` modify labels: rollup

transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc ````@saethlin```` ````@scottmcm```` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

…kingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122460 (Rework rmake support library API) - rust-lang#122797 (Fix compile of wasm64-unknown-unknown target) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122895 (add some ice tests 5xxxx to 9xxxx) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#122168 (Fix validation on substituted callee bodies in MIR inliner) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122840 (`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122943 (add a couple more ice tests) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc `````@saethlin````` `````@scottmcm````` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

Rollup merge of rust-lang#122379 - RalfJung:int2ptr-transmute, r=m-ou-se transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc ```@saethlin``` ```@scottmcm``` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

transmute: caution against int2ptr transmutation This came up in rust-lang#121282. Cc ```@saethlin``` ```@scottmcm``` Eventually we'll add a proper description of provenance that we can reference, but that's a bunch of work and it's unclear who will have the time to do that when. Meanwhile, let's at least do what we can without mentioning provenance explicitly.

…iaskrgr Rollup of 9 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#122168 (Fix validation on substituted callee bodies in MIR inliner) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122840 (`rustdoc --test`: Prevent reaching the maximum size of command-line by using files for arguments if there are too many) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122943 (add a couple more ice tests) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned m-ou-se Mar 12, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 12, 2024

m-ou-se reviewed Mar 12, 2024

View reviewed changes

library/core/src/intrinsics.rs Outdated Show resolved Hide resolved

m-ou-se approved these changes Mar 12, 2024

View reviewed changes

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 12, 2024

RalfJung force-pushed the int2ptr-transmute branch from be28971 to 90a5a6c Compare March 12, 2024 13:08

transmute: caution against int2ptr transmutation

d3299af

RalfJung force-pushed the int2ptr-transmute branch from 90a5a6c to d3299af Compare March 12, 2024 16:26

saethlin approved these changes Mar 12, 2024

View reviewed changes

RalfJung added T-opsem Relevant to the opsem team and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 13, 2024

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Mar 13, 2024

RalfJung force-pushed the int2ptr-transmute branch from 474f071 to f4adb1e Compare March 19, 2024 06:19

rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Mar 23, 2024

rfcbot added the to-announce Announce this issue on triage meeting label Mar 23, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2024

matthiaskrgr mentioned this pull request Mar 23, 2024

Rollup of 6 pull requests #122953

Closed

matthiaskrgr mentioned this pull request Mar 23, 2024

Rollup of 8 pull requests #122959

Closed

matthiaskrgr mentioned this pull request Mar 24, 2024

Rollup of 9 pull requests #122966

Merged

workingjubilee mentioned this pull request Mar 24, 2024

Rollup of 13 pull requests #122967

Closed

workingjubilee mentioned this pull request Mar 24, 2024

Rollup of 14 pull requests #122973

Closed

bors merged commit 2dcc968 into rust-lang:master Mar 24, 2024
11 checks passed

rustbot added this to the 1.79.0 milestone Mar 24, 2024

RalfJung deleted the int2ptr-transmute branch March 24, 2024 08:29

apiraino removed the to-announce Announce this issue on triage meeting label Apr 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

transmute: caution against int2ptr transmutation #122379

transmute: caution against int2ptr transmutation #122379

RalfJung commented Mar 12, 2024 •

edited

rustbot commented Mar 12, 2024

m-ou-se left a comment

RalfJung commented Mar 12, 2024

saethlin commented Mar 12, 2024 •

edited

workingjubilee commented Mar 12, 2024 •

edited

RalfJung commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

saethlin commented Mar 12, 2024

scottmcm commented Mar 13, 2024

RalfJung commented Mar 13, 2024

RalfJung commented Mar 13, 2024

rfcbot commented Mar 13, 2024 •

edited by JakobDegen

rfcbot commented Mar 23, 2024

RalfJung commented Mar 23, 2024

bors commented Mar 23, 2024

transmute: caution against int2ptr transmutation #122379

transmute: caution against int2ptr transmutation #122379

Conversation

RalfJung commented Mar 12, 2024 • edited

rustbot commented Mar 12, 2024

m-ou-se left a comment

Choose a reason for hiding this comment

RalfJung commented Mar 12, 2024

saethlin commented Mar 12, 2024 • edited

workingjubilee commented Mar 12, 2024 • edited

RalfJung commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

RalfJung commented Mar 12, 2024

workingjubilee commented Mar 12, 2024

saethlin commented Mar 12, 2024

scottmcm commented Mar 13, 2024

RalfJung commented Mar 13, 2024

RalfJung commented Mar 13, 2024

rfcbot commented Mar 13, 2024 • edited by JakobDegen

rfcbot commented Mar 23, 2024

RalfJung commented Mar 23, 2024

bors commented Mar 23, 2024

RalfJung commented Mar 12, 2024 •

edited

saethlin commented Mar 12, 2024 •

edited

workingjubilee commented Mar 12, 2024 •

edited

rfcbot commented Mar 13, 2024 •

edited by JakobDegen