mab122
/
radicle-heartwood-lfs
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

signals: Use `signals_receipts` crate instead 

This resolves issues:
- 111cf90 (`handler` is not async-signal-safe)
- c8e0300 (`libc::signal` should not be used)
- b91c6c3 (Mutex can cause signals to be missed)

See also: patch 8e05702.

The API of `radicle-signals` is the same as before, and so uses of it
don't need to be and aren't changed.  The behavior is slightly different
in that:

- If the channel is full then signals will not be lost, which is an
  improvement.  This is achieved without blocking in the signal handler.
  This is possible because of the counters approach along with the
  internal receipts-processing thread of the `signals_receipts` crate.

- `install()` and `uninstall()` might block very briefly if necessary to
  acquire the mutex, which is now internal to and managed by the
  `signals_receipts` crate, only if there are concurrent calls to them
  (which is unlikely), but such blocking is guaranteed to be bounded to
  be very brief.  This is done so they no longer can fail to do their
  purpose, which is an improvement.  They still return errors if the
  handling is already installed or uninstalled, respectively, which
  preserves the previous use cases.

- The new `finish()` function is introduced.  This is provided in case
  it's ever needed to completely clean-up the facility, by terminating
  the internal receipts-processing thread, to be like it hadn't been
  installed before.

- The user must ensure that the notifications channel is disconnected,
  by dropping the receiver(s), when doing `uninstall()` or `finish()`.
  Such dropping usually occurs naturally, and already occurs for all the
  preexisting uses of `radicle_signals`.  (The `signals_receipts` crate
  is capable of a more robust approach, but this commit doesn't use
  that, to avoid changing the preexisting uses of `radicle_signals`.)

- The `TryFrom` impl for `Signal` is of `SignalNumber` which is `c_int`,
  instead of `i32`, because `c_int` (the type of signal numbers) might
  not be `i32` on all platforms (POSIX only requires `c_int` to be at
  least 32-bit).

The version of `radicle_signals` is incremented, to reflect those
changes and the substantially different internal implementation.

The new dependency on the `base64` crate is needed by the
`signals_receipts` crate for it to work on macOS, because its dependency
on the `sem_safe` crate uses `base64` as part of creating anonymous
semaphores on macOS (which lacks support for unnamed semaphores (in
violation of POSIX)).

Signed-off-by: Derick Eddington <kcired@pm.me>
Signed-off-by: Lorenz Leutgeb <lorenz@leutgeb.xyz>
This commit is contained in:
Derick Eddington 2024-09-14 14:30:43 -07:00 committed by Lorenz Leutgeb
parent c05434ebd5
commit 47c785b916
No known key found for this signature in database
3 changed files with 126 additions and 82 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
Cargo.lock generated
View File

