Skip to content

Commit bd7aee9

Browse files
committed
fix: guard PTY FD drops with musl lock to prevent SIGSEGV
The previous fix serialized openpty+spawn and background cleanup, but PtyReader and PtyWriter drops (which close FDs) were unguarded. When parallel tests drop Terminals concurrently, FD closes race with openpty in musl internals causing SIGSEGV. Use ManuallyDrop for FD-owning fields and acquire PTY_LOCK in Drop impls so all FD operations are serialized on musl.
1 parent d3244e7 commit bd7aee9

File tree

2 files changed

+83
-10
lines changed

2 files changed

+83
-10
lines changed
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
name: Musl Stability Test
2+
3+
permissions:
4+
contents: read
5+
6+
on:
7+
push:
8+
branches:
9+
- claude/reproduce-flaky-failure-RuwlG
10+
11+
concurrency:
12+
group: musl-stability-${{ github.sha }}
13+
14+
jobs:
15+
test-musl:
16+
name: Test (musl) - Run ${{ matrix.run }}
17+
strategy:
18+
fail-fast: false
19+
matrix:
20+
run: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
21+
runs-on: ubuntu-latest
22+
container:
23+
image: node:22-alpine3.21
24+
options: --shm-size=256m
25+
env:
26+
RUSTFLAGS: --cfg tokio_unstable -D warnings -C target-feature=-crt-static
27+
steps:
28+
- name: Install Alpine dependencies
29+
shell: sh {0}
30+
run: apk add --no-cache bash curl git musl-dev gcc g++ python3
31+
32+
- uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
33+
with:
34+
persist-credentials: false
35+
submodules: true
36+
37+
- name: Install rustup
38+
run: |
39+
curl --proto '=https' --tlsv1.2 -sSf https://sh.rustup.rs | sh -s -- -y --default-toolchain none
40+
echo "$HOME/.cargo/bin" >> "$GITHUB_PATH"
41+
42+
- name: Install Rust toolchain
43+
run: rustup show
44+
45+
- name: Install pnpm and Node tools
46+
run: |
47+
corepack enable
48+
pnpm install
49+
50+
- run: cargo test

crates/pty_terminal/src/terminal.rs

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
use std::{
22
collections::VecDeque,
33
io::{Read, Write},
4+
mem::ManuallyDrop,
45
sync::{Arc, Mutex, OnceLock},
56
thread,
67
};
@@ -12,12 +13,19 @@ use crate::geo::ScreenSize;
1213

1314
type ChildWaitResult = Result<ExitStatus, Arc<std::io::Error>>;
1415

16+
// On musl libc (Alpine Linux), concurrent PTY operations trigger
17+
// SIGSEGV/SIGBUS in musl internals (sysconf, fcntl). This affects
18+
// openpty+fork, FD cleanup (close), and FD drops from any thread.
19+
// Serialize all PTY lifecycle operations that touch musl internals.
20+
#[cfg(target_env = "musl")]
21+
static PTY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
22+
1523
/// The read half of a PTY connection. Implements [`Read`].
1624
///
1725
/// Reading feeds data through an internal vt100 parser (shared with [`PtyWriter`]),
1826
/// keeping `screen_contents()` up-to-date with parsed terminal output.
1927
pub struct PtyReader {
20-
reader: Box<dyn Read + Send>,
28+
reader: ManuallyDrop<Box<dyn Read + Send>>,
2129
parser: Arc<Mutex<vt100::Parser<Vt100Callbacks>>>,
2230
}
2331

@@ -28,7 +36,25 @@ pub struct PtyReader {
2836
pub struct PtyWriter {
2937
writer: Arc<Mutex<Option<Box<dyn Write + Send>>>>,
3038
parser: Arc<Mutex<vt100::Parser<Vt100Callbacks>>>,
31-
master: Box<dyn MasterPty + Send>,
39+
master: ManuallyDrop<Box<dyn MasterPty + Send>>,
40+
}
41+
42+
impl Drop for PtyReader {
43+
fn drop(&mut self) {
44+
#[cfg(target_env = "musl")]
45+
let _guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
46+
// SAFETY: called exactly once, from drop.
47+
unsafe { ManuallyDrop::drop(&mut self.reader) };
48+
}
49+
}
50+
51+
impl Drop for PtyWriter {
52+
fn drop(&mut self) {
53+
#[cfg(target_env = "musl")]
54+
let _guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
55+
// SAFETY: called exactly once, from drop.
56+
unsafe { ManuallyDrop::drop(&mut self.master) };
57+
}
3258
}
3359

3460
/// A cloneable handle to a child process spawned in a PTY.
@@ -256,12 +282,6 @@ impl Terminal {
256282
///
257283
/// Panics if the writer lock is poisoned when the background thread closes it.
258284
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(());
265285
#[cfg(target_env = "musl")]
266286
let _spawn_guard = PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner());
267287

@@ -316,8 +336,11 @@ impl Terminal {
316336
)));
317337

318338
Ok(Self {
319-
pty_reader: PtyReader { reader, parser: Arc::clone(&parser) },
320-
pty_writer: PtyWriter { writer, parser, master },
339+
pty_reader: PtyReader {
340+
reader: ManuallyDrop::new(reader),
341+
parser: Arc::clone(&parser),
342+
},
343+
pty_writer: PtyWriter { writer, parser, master: ManuallyDrop::new(master) },
321344
child_handle: ChildHandle { child_killer, exit_status },
322345
})
323346
}

0 commit comments

Comments
 (0)