Skip to content

Commit 2b6668d

Browse files
committed
fix: hold PTY_LOCK for entire Terminal lifetime on musl
Extend the musl PTY_LOCK to cover the complete Terminal session, including I/O and background thread cleanup. Previously the lock only covered spawn and FD cleanup, but concurrent PTY I/O also triggers SIGSEGV in musl internals. Key changes: - Store MutexGuard in Terminal struct (held until drop) - Add JoinOnDrop wrapper to join background cleanup thread before releasing the lock, preventing FD cleanup from racing with the next PTY session - Drop order: reader/writer/child → join BG thread → release lock https://claude.ai/code/session_011H8UR3gS6hoyQAf2x7Dfw8
1 parent ef64d0f commit 2b6668d

File tree

1 file changed

+42
-12
lines changed

1 file changed

+42
-12
lines changed

crates/pty_terminal/src/terminal.rs

Lines changed: 42 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,42 @@ impl Clone for ChildHandle {
4646
}
4747
}
4848

49+
/// On musl libc, concurrent PTY operations (openpty, fork, I/O, close) trigger
50+
/// SIGSEGV/SIGBUS in musl internals. This lock serializes the entire PTY session
51+
/// so that only one Terminal exists at a time, while allowing non-PTY tests to
52+
/// run in parallel.
53+
#[cfg(target_env = "musl")]
54+
static PTY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
55+
56+
/// Wrapper that joins a thread on drop instead of detaching it.
57+
#[cfg(target_env = "musl")]
58+
struct JoinOnDrop(Option<thread::JoinHandle<()>>);
59+
60+
#[cfg(target_env = "musl")]
61+
impl Drop for JoinOnDrop {
62+
fn drop(&mut self) {
63+
if let Some(handle) = self.0.take() {
64+
let _ = handle.join();
65+
}
66+
}
67+
}
68+
4969
/// A headless terminal consisting of a PTY reader, writer, and a child process handle.
70+
///
71+
/// On musl, fields are dropped in declaration order: reader/writer/child first,
72+
/// then background thread is joined, then PTY lock is released. This ensures
73+
/// all FD cleanup completes before the next PTY session can start.
5074
pub struct Terminal {
5175
pub pty_reader: PtyReader,
5276
pub pty_writer: PtyWriter,
5377
pub child_handle: ChildHandle,
78+
/// On musl, join the background cleanup thread before releasing the PTY lock.
79+
#[cfg(target_env = "musl")]
80+
_bg_handle: JoinOnDrop,
81+
/// On musl, holds the PTY lifecycle lock for the entire terminal session to
82+
/// prevent concurrent PTY operations that trigger SIGSEGV in musl internals.
83+
#[cfg(target_env = "musl")]
84+
_pty_guard: std::sync::MutexGuard<'static, ()>,
5485
}
5586

5687
struct Vt100Callbacks {
@@ -256,14 +287,8 @@ impl Terminal {
256287
///
257288
/// Panics if the writer lock is poisoned when the background thread closes it.
258289
pub fn spawn(size: ScreenSize, cmd: CommandBuilder) -> anyhow::Result<Self> {
259-
// On musl libc (Alpine Linux), concurrent PTY operations trigger
260-
// SIGSEGV/SIGBUS in musl internals (sysconf, fcntl). This affects
261-
// both openpty+fork and FD cleanup (close) from background threads.
262-
// Serialize all PTY lifecycle operations that touch musl internals.
263-
#[cfg(target_env = "musl")]
264-
static PTY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
265290
#[cfg(target_env = "musl")]
266-
let _spawn_guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
291+
let pty_guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
267292

268293
let pty_pair = portable_pty::native_pty_system().openpty(portable_pty::PtySize {
269294
rows: size.rows,
@@ -289,16 +314,12 @@ impl Terminal {
289314
// macOS then returns EIO on the master without draining the output buffer,
290315
// causing data loss. Holding the slave until the background thread takes
291316
// over guarantees the PTY stays connected while the child runs.
292-
thread::spawn({
317+
let bg_handle = thread::spawn({
293318
let writer = Arc::clone(&writer);
294319
let exit_status = Arc::clone(&exit_status);
295320
let slave = pty_pair.slave;
296321
move || {
297322
let _ = exit_status.set(child.wait().map_err(Arc::new));
298-
// On musl, serialize FD cleanup (close) with PTY spawn to
299-
// prevent racing on musl-internal state.
300-
#[cfg(target_env = "musl")]
301-
let _cleanup_guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
302323
// Close writer first, then drop slave to trigger EOF on the reader.
303324
*writer.lock().unwrap() = None;
304325
drop(slave);
@@ -315,10 +336,19 @@ impl Terminal {
315336
},
316337
)));
317338

339+
// Suppress unused-variable warning on non-musl targets where
340+
// the handle is not stored in the struct.
341+
#[cfg(not(target_env = "musl"))]
342+
drop(bg_handle);
343+
318344
Ok(Self {
319345
pty_reader: PtyReader { reader, parser: Arc::clone(&parser) },
320346
pty_writer: PtyWriter { writer, parser, master },
321347
child_handle: ChildHandle { child_killer, exit_status },
348+
#[cfg(target_env = "musl")]
349+
_bg_handle: JoinOnDrop(Some(bg_handle)),
350+
#[cfg(target_env = "musl")]
351+
_pty_guard: pty_guard,
322352
})
323353
}
324354
}

0 commit comments

Comments
 (0)