@ -213,6 +213,12 @@ version = "0.21.7"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "9d297deb1925b89f2ccc13d7635fa0714f12c87adce1c75356b39ca9b7178567"
[[package]]
name = "base64"
version = "0.22.1"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "72b3254f16251a8381aa12e40e3c4d2f0199f8c6508fbecb9d91f575e0fbb8c6"
[[package]]
name = "base64ct"
version = "1.6.0"
@ -655,9 +661,9 @@ checksum = "5443807d6dff69373d433ab9ef5378ad8df50ca6298caf15de6e52e24aaf54d5"
[[package]]
name = "errno"
version = "0.3.8"
version = "0.3.9"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "a258e46cdc063eb8519c00b9fc845fc47bcfca4130e2f08e88665ceda8474245"
checksum = "534c5cf6194dfab3db3242765c03bbe257cf92f22b38f6bc0c58d59108a820ba"
dependencies = [
"libc",
"windows-sys 0.52.0",
@ -750,9 +756,9 @@ dependencies = [
[[package]]
name = "getrandom"
version = "0.2.14"
version = "0.2.15"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "94b22e06ecb0110981051723910cbf0b5f5e09a2062dd7663334ee79a9d1286c"
checksum = "c4567c8db10ae91089c99af84c68c38da3ec2f087c3f82960bcdbf3656b6f4d7"
dependencies = [
"cfg-if",
"libc",
@ -2134,10 +2140,11 @@ dependencies = [
[[package]]
name = "radicle-signals"
version = "0.10.0"
version = "0.11.0"
dependencies = [
"crossbeam-channel",
"libc",
"signals_receipts",
]
[[package]]
@ -2385,6 +2392,18 @@ dependencies = [
"zeroize",
]
[[package]]
name = "sem_safe"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "9110d660d39b5f65f10a978ce0dd5e0a08c50e1d5589cadfad9b9a5f221fdb14"
dependencies = [
"base64 0.22.1",
"errno",
"getrandom",
"libc",
]
[[package]]
name = "serde"
version = "1.0.198"
@ -2456,6 +2475,18 @@ version = "1.3.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64"
[[package]]
name = "signals_receipts"
version = "0.2.0"
source = "registry+https://github.com/rust-lang/crates.io-index"
checksum = "74fa07c27e948f68f2f72241a189850526fbd3b428eee702884ba1883a39610e"
dependencies = [
"cfg-if",
"errno",
"libc",
"sem_safe",
]
[[package]]
name = "signature"
version = "1.6.4"

3
radicle-signals/Cargo.toml
View File

@ -5,8 +5,9 @@ homepage = "https://radicle.xyz"
repository = "https://app.radicle.xyz/seeds/seed.radicle.xyz/rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5"
license = "MIT OR Apache-2.0"
edition = "2021"
version = "0.10.0"
version = "0.11.0"
[dependencies]
crossbeam-channel = { version = "0.5.6" }
libc = { version = "0.2" }
signals_receipts = { version = "0.2.0", features = ["channel_notify_facility"] }

164
radicle-signals/src/lib.rs
View File

@ -1,7 +1,12 @@
use std::io;
use std::sync::Mutex;
use crossbeam_channel as chan;
use signals_receipts::channel_notify_facility::{
self, FinishError, InstallError, SendError, SignalsChannel as _, UninstallError,
};
use signals_receipts::SignalNumber;
use crate::channel_notify_facility_premade::SignalsChannel;
/// Operating system signal.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
@ -16,10 +21,10 @@ pub enum Signal {
WindowChanged,
}
impl TryFrom<i32> for Signal {
type Error = i32;
impl TryFrom<SignalNumber> for Signal {
type Error = SignalNumber;
fn try_from(value: i32) -> Result<Self, Self::Error> {
fn try_from(value: SignalNumber) -> Result<Self, Self::Error> {
match value {
libc::SIGTERM => Ok(Self::Terminate),
libc::SIGINT => Ok(Self::Interrupt),
@ -30,90 +35,97 @@ impl TryFrom<i32> for Signal {
}
}
/// Signal notifications are sent via this channel.
static NOTIFY: Mutex<Option<chan::Sender<Signal>>> = Mutex::new(None);
// The signals of interest to handle.
signals_receipts::channel_notify_facility! {
SIGINT
SIGTERM
SIGHUP
SIGWINCH
}
/// A slice of signals to handle.
const SIGNALS: &[i32] = &[libc::SIGINT, libc::SIGTERM, libc::SIGHUP, libc::SIGWINCH];
/// Install global signal handlers.
/// Install global signal handlers, with notifications sent to the given
/// `notify` channel.
pub fn install(notify: chan::Sender<Signal>) -> io::Result<()> {
if let Ok(mut channel) = NOTIFY.try_lock() {
if channel.is_some() {
return Err(io::Error::new(
io::ErrorKind::AlreadyExists,
"signal handler is already installed",
));
}
*channel = Some(notify);
/// The sender type must implement the facility's trait.
#[derive(Debug)]
struct ChanSender(chan::Sender<Signal>);
unsafe { _install() }?;
} else {
return Err(io::Error::new(
io::ErrorKind::WouldBlock,
"unable to install signal handler",
));
/// This also does our desired conversion from signal numbers to our
/// `Signal` representation.
impl channel_notify_facility::Sender for ChanSender {
fn send(&self, sig_num: SignalNumber) -> Result<(), SendError> {
if let Ok(sig) = sig_num.try_into() {
self.0.send(sig).or(Err(SendError::Disconnected))
} else {
debug_assert!(false, "only called for recognized signal numbers");
// Unrecognized signal numbers would be ignored, but
// this never occurs.
Err(SendError::Ignored)
}
}
}
Ok(())
SignalsChannel::install_with_outside_channel(ChanSender(notify)).map_err(|e| match e {
InstallError::AlreadyInstalled { unused_notify: _ } => io::Error::new(
io::ErrorKind::AlreadyExists,
"signal handling is already installed",
),
_ => io::Error::other(e), // The error type is non-exhaustive.
})
}
/// Uninstall global signal handlers.
///
/// The caller must ensure that all `Receiver`s for the other end of the
/// channel are dropped to disconnect the channel, to ensure that the
/// internal "signals-receipt" thread wakes to clean-up (in case it's
/// blocked on sending on the channel). Such dropping usually occurs
/// naturally when uninstalling, since the other end is no longer needed
/// then, and may be done after calling this. Not doing so might
/// deadlock the "signals-receipt" thread.
pub fn uninstall() -> io::Result<()> {
if let Ok(mut channel) = NOTIFY.try_lock() {
if channel.is_none() {
return Err(io::Error::new(
io::ErrorKind::NotFound,
"signal handler is already uninstalled",
));
SignalsChannel::uninstall_with_outside_channel().map_err(|e| match e {
UninstallError::AlreadyUninstalled => io::Error::new(
io::ErrorKind::NotFound,
"signal handling is already uninstalled",
),
#[allow(clippy::unreachable)]
UninstallError::WrongMethod => {
// SAFETY: Impossible, because `SignalsChannel` is private
// and so `SignalsChannel::install()` is never done.
unreachable!()
}
*channel = None;
unsafe { _uninstall() }?;
} else {
return Err(io::Error::new(
io::ErrorKind::WouldBlock,
"unable to uninstall signal handler",
));
}
Ok(())
_ => io::Error::other(e), // The error type is non-exhaustive.
})
}
/// Install global signal handlers.
/// Do [`uninstall()`], terminate the internal "signals-receipt"
/// thread, and wait for that thread to finish.
///
/// # Safety
/// This is provided in case it's ever needed to completely clean-up the
/// facility to be like it hadn't been installed before. It's
/// unnecessary to use this, just to uninstall the handling. Usually,
/// only using `uninstall` is desirable so that the "signals-receipt"
/// thread is kept alive for faster reuse when re-installing the
/// handling.
///
/// Calls `libc` functions safely.
unsafe fn _install() -> io::Result<()> {
for signal in SIGNALS {
if libc::signal(*signal, handler as libc::sighandler_t) == libc::SIG_ERR {
return Err(io::Error::last_os_error());
/// The caller must ensure that all `Receiver`s for the other end of the
/// channel have **already** been dropped to disconnect the channel,
/// before calling this, to ensure that the "signals-receipt" thread
/// wakes (in case it's blocked on sending on the channel) to see that
/// it must finish. If this is not done, this might deadlock.
pub fn finish() -> io::Result<()> {
SignalsChannel::finish_with_outside_channel().map_err(|e| match e {
FinishError::AlreadyFinished => io::Error::new(
io::ErrorKind::NotFound,
"signal-handling facility is already finished",
),
#[allow(clippy::unreachable)]
FinishError::WrongMethod => {
// SAFETY: Impossible, because `SignalsChannel` is private
// and so `SignalsChannel::install()` is never done.
unreachable!()
}
}
Ok(())
}
/// Uninstall global signal handlers.
///
/// # Safety
///
/// Calls `libc` functions safely.
unsafe fn _uninstall() -> io::Result<()> {
for signal in SIGNALS {
if libc::signal(*signal, libc::SIG_DFL) == libc::SIG_ERR {
return Err(io::Error::last_os_error());
}
}
Ok(())
}
/// Called by `libc` when a signal is received.
extern "C" fn handler(sig: libc::c_int, _info: *mut libc::siginfo_t, _data: *mut libc::c_void) {
let Ok(sig) = sig.try_into() else {
return;
};
if let Ok(guard) = NOTIFY.try_lock() {
if let Some(c) = &*guard {
c.try_send(sig).ok();
}
}
_ => io::Error::other(e), // The error type is non-exhaustive.
})
}