Skip to content

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

Merged
bors merged 1 commit intorust-lang:masterfrom
RalfJung:repr-c-abi-mismatch
Dec 20, 2023
Merged

do not allow ABI mismatches inside repr(C) types#119037
bors merged 1 commit intorust-lang:masterfrom
RalfJung:repr-c-abi-mismatch

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

Loading
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