Using the correct type of sighandler_t windows

[vorner]

I'm opening this here as continuation of discussion on #1626. This also might be related to #1359.

Currently, sighandler_t is an alias to usize on Windows targets. While this works, it is strictly speaking wrong and the right type should be something like Option<extern "C" fn(c_int) -> ()>.

There are, however, concerns about the switch:

• It is an API change that might break someone's code. At least a crater run would be needed.
• What is the correct type? Due to the fact this doesn't always contain a valid function pointer, but also some marker constants (eg. SIG_IGN, which has value of 1), it is unclear if this is correct on the Rust side. It seems the consensus is function pointers need to be non-null (solved by the Option) and properly aligned, but the alignment of function pointer on all platforms Windows currently run is 1, so for that reason this should be fine.

[vorner]

Specifically in answer to this #1626 (comment)

How does this work in C?

IIRC, C requires function pointers to be properly aligned as well, at least when you call them, right ?

I guess it doesn't call these weird constants, eg on the windows kernel side it might look like something like:

switch (handler) {
  case SIG_DFL: // = 0
    do_default();
    break;
  case SIG_IGN: // = 1
    break;
  ...
  default:
    handler();
    break;
}

Therefore, in case it is one of the constants, the value is never used as a pointer, only compared. Which you probably can do in C, especially if you are the platform owner and control the compiler.

I was worried if, on the Rust side, it could be UB just to create the invalid function pointer ‒ at least, it is insta-UB to create an enum with invalid discriminant or reference to invalid memory, you don't even have to use it.

[nico-abram]

Maybe a MaybeUninit<Option<extern "C" fn(c_int) -> ()>> could be used to make sure the alignment requirement doesn't trigger UB?

[tgross35]

Here's one msys reference for newlib https://github.com/msys2/msys2-runtime/blob/7484c5594806de5751a264600f648d246851fa15/newlib/libc/include/signal.h#L15C9-L15C22 which makes sighandler_t a _sig_func_ptr. I assume Option<extern "C" fn(_: c_int) -> ()> would be the correct type here, but I can't find a source (please post a header link if you have one).

PR welcome to change this in 1.0.

[tgross35]

Correcting myself: if we can't verify that the value is always a valid function, the field probably just has to be something like *const c_void. It would be good to check the history of when this got added to see if there is any discussion.

[mbyx]

There's a Wine reference: https://github.com/wine-mirror/wine/blob/3d09a2547c540a42741aa57b46265495f7f284c1/include/msvcrt/signal.h#L39
The msys reference: https://github.com/msys2/msys2-runtime/blob/23a25d49e3b8c18f12ce588869d4cb24de9f2454/newlib/libc/machine/cris/sys/signal.h#L23

So the correct type would be Option<extern "C" fn(_: c_int) -> ()>, but even just creating a pointer to an invalid function is UB, so it wouldn't be possible to use it.

From the Rustnomicon (https://doc.rust-lang.org/nomicon/transmutes.html):

First and foremost, creating an instance of any type with an invalid state is going to cause arbitrary chaos that can't really be predicted. Do not transmute 3 to bool. Even if you never do anything with the bool. Just don't.

[retep998]

The correct thing to do here is in fact some sort of raw pointer. So long as you can't guarantee that the function pointer will be valid, then you cannot use a function pointer type (unless Rust gains raw function pointers).
MaybeUninit is not really the right tool for the job because the value inside is definitely initialized, it's just not always a valid function pointer.