Skip to content
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 95 additions & 8 deletions src/cortex-tui/src/sound.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@
//!
//! On platforms without audio support (e.g., musl builds), falls back to
//! terminal bell notifications.
//!
//! On Linux, ALSA error messages (e.g., "cannot find card 0") are suppressed
//! during audio initialization to avoid noisy output on headless systems.

#[cfg(feature = "audio")]
use std::io::Cursor;
Expand Down Expand Up @@ -44,6 +47,96 @@ const COMPLETE_WAV: &[u8] = include_bytes!("sounds/complete.wav");
#[cfg(feature = "audio")]
const APPROVAL_WAV: &[u8] = include_bytes!("sounds/approval.wav");

/// Try to create audio output stream, suppressing ALSA errors on Linux.
///
/// On Linux, ALSA prints error messages directly to stderr when no audio
/// hardware is available (e.g., "ALSA lib confmisc.c: cannot find card 0").
/// This function suppresses those messages by temporarily redirecting stderr
/// to /dev/null during initialization.
#[cfg(all(feature = "audio", target_os = "linux"))]
fn try_create_output_stream() -> Option<(rodio::OutputStream, rodio::OutputStreamHandle)> {
use std::os::unix::io::AsRawFd;

// Open /dev/null for redirecting stderr
let dev_null = match std::fs::File::open("/dev/null") {
Ok(f) => f,
Err(_) => {
// Can't open /dev/null, try without suppression
return match rodio::OutputStream::try_default() {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
};
}
};

// Save the original stderr file descriptor
// SAFETY: dup is safe to call with a valid file descriptor (2 = stderr)
let original_stderr = unsafe { libc::dup(2) };
if original_stderr == -1 {
// dup failed, try without suppression
return match rodio::OutputStream::try_default() {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
};
}

// Redirect stderr to /dev/null
// SAFETY: dup2 is safe with valid file descriptors
let redirect_result = unsafe { libc::dup2(dev_null.as_raw_fd(), 2) };
drop(dev_null); // Close our handle to /dev/null

if redirect_result == -1 {
// dup2 failed, restore and try without suppression
// SAFETY: close is safe with a valid file descriptor
unsafe { libc::close(original_stderr) };
return match rodio::OutputStream::try_default() {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
};
Comment on lines +105 to +120
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Global stderr redirection race

This temporarily redirects the process-wide fd 2 in the audio thread (src/cortex-tui/src/sound.rs:89-104). In a multi-threaded CLI, any concurrent writes to stderr (including from other libraries) during this window will be dropped, which can hide unrelated errors. To avoid losing diagnostics, consider performing the redirection as early as possible (before starting other threads) or gating it behind a mutex/global guard so only one thread can run it while other stderr writes are paused (or document the trade-off explicitly if you’re accepting it).

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-tui/src/sound.rs
Line: 89:104

Comment:
**Global stderr redirection race**

This temporarily redirects the *process-wide* fd 2 in the audio thread (`src/cortex-tui/src/sound.rs:89-104`). In a multi-threaded CLI, any concurrent writes to stderr (including from other libraries) during this window will be dropped, which can hide unrelated errors. To avoid losing diagnostics, consider performing the redirection as early as possible (before starting other threads) or gating it behind a mutex/global guard so only one thread can run it while other stderr writes are paused (or document the trade-off explicitly if you’re accepting it).

How can I resolve this? If you propose a fix, please make it concise.

}

// Try to create the audio output stream (ALSA errors will go to /dev/null)
let result = rodio::OutputStream::try_default();

// Restore the original stderr
// SAFETY: dup2 and close are safe with valid file descriptors
unsafe {
libc::dup2(original_stderr, 2);
libc::close(original_stderr);
}
Comment on lines +123 to +144
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stderr restoration unchecked

After redirecting stderr, the restore path ignores return values from libc::dup2(original_stderr, 2) and libc::close(original_stderr) (src/cortex-tui/src/sound.rs:112-115). If either fails (e.g., dup2 gets EBADF/EINTR), stderr can remain redirected for the rest of the process, which is a behavior change outside audio init. At minimum, check dup2’s return value and, on failure, log (and avoid closing original_stderr if you still need it).

Also appears in the redirect path where dup2(dev_null, 2) failure is handled but the restore attempt isn’t re-validated.

Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cortex-tui/src/sound.rs
Line: 107:115

Comment:
**Stderr restoration unchecked**

After redirecting stderr, the restore path ignores return values from `libc::dup2(original_stderr, 2)` and `libc::close(original_stderr)` (`src/cortex-tui/src/sound.rs:112-115`). If either fails (e.g., `dup2` gets `EBADF`/`EINTR`), stderr can remain redirected for the rest of the process, which is a behavior change outside audio init. At minimum, check `dup2`’s return value and, on failure, log (and avoid closing `original_stderr` if you still need it).

Also appears in the redirect path where `dup2(dev_null, 2)` failure is handled but the restore attempt isn’t re-validated.

How can I resolve this? If you propose a fix, please make it concise.


match result {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
}
}

/// Try to create audio output stream (non-Linux platforms).
///
/// On non-Linux platforms, ALSA is not used, so no stderr suppression is needed.
#[cfg(all(feature = "audio", not(target_os = "linux")))]
fn try_create_output_stream() -> Option<(rodio::OutputStream, rodio::OutputStreamHandle)> {
match rodio::OutputStream::try_default() {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
}
}

/// Initialize the global sound system.
/// Spawns a dedicated audio thread that owns the OutputStream.
/// Should be called once at application startup.
Expand All @@ -67,14 +160,8 @@ pub fn init() {
thread::Builder::new()
.name("cortex-audio".to_string())
.spawn(move || {
// Try to create audio output
let output = match rodio::OutputStream::try_default() {
Ok((stream, handle)) => Some((stream, handle)),
Err(e) => {
tracing::debug!("Failed to initialize audio output: {}", e);
None
}
};
// Try to create audio output (with ALSA error suppression on Linux)
let output = try_create_output_stream();

// Process sound requests
while let Ok(sound_type) = rx.recv() {
Expand Down