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

do not allow ABI mismatches inside repr(C) types #119037

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

RalfJung
Copy link
Member

In #115476 we allowed ABI mismatches inside repr(C) types. This wasn't really discussed much; I added it because from how I understand calling conventions, this should actually be safe in practice. However I entirely forgot to actually allow this in Miri, and in the mean time I have learned that too much ABI compatibility can be a problem for CFI (it can reject fewer calls so that gives an attacker more room to play with).

So I propose we take back that part about ABI compatibility in repr(C). It is anyway something that C and C++ do not allow, as far as I understand.

In the future we might want to introduce a class of ABI compatibilities where we say "this is a bug and it may lead to aborting the process, but it won't lead to arbitrary misbehavior -- worst case it'll just transmute the arguments from the caller type to the callee type". That would give CFI leeway to reject such calls without introducing the risk of arbitrary UB. (The UB can still happen if the transmute leads to bad results, of course, but it wouldn't be due to ABI weirdness.)

#115476 hasn't reached beta yet so if we land this before Dec 22nd we can just pretend this all never happened. ;) Otherwise we should do a beta backport (of the docs change at least).

Cc @rust-lang/opsem @rust-lang/types

@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot 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 Dec 17, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 17, 2023

The Miri subtree was changed

cc @rust-lang/miri

@Mark-Simulacrum Mark-Simulacrum added the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 17, 2023
@Mark-Simulacrum
Copy link
Member

Nominating due to the relatively tight timeline for T-lang at least.

@RalfJung RalfJung added the P-high High priority label Dec 19, 2023
@scottmcm
Copy link
Member

We had a brief discussion in the (sparsely-attended) lang meeting today.

Everyone agreed that if there's concerns about a guarantee, we might as well retract it for now since we can always add it back again later.

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 20, 2023

📌 Commit c7e3b3f has been approved by scottmcm

It is now in the queue for this repository.

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 20, 2023
@bors
Copy link
Contributor

bors commented Dec 20, 2023

⌛ Testing commit c7e3b3f with merge 5ac4c8a...

@bors
Copy link
Contributor

bors commented Dec 20, 2023

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 5ac4c8a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 20, 2023
@bors bors merged commit 5ac4c8a into rust-lang:master Dec 20, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 20, 2023
@traviscross traviscross removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Dec 20, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5ac4c8a): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
1.1% [1.1%, 1.1%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-0.2% [-0.2%, -0.2%] 1
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-3.5%, -2.1%] 2
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.9% [2.9%, 2.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.0% [0.0%, 0.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Bootstrap: 672.918s -> 672.809s (-0.02%)
Artifact size: 312.81 MiB -> 312.78 MiB (-0.01%)

@RalfJung
Copy link
Member Author

RalfJung commented Dec 20, 2023 via email

@RalfJung RalfJung deleted the repr-c-abi-mismatch branch December 22, 2023 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. P-high High priority S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants