Validity of function pointers

[RalfJung]

Discussing the validity of function pointers.

Clearly, the must be non-NULL. Since we exclude some bit patterns, we likely also do not want to allow the entire value to be uninitialized. We could allow some bits to be uninitialized as long as there is at least one bit initialized to 1, but, uh, why?^^

Anything else? Do fn ptrs have to point to allocated executable memory? This discussion concludes that the answer ought to be "no", because there's little to no benefit and it would interact badly with unloading shared libraries.

[nikomatsakis]

I think we should not require that they point to allocated, executable memory and we should forbid NULL, but otherwise permit any initialized value. As you say, I don't see much value to permitting some bits to be uninitialized.

[RalfJung]

I recently learned that function pointers could, in principle, have alignment requirements. However, currently the alignment seems to be 1 for all supported platforms. The fn docs call out the non-NULL property as does the Nomicon, but they do not call out alignment. Miri also does not check fn ptr alignment as it has no good way to figure out what the requirements for that are, and anway it currently would not do anything.

So should those docs be amended to call out the alignment situation? Which place should they point at if someone wants to figure out the fn ptr alignment for their platform? There is no such thing as align_of for this.

Also, should https://doc.rust-lang.org/reference/types/function-pointer.html or https://doc.rust-lang.org/book/ch19-05-advanced-functions-and-closures.html call out the non-NULL property and/or the alignment?

[hanna-kruppe]

I don't think we should put any alignment requirement into the validity invariant. LLVM having this notion is no reason to -- in fact, the function pointer alignment patch fixed a wrong assumption made by LLVM's optimizations about what happens when you take a pointer to a known function. Trying to establish alignment requirements for arbitrary pointers to unknown functions would go far beyond that and is too much headache for too little reward:

• In terms of instruction alignment required by the hardware, there's often not much alignment to be had: at most 2 bytes on pretty much every ISA that doesn't have horrible code density, and not even that on e.g. x86(-64).
  • Sometimes we can't even exploit the instruction alignment the hardware does require, e.g. ARM hardware uses the LSB of function pointers to indicate a switch to Thumb mode, so their LSB can be set even though the function it points to is aligned to 2 bytes or more.
  • The instruction alignment requirement can be lowered by later extensions that are intended to be backwards compatible (e.g., for RISC-V, it's normally 4, but once the C extension is added it can become 2).
• Even if the platform ABI prescribes that functions are to be aligned to such-and-such, it's perfectly possible to generate a symbol that goes below this threshold, and it would be really annoying if that would cause subtle UB in Rust code that handles a pointer to that function.

[hanna-kruppe]

Oh, and all of this is assuming we're only interested in allowing legitimate pointers to actual functions you can jump to. It's potentially useful if one can just smuggled arbitrary non-null addresses through a function pointer that is never called. It's certainly less of a footgun. People are already being bitten relatively frequently by function pointers being non-null, making the niche larger can only make that (marginally) worse.

[RalfJung]

I don't think we should put any alignment requirement into the validity invariant.

That's fine for me as long as LLVM does the same.

[hanna-kruppe]

Function pointers aren't really different from other pointers (in fact, they'll be literally indistinguishable once the long-awaited untyped ptr arrives). And alignment of pointers is only asserted if you do something with them (e.g., dereference or, in this case, call) or explicitly put in an assume call, align attribute, etc. into the IR.

[RalfJung]

I see. So I guess what I mean is "as long as we don't add any assume or align".

[RalfJung]

Also see this discussion. Currently at least docs would have to keep open the possibility that function pointers have to be aligned to "something" in the future. OTOH, I feel adding something like that at rust-lang/nomicon#149 would not be very useful, as we couldn't even define the "something".

Would it be worth trying to write a (small) RFC that basically just says "for function pointers, NULL is and will be the only invalid value"? Just to get that out of the way?

[Lokathor]

I'd like an RFC to that effect, and, because it's such a simple statement, I'd like that to be considered for entry into the proposed "normative statements about Rust list" that the Reference can hopefully begin to grow (rust-lang/reference#629).

[elichai]

Is there any way to create a dangling function pointer "safely"?
i.e.

pub type MyFn = unsafe extern "C" fn(a: *const u8);

static EMPTY_FUNC: MyFn = std::mem::align_of::<MyFn>() as MyFn;

[gnzlbg]

@elichai

With the proposed wording for a future RFC, the only value you can't assign to a function pointer is 0, all other values are ok. If you want to represent "emptiness", you probably want to use an Option<...fn ptr...> which is already guaranteed to have the same size as the pointer due to its niche.

---

A particular example of where we need unaligned function pointers came up in libc (https://github.com/rust-lang/libc/pull/1626/files#r360995164). On Windows, libc would like to map Windows C API to:

// In C: typedef void (__CRTDECL* _crt_signal_t)(int);
type sighandler_t = Option<extern "cdecl" fn(c_int) -> ()>;
// In C: #define SIG_... ((_crt_signal_t)-i)  // where i = [0, 4]
pub const SIG_DFL: sighandler_t = None;
pub const SIG_IGN: sighandler_t = Some(1_usize as );
pub const SIG_GET: sighandler_t = Some(2_usize as );
pub const SIG_SGE: sighandler_t = Some(3_usize as );
pub const SIG_ACK: sighandler_t = Some(4_usize as );

so even if Windows requires functions to be 2-byte or 4-byte aligned, that would still require that unaligned function pointers aren't invalid (cc @retep998).

[RalfJung]

static EMPTY_FUNC: MyFn = std::mem::align_of::<MyFn>() as MyFn;

I am not sure what you were trying to accomplish here, but this makes little sense. align_of::<MyFn> defines which alignment a &MyFn must have -- just like align_of::<i32> defines which alignment a &i32 must have. IOW, you are using the alignment of references to function pointers here. There is no way to query the alignment of function pointers themselves.

unaligned function pointers

Are they unaligned though? On all currently supported platforms, the alignment is 1.

[saethlin]

Miri currently does some kind of type validation for function pointers:

error: Undefined Behavior: type validation failed: encountered 0x0000000000000001, but expected a function pointer
 --> src/main.rs:3:9
  |
3 |         core::mem::transmute::<usize, fn()>(0x1);
  |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered 0x0000000000000001, but expected a function pointer
  |
  = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
  = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information

What rule is Miri using to reject this code? As far as I can tell the only thing agreed-upon above is that fn pointers may not be null.

[RalfJung]

Indeed, Miri currently checks that the pointer actually points to a function:

https://github.com/rust-lang/rust/blob/450ef8613ce80278b98e1b1a73448ea810322567/compiler/rustc_const_eval/src/interpret/validity.rs#L578

[thomcc]

There are a number of FFI apis that require this. It's kind of annoying that something that's a #define SOME_CONST ((some_fptr_type)-1) or something can't be used as a const because of this, but yeah, i guess arguably that code should use MaybeUninit<extern "C" unsafe fn(...)> anyway (I'd rather this wasn't necessary, of course, since its an easy mistake to make and not directly supported in bindgen)

[saethlin]

It makes some sense that a fn pointer must be valid because once created, since they are safe to call. I'm just here because futures-0.1.31 which is still in shockingly wide use does this:
https://github.com/rust-lang/futures-rs/blob/49ad690a604304c6dfd750f40f4b0809faeb01cf/src/task_impl/std/mod.rs#L61-L67

    // Note that we won't actually use these functions ever, we'll instead be
    // testing the pointer's value elsewhere and calling our own functions.
    INIT.call_once(|| unsafe {
        let get = mem::transmute::<usize, _>(0x1);
        let set = mem::transmute::<usize, _>(0x2);
        init(get, set);
    });

and I can't find any documentation that I could point the maintainers to which says that they can't do this.

[thomcc]

Then would unsafe fn have a different validity requirement, because they're not safe to call?

[RalfJung]

It makes some sense that a fn pointer must be valid because once created, since they are safe to call.

Certainly its safety invariant requires that it points to an actual function.

Its validity invariant, however, could be much weaker than that -- and indeed I see no good reason to make it any stronger than "non-null". So I would be fine with relaxing this check in Miri.

Whether we also want to relax this check for const validation is a different question, those checks are somewhat more strict than Miri's checks already anyway (e.g. they recurse through references).

[thomcc]

If we don't relax it for const validation, then at least loosening the error message from "This is UB" to "This is not supported in const" seems like it might be nice, if possible.

[saethlin]

If this is permitted, I would prefer to not get an error from Miri. Perhaps selfishly, producing an error on this code makes Miri unable to find anything further in codebases which depend on a library that does this. There are quite a few.

I feel like this is normally the kind of place you'd get a warning, but I don't think Miri does warnings (at the moment?).

[thomcc]

Another example of something like this in the wild is in window-sys 0.32.0. GetProcAddress returns a FARPROC, which is a Option<unsafe extern "system" fn() -> isize>. You then need to transmute this to a function pointer of the type you actually need, which potentially has a different ABI/return type/parameter types.

This seems like it should be fine for most of the suggested validity requirements here, but the "FIXME: Check if the signature matches" comment at https://github.com/rust-lang/rust/blob/450ef8613ce80278b98e1b1a73448ea810322567/compiler/rustc_const_eval/src/interpret/validity.rs#L585 could cause issues if the signature had to match to avoid UB.

[RalfJung]

Following this discussion, rust-lang/rust#94270 relaxes the function pointer validity check in Miri to just check for non-null. (The CTFE check is unchanged, that is a separate discussion.)

[RalfJung]

So this is closed as... team has consensus? But where is the consensus documented? If "nowhere", shouldn't the issue track writing down the consensus?