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

Decide on the validity of function pointers #440

Closed
RalfJung opened this issue Aug 4, 2023 · 9 comments
Closed

Decide on the validity of function pointers #440

RalfJung opened this issue Aug 4, 2023 · 9 comments

Comments

@RalfJung
Copy link
Member

RalfJung commented Aug 4, 2023

We decide

that the validity invariant for function pointers is: they need to be non-null.

Transmuting any provenance-free non-null input to a function pointer is definitely allowed without causing immediate UB.
When and whether these function pointers can actually be invoked is a different question and not answered here.

We do not decide what happens when the input has provenance. This is tracked here. In particular, values such as &0 (that have provenance) might or might not be legal to transmute to function pointers.

Rationale

The non-null requirement is already out there since forever and plenty of code relies on Option<fn()> having a null niche, so that part is not up for debate any more.

We considered requiring function pointers to be aligned to some target-specific alignment. However, for all currently supported targets, that alignment would be 1. (Note that even on ARM, where functions have alignment requirements, the lowest bit is used to indicate Thumb mode. So while functions are aligned, function pointers are not.) Furthermore, C clearly allows casting 0 to function pointers; it will be hard to argue that casting 1 or -1 to a function pointer is UB in C, so a target that makes alignment requirements for function pointers a validity concern is likely to be incompatible with C. Common practice conforms this: for instance, 1 is a common value for SIG_IGN which is cast to a function pointer to be stored in the sa_handler field of sigaction -- and while a new target could of course pick a higher-aligned value for SIG_IGN, there is likely more C code out there using various sentinel values. SQLite uses -1 as a sentinel. Even Rust code using 1 as a sentinel function pointer has been spotted in the wild.

Examples

The following pieces of code cause UB (as in, the UB arises when executing the code, not just potentially later):

let _val: fn() = MaybeUninit::uninit().assume_init();
let _val: fn() = mem::transmute(0usize);

The following piece of code is well-defined:

let val: fn() = mem::transmute(1usize);
// Note that safe code can cause UB with `val`!
// This operation is not sound, but it is well-defined.
// It violates only the safety invariant of `fn()`,
// not its validity invariant.

The following is not decided by this FCP:

let ptr = &0i32;
let ptr_to_ptr = addr_of!(ptr).cast::<usize>();
ptr_to_ptr.cast::<fn()>().read(); // pointer-to-fnptr transmutation -- UB or not?

The following functions are sound (as in, safe code invoking these functions can never have UB):

fn to_fn_then_discard(x: usize) {
  if x == 0 { return; }
  let f: fn() = unsafe { mem::transmute(x) };
  // Calling `f` would be very unsound, but just creating this pointer and then never using it is fine.
  drop(f);
}

Prior discussion

@RalfJung

This comment was marked as outdated.

@CAD97
Copy link

CAD97 commented Aug 4, 2023

While I did make the argument to not bundle this with the other simple validity, I have no concerns with this actually being the validity requirement for function pointers. Even in the mentioned case where ABI expects function pointers to be aligned (say, to pack bool arguments as logically part of the ABI rather than part of the pointer), this can be made to be a requirement of the ABI rather than directly of function pointer layout.

@comex

This comment was marked as resolved.

@RalfJung

This comment was marked as resolved.

@RalfJung
Copy link
Member Author

RalfJung commented Aug 5, 2023

Now again with T-opsem label...
@rfcbot merge

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 5, 2023

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
Copy link
Collaborator

rfcbot commented Aug 5, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot
Copy link
Collaborator

rfcbot commented Aug 15, 2023

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
Copy link
Member Author

RalfJung commented Aug 21, 2023

Added to our FCP list of "decisions we made", so the issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants