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

Signal should support arbitrary signals #1249

Closed
joshtriplett opened this issue Jan 4, 2025 · 7 comments · Fixed by #1292
Closed

Signal should support arbitrary signals #1249

joshtriplett opened this issue Jan 4, 2025 · 7 comments · Fixed by #1292
Labels
semver bump Issues that will require a semver-incompatible fix

Comments

@joshtriplett
Copy link
Member

joshtriplett commented Jan 4, 2025

The Signal enum is missing the real-time signals, from SIGRTMIN to SIGRTMAX.

Handling these will be interesting, because glibc reserves the first two of them, and does so by changing its exposed value of SIGRTMIN.

I would suggest exposing the same SIGRTMIN that glibc does (34 rather than 32), and additionally exposing enum values for the two reserved signal numbers, with names that clearly indicate that they're reserved by libc.

That way, when people port from C to Rust, they won't get surprised by the difference.

@sunfishcode
Copy link
Member

A complication is that glibc does not expose the values of SIGRTMIN or SIGRTMAX in its ABI. Even the number of signals it reserves is not exposed. The number of reserved signals has changed in the past and could theoretically change again in the future.

Perhaps we should make functions like kill_process unsafe. That way we could allow Signal to hold any SIGRTMIN+n signal and just say it's UB if you ever use a reserved signal value.

Another would be to make Signal::from_raw unsafe, and make it UB to construct a Signal with a reserved signal value.

@sunfishcode sunfishcode added the semver bump Issues that will require a semver-incompatible fix label Jan 27, 2025
@joshtriplett
Copy link
Member Author

I think at this point it is safe to assume that the number of reserved signals won't change anymore; it has been the same since the move from libc5 to libc6, and there will almost certainly never be a libc7. If that did change, far too much would break.

If you prefer to not bake in that assumption, you could expose the real SIGRTMIN, and let applications handle avoiding the reserved signals.

Either way, I don't think you need to make the possibility of sending those signals any more unsafe than any other signal; sending any signal an application doesn't expect can cause problems. So if you want to make something unsafe, I would suggest kill_process rather than the creation of the signal number.

sunfishcode added a commit that referenced this issue Jan 27, 2025
Change `Signal` from an `enum` to a `struct` wrapper around an `i32`
so that it can accomodate values it doesn't know about statically,
and add APIs for constructing and working with `SIGRTMIN+n` signal
values.

This requires making `Signal::from_raw` depend on a new "use-libc-sigrt"
feature, as it depends on being able to call into libc. This also adds
`Signal::from_raw_unchecked` which does not call into libc and is
unsafe.

Fixes #1249.
@sunfishcode
Copy link
Member

I agree that it's unlikely that libc would claim new SIGRTMIN+n signals at this point, but I'd prefer to avoid claiming that space as our own if practical.

I think I've found a good way to approach this, so I've now filed #1292 which has:

  • from_raw is now behind "use-libc-sigrt" feature and now supports SIGRTMIN+n values (safely).
  • there's now a Signal::rt(n) constructor for constructing SIGRTMIN+n values, also enabled with "use-libc-sigrt"
  • an unsafe from_raw_unchecked that returns Self
  • etc.

Does that look like it'll work for your use cases?

@joshtriplett
Copy link
Member Author

Having rt_min and rt_max, which depend on libc, does make sense.

However, I'd really like to be able to work with signals safely without introducing a dependency on libc.

Would you consider, as an alternative:

  • Keeping the use-libc-sigrt feature, as you have now, with the current behavior, and
  • Having a use-hardcoded-libc-sigrt feature on Linux, which hardcodes the current well-known values, and the name self-documents that the user is taking responsibility for the decision to hardcode them.

That way, it'd be possible for applications to either depend on libc or hardcode the libc6 ABI, and use safe code.

@sunfishcode
Copy link
Member

Are you writing code which can know it isn't sharing a process with a libc? If so, we could add a safe pub fn sigrt(n: i32) -> Option<Signal> to the rustix::runtime module, that works in terms of linux_raw_sys::general::SIGRTMIN instead of libc's, so it would give you access to the whole range.

@joshtriplett
Copy link
Member Author

Are you writing code which can know it isn't sharing a process with a libc?

Not necessarily, I'd just like to avoid calling into libc if not necessary.

If so, we could add a safe pub fn sigrt(n: i32) -> Option to the rustix::runtime module, that works in terms of linux_raw_sys::general::SIGRTMIN instead of libc's, so it would give you access to the whole range.

That would help, and I could live with that being in runtime.

@sunfishcode
Copy link
Member

Some datapoints:

None of these necessarily rules out using hardcoded values, but do mean it's worth considering alternatives before doing so.

I've now added a patch to #1292 adding rustix::runtime::sigrt, which constructs SIGRTMIN+n Signal values using the kernel SIGRTMIN value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver bump Issues that will require a semver-incompatible fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants