From 20b3f229f5e73570233759b2a1c2a2b12fc6febd Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 17 Apr 2026 10:05:02 +0200 Subject: [PATCH 1/2] shadow-rs: replace println!/eprintln! with non-panicking writes (#141) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit println!() and eprintln!() panic when stdout/stderr is full or closed (e.g., `command >/dev/full`). Replace all production-code occurrences with non-panicking alternatives: - show_error!/show_warning! macros rewritten to use `let _ = writeln!(stderr())` instead of eprintln! - Normal output uses `let _ = writeln!(stdout)` with locked handles - PAM display_message uses `let _ = writeln!(stderr())` Also fixes a pre-existing clippy::map_unwrap_or lint in atomic.rs. Test code is intentionally left unchanged — panicking on write failure in tests is acceptable and desirable for fast failure. Fixes #141. --- src/bin/completions.rs | 9 ++++-- src/bin/shadow-rs.rs | 52 ++++++++++++++++++++++------------- src/shadow-core/src/atomic.rs | 6 ++-- src/shadow-core/src/error.rs | 14 ++++++++-- src/shadow-core/src/pam.rs | 5 +++- src/uu/chage/src/chage.rs | 25 ++++++++++++----- src/uu/chsh/src/chsh.rs | 5 ++-- src/uu/passwd/src/passwd.rs | 9 ++++-- src/uu/pwck/src/pwck.rs | 16 +++++++---- src/uu/useradd/src/useradd.rs | 16 ++++++----- 10 files changed, 105 insertions(+), 52 deletions(-) diff --git a/src/bin/completions.rs b/src/bin/completions.rs index 98ff357..cacb254 100644 --- a/src/bin/completions.rs +++ b/src/bin/completions.rs @@ -19,6 +19,7 @@ use clap::{Arg, ArgAction, Command}; use clap_complete::Shell; use clap_complete::generate; use std::io; +use std::io::Write as _; fn get_tool_app(name: &str) -> Option { match name { @@ -144,7 +145,7 @@ fn main() -> std::process::ExitCode { match run() { Ok(()) => std::process::ExitCode::SUCCESS, Err(msg) => { - eprintln!("{msg}"); + let _ = writeln!(std::io::stderr(), "{msg}"); std::process::ExitCode::FAILURE } } @@ -175,7 +176,11 @@ fn run() -> Result<(), String> { generate(shell, &mut cmd, *name, &mut file); } } - eprintln!("generated {} completions in {dir}/", tools.len()); + let _ = writeln!( + std::io::stderr(), + "generated {} completions in {dir}/", + tools.len() + ); } else { let stdout = io::stdout(); let mut out = stdout.lock(); diff --git a/src/bin/shadow-rs.rs b/src/bin/shadow-rs.rs index e538800..9ddd65e 100644 --- a/src/bin/shadow-rs.rs +++ b/src/bin/shadow-rs.rs @@ -8,6 +8,7 @@ //! Dispatches to the appropriate utility based on `argv[0]`. //! When invoked as `shadow-rs `, uses the first argument instead. +use std::io::Write; use std::path::Path; use std::process::ExitCode; @@ -47,13 +48,25 @@ fn main() -> ExitCode { return to_exit_code(code); } - eprintln!("shadow-rs: unknown utility '{util_name}'"); - eprintln!("Run 'shadow-rs --list' for available utilities."); + let _ = writeln!( + std::io::stderr(), + "shadow-rs: unknown utility '{util_name}'" + ); + let _ = writeln!( + std::io::stderr(), + "Run 'shadow-rs --list' for available utilities." + ); return ExitCode::FAILURE; } - eprintln!("Usage: shadow-rs [arguments...]"); - eprintln!("Run 'shadow-rs --list' for available utilities."); + let _ = writeln!( + std::io::stderr(), + "Usage: shadow-rs [arguments...]" + ); + let _ = writeln!( + std::io::stderr(), + "Run 'shadow-rs --list' for available utilities." + ); ExitCode::FAILURE } @@ -92,34 +105,35 @@ fn dispatch(name: &str, args: &[std::ffi::OsString]) -> Option { } fn print_available_utils() { - println!("Available utilities:"); + let mut out = std::io::stdout().lock(); + let _ = writeln!(out, "Available utilities:"); #[cfg(feature = "chage")] - println!(" chage"); + let _ = writeln!(out, " chage"); #[cfg(feature = "chfn")] - println!(" chfn"); + let _ = writeln!(out, " chfn"); #[cfg(feature = "chpasswd")] - println!(" chpasswd"); + let _ = writeln!(out, " chpasswd"); #[cfg(feature = "chsh")] - println!(" chsh"); + let _ = writeln!(out, " chsh"); #[cfg(feature = "groupadd")] - println!(" groupadd"); + let _ = writeln!(out, " groupadd"); #[cfg(feature = "groupdel")] - println!(" groupdel"); + let _ = writeln!(out, " groupdel"); #[cfg(feature = "groupmod")] - println!(" groupmod"); + let _ = writeln!(out, " groupmod"); #[cfg(feature = "grpck")] - println!(" grpck"); + let _ = writeln!(out, " grpck"); #[cfg(feature = "newgrp")] - println!(" newgrp"); + let _ = writeln!(out, " newgrp"); #[cfg(feature = "passwd")] - println!(" passwd"); + let _ = writeln!(out, " passwd"); #[cfg(feature = "pwck")] - println!(" pwck"); + let _ = writeln!(out, " pwck"); #[cfg(feature = "useradd")] - println!(" useradd"); + let _ = writeln!(out, " useradd"); #[cfg(feature = "userdel")] - println!(" userdel"); + let _ = writeln!(out, " userdel"); #[cfg(feature = "usermod")] - println!(" usermod"); + let _ = writeln!(out, " usermod"); } diff --git a/src/shadow-core/src/atomic.rs b/src/shadow-core/src/atomic.rs index 9460b9a..ecb6cec 100644 --- a/src/shadow-core/src/atomic.rs +++ b/src/shadow-core/src/atomic.rs @@ -101,9 +101,9 @@ where // Determine permissions: preserve original if target exists, otherwise 0600. // Set mode at creation time to avoid any window where the file is world-readable. - let mode = fs::metadata(target) - .map(|m| std::os::unix::fs::PermissionsExt::mode(&m.permissions())) - .unwrap_or(0o600); + let mode = fs::metadata(target).map_or(0o600, |m| { + std::os::unix::fs::PermissionsExt::mode(&m.permissions()) + }); let mut guard = TmpGuard::new(tmp_path.clone()); diff --git a/src/shadow-core/src/error.rs b/src/shadow-core/src/error.rs index 0cd2f92..bfac5c1 100644 --- a/src/shadow-core/src/error.rs +++ b/src/shadow-core/src/error.rs @@ -47,7 +47,12 @@ pub enum ShadowError { #[macro_export] macro_rules! show_error { ($util:expr, $($arg:tt)*) => { - eprintln!("{}: {}", $util, format_args!($($arg)*)); + { + use std::io::Write as _; + let mut err = std::io::stderr().lock(); + let _ = write!(err, "{}: ", $util); + let _ = writeln!(err, $($arg)*); + } }; } @@ -55,6 +60,11 @@ macro_rules! show_error { #[macro_export] macro_rules! show_warning { ($util:expr, $($arg:tt)*) => { - eprintln!("{}: warning: {}", $util, format_args!($($arg)*)); + { + use std::io::Write as _; + let mut err = std::io::stderr().lock(); + let _ = write!(err, "{}: warning: ", $util); + let _ = writeln!(err, $($arg)*); + } }; } diff --git a/src/shadow-core/src/pam.rs b/src/shadow-core/src/pam.rs index e5eb255..82e65dd 100644 --- a/src/shadow-core/src/pam.rs +++ b/src/shadow-core/src/pam.rs @@ -338,7 +338,10 @@ fn display_message(message: &PamMessage, _is_error: bool) { let text = unsafe { CStr::from_ptr(message.msg) }; let text = text.to_string_lossy(); - eprintln!("{text}"); + { + use std::io::Write as _; + let _ = writeln!(std::io::stderr(), "{text}"); + } } /// Prompt for user input (with or without echo) and return a `malloc`-allocated diff --git a/src/uu/chage/src/chage.rs b/src/uu/chage/src/chage.rs index 635e96c..79f2322 100644 --- a/src/uu/chage/src/chage.rs +++ b/src/uu/chage/src/chage.rs @@ -9,6 +9,7 @@ //! Drop-in replacement for GNU shadow-utils `chage(1)`. use std::fmt; +use std::io::Write as _; use std::path::Path; use clap::{Arg, ArgAction, Command}; @@ -460,13 +461,23 @@ fn print_aging_info(entry: &ShadowEntry) { .warn_days .map_or_else(|| "-1".to_string(), |v| v.to_string()); - println!("Last password change\t\t\t\t\t: {last_change}"); - println!("Password expires\t\t\t\t\t: {password_expires}"); - println!("Password inactive\t\t\t\t\t: {password_inactive}"); - println!("Account expires\t\t\t\t\t\t: {account_expires}"); - println!("Minimum number of days between password change\t\t: {min_days}"); - println!("Maximum number of days between password change\t\t: {max_days}"); - println!("Number of days of warning before password expires\t: {warn_days}"); + let mut out = std::io::stdout().lock(); + let _ = writeln!(out, "Last password change\t\t\t\t\t: {last_change}"); + let _ = writeln!(out, "Password expires\t\t\t\t\t: {password_expires}"); + let _ = writeln!(out, "Password inactive\t\t\t\t\t: {password_inactive}"); + let _ = writeln!(out, "Account expires\t\t\t\t\t\t: {account_expires}"); + let _ = writeln!( + out, + "Minimum number of days between password change\t\t: {min_days}" + ); + let _ = writeln!( + out, + "Maximum number of days between password change\t\t: {max_days}" + ); + let _ = writeln!( + out, + "Number of days of warning before password expires\t: {warn_days}" + ); } /// Compute the password expiry display string. diff --git a/src/uu/chsh/src/chsh.rs b/src/uu/chsh/src/chsh.rs index f03be1c..ff5aeb0 100644 --- a/src/uu/chsh/src/chsh.rs +++ b/src/uu/chsh/src/chsh.rs @@ -10,7 +10,7 @@ //! Changes the login shell field in `/etc/passwd`. use std::fmt; -use std::io::{self, BufRead}; +use std::io::{self, BufRead, Write as _}; use std::path::Path; use clap::{Arg, ArgAction, Command}; @@ -264,8 +264,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if shells.is_empty() { uucore::show_error!("no shells found in {}", root.shells_path().display()); } else { + let mut out = io::stdout().lock(); for shell in &shells { - println!("{shell}"); + let _ = writeln!(out, "{shell}"); } } return Ok(()); diff --git a/src/uu/passwd/src/passwd.rs b/src/uu/passwd/src/passwd.rs index 27b8b0f..1b3972c 100644 --- a/src/uu/passwd/src/passwd.rs +++ b/src/uu/passwd/src/passwd.rs @@ -9,6 +9,7 @@ //! Drop-in replacement for GNU shadow-utils `passwd(1)`. use std::fmt; +use std::io::Write as _; use std::path::Path; use clap::{Arg, ArgAction, Command}; @@ -444,6 +445,7 @@ fn cmd_status(root: &SysRoot, target_user: Option<&str>) -> UResult<()> { } }; + let mut out = std::io::stdout().lock(); match target_user { Some(user) => { let Some(entry) = entries.iter().find(|e| e.name == user) else { @@ -453,12 +455,12 @@ fn cmd_status(root: &SysRoot, target_user: Option<&str>) -> UResult<()> { )) .into()); }; - println!("{}", format_status(entry)); + let _ = writeln!(out, "{}", format_status(entry)); } None => { // --all: show all users. for entry in &entries { - println!("{}", format_status(entry)); + let _ = writeln!(out, "{}", format_status(entry)); } } } @@ -499,7 +501,8 @@ impl Drop for PrivDrop { if let Err(e) = nix::unistd::seteuid(self.original_euid) { // Failing to restore privileges is a critical error — log it loudly. // We can't return an error from Drop, so at least make it visible. - eprintln!( + let _ = writeln!( + std::io::stderr(), "passwd: CRITICAL: failed to restore euid to {}: {e}", self.original_euid ); diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 40e5394..9b1a995 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/src/uu/pwck/src/pwck.rs @@ -10,7 +10,7 @@ use std::collections::HashSet; use std::fmt; -use std::io::BufRead; +use std::io::{BufRead, Write as _}; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; @@ -203,7 +203,7 @@ fn run_checks(opts: &PwckOptions) -> UResult<()> { opts.read_only, )?; } else { - eprintln!("pwck: no changes"); + uucore::show_error!("no changes"); } if errors > 0 { @@ -426,9 +426,11 @@ fn check_passwd_entries( PathBuf::from(&entry.home) }; if !home_path.exists() { - eprintln!( + let _ = writeln!( + std::io::stderr(), "user '{}': directory '{}' does not exist", - entry.name, entry.home + entry.name, + entry.home ); errors += 1; } @@ -450,9 +452,11 @@ fn check_passwd_entries( && !valid_shells.contains(Path::new(&entry.shell)) && !shell_path.exists() { - eprintln!( + let _ = writeln!( + std::io::stderr(), "user '{}': program '{}' does not exist", - entry.name, entry.shell + entry.name, + entry.shell ); errors += 1; } diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index d766bb7..1ef3ba3 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -14,6 +14,7 @@ //! directory and populate it from `/etc/skel`. use std::fmt; +use std::io::Write as _; use std::os::unix::fs::PermissionsExt; use std::path::Path; @@ -320,13 +321,14 @@ fn cmd_defaults(_matches: &clap::ArgMatches) -> UResult<()> { let default_skel = defs.get("SKEL").unwrap_or("/etc/skel"); let default_create_mail = defs.get("CREATE_MAIL_SPOOL").unwrap_or("no"); - println!("GROUP=100"); - println!("HOME={default_home}"); - println!("INACTIVE={default_inactive}"); - println!("EXPIRE={default_expire}"); - println!("SHELL={default_shell}"); - println!("SKEL={default_skel}"); - println!("CREATE_MAIL_SPOOL={default_create_mail}"); + let mut out = std::io::stdout().lock(); + let _ = writeln!(out, "GROUP=100"); + let _ = writeln!(out, "HOME={default_home}"); + let _ = writeln!(out, "INACTIVE={default_inactive}"); + let _ = writeln!(out, "EXPIRE={default_expire}"); + let _ = writeln!(out, "SHELL={default_shell}"); + let _ = writeln!(out, "SKEL={default_skel}"); + let _ = writeln!(out, "CREATE_MAIL_SPOOL={default_create_mail}"); Ok(()) } From ab4ed5977c6f1ecfd0e9e9c3c8388fcea6b1414f Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 17 Apr 2026 10:59:33 +0200 Subject: [PATCH 2/2] shadow-rs: optimize stderr writes per Copilot review - show_error!/show_warning!: combine prefix + message into a single writeln! call (was two writes: write! then writeln!) - pam display_message: lock stderr before writing - passwd PrivDrop::Drop: lock stderr before writing - pwck check_passwd_entries: hoist stderr lock above the loop so per-entry diagnostics reuse one locked handle instead of reconstructing it on each iteration --- src/shadow-core/src/error.rs | 8 ++------ src/shadow-core/src/pam.rs | 2 +- src/uu/passwd/src/passwd.rs | 2 +- src/uu/pwck/src/pwck.rs | 11 +++++------ 4 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/shadow-core/src/error.rs b/src/shadow-core/src/error.rs index bfac5c1..3571664 100644 --- a/src/shadow-core/src/error.rs +++ b/src/shadow-core/src/error.rs @@ -49,9 +49,7 @@ macro_rules! show_error { ($util:expr, $($arg:tt)*) => { { use std::io::Write as _; - let mut err = std::io::stderr().lock(); - let _ = write!(err, "{}: ", $util); - let _ = writeln!(err, $($arg)*); + let _ = writeln!(std::io::stderr().lock(), "{}: {}", $util, format_args!($($arg)*)); } }; } @@ -62,9 +60,7 @@ macro_rules! show_warning { ($util:expr, $($arg:tt)*) => { { use std::io::Write as _; - let mut err = std::io::stderr().lock(); - let _ = write!(err, "{}: warning: ", $util); - let _ = writeln!(err, $($arg)*); + let _ = writeln!(std::io::stderr().lock(), "{}: warning: {}", $util, format_args!($($arg)*)); } }; } diff --git a/src/shadow-core/src/pam.rs b/src/shadow-core/src/pam.rs index 82e65dd..ebcb5e8 100644 --- a/src/shadow-core/src/pam.rs +++ b/src/shadow-core/src/pam.rs @@ -340,7 +340,7 @@ fn display_message(message: &PamMessage, _is_error: bool) { { use std::io::Write as _; - let _ = writeln!(std::io::stderr(), "{text}"); + let _ = writeln!(std::io::stderr().lock(), "{text}"); } } diff --git a/src/uu/passwd/src/passwd.rs b/src/uu/passwd/src/passwd.rs index 1b3972c..9f14060 100644 --- a/src/uu/passwd/src/passwd.rs +++ b/src/uu/passwd/src/passwd.rs @@ -502,7 +502,7 @@ impl Drop for PrivDrop { // Failing to restore privileges is a critical error — log it loudly. // We can't return an error from Drop, so at least make it visible. let _ = writeln!( - std::io::stderr(), + std::io::stderr().lock(), "passwd: CRITICAL: failed to restore euid to {}: {e}", self.original_euid ); diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 9b1a995..88c187d 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/src/uu/pwck/src/pwck.rs @@ -373,6 +373,7 @@ fn check_passwd_entries( let mut seen_names: HashSet<&str> = HashSet::new(); let group_gids: HashSet = group_entries.iter().map(|g| g.gid).collect(); let shadow_names: HashSet<&str> = shadow_entries.iter().map(|s| s.name.as_str()).collect(); + let mut stderr = std::io::stderr().lock(); for entry in passwd_entries { // Check 2: Unique and valid usernames. @@ -427,10 +428,9 @@ fn check_passwd_entries( }; if !home_path.exists() { let _ = writeln!( - std::io::stderr(), + stderr, "user '{}': directory '{}' does not exist", - entry.name, - entry.home + entry.name, entry.home ); errors += 1; } @@ -453,10 +453,9 @@ fn check_passwd_entries( && !shell_path.exists() { let _ = writeln!( - std::io::stderr(), + stderr, "user '{}': program '{}' does not exist", - entry.name, - entry.shell + entry.name, entry.shell ); errors += 1; }