From d0dc9e7f820fa68cea1bf0cab020adb65abb0f1d Mon Sep 17 00:00:00 2001 From: Pierre Warnier Date: Fri, 4 Sep 2026 17:50:19 +0200 Subject: [PATCH] shadow-core: commit several files together, and convert the last six tools group and gshadow have to agree: a group present in one and absent from the other is a broken system, and every tool that touches a group touches both. Committing them one at a time leaves a window in which they disagree, and a failure in the second makes that permanent. commit_all validates every file before writing any, which closes the failure this actually hits -- a value that would corrupt a record is rejected while nothing has been touched, instead of after the first file is already on disk. A genuine I/O error partway through can still leave the set half applied; there is no journal, and a rollback that can itself fail would not be an improvement. All the locks are held until the last write finishes, so no other process sees the intermediate state. A commit that would write the same bytes now writes nothing. A rewrite is not free: it replaces the inode and moves the mtime, and for an unchanged empty file it failed outright, since the atomic writer refuses to produce a zero-length file. Every tool used to carry its own 'did anything change' flag to work around that; those are gone. Found by usermod's tests, whose fixture has an empty gshadow that -G does not touch. groupmod, useradd, usermod, userdel, pwck and grpck are converted, and there is no longer a single FileLock::acquire left in any tool. userdel loses its hand-rolled line filter, which had its own idea of which lines were comments next to the parser that already knows; one consequence is that a comment above a removed account now moves to the end of the file rather than staying in place, which is what every other tool already did. Exit codes are preserved, including the two pwck keeps for a failed write: 6 'can not sort' and 5 'can not update the files' stay distinct by matching on the file the error names. Verified against a prefix tree for each tool: useradd creating an account with a user group, supplementary groups, aging and subid; usermod renaming across four files and replacing a group list; groupmod moving a GID and following it into passwd; userdel removing an account from six files; grpck sorting group and gshadow together, keeping each comment with its group, and writing nothing on a second run. --- src/shadow-core/src/transaction.rs | 294 +++++++++++++++++++++--- src/uu/groupmod/src/groupmod.rs | 163 +++++--------- src/uu/grpck/src/grpck.rs | 69 ++---- src/uu/pwck/src/pwck.rs | 79 +++---- src/uu/useradd/src/useradd.rs | 346 +++++++++-------------------- src/uu/userdel/src/userdel.rs | 221 +++++------------- src/uu/usermod/src/usermod.rs | 147 +++++------- 7 files changed, 601 insertions(+), 718 deletions(-) diff --git a/src/shadow-core/src/transaction.rs b/src/shadow-core/src/transaction.rs index fdf7bf6..69b0530 100644 --- a/src/shadow-core/src/transaction.rs +++ b/src/shadow-core/src/transaction.rs @@ -22,6 +22,7 @@ //! lock file behind, which every later run has to wait out. use std::fmt::Display; +use std::io::Write as _; use std::path::{Path, PathBuf}; use std::str::FromStr; @@ -54,6 +55,9 @@ pub struct LockedFile { lock: Option, entries: Vec, layout: Layout, + /// The file's contents as read, so a commit that would write the same + /// bytes can write nothing at all. + original: Vec, /// Restores the signal mask when the transaction ends, whichever way. _signals: SignalBlocker, } @@ -75,11 +79,13 @@ impl LockedFile { let lock = FileLock::acquire(path)?; // On any failure from here on, `lock` drops and the file is untouched. let (entries, layout) = records::read_with_layout::(path)?; + let original = std::fs::read(path).unwrap_or_default(); Ok(Self { path: path.to_owned(), lock: Some(lock), entries, layout, + original, _signals: signals, }) } @@ -107,11 +113,13 @@ impl LockedFile { } else { (Vec::new(), Layout::default()) }; + let original = std::fs::read(path).unwrap_or_default(); Ok(Self { path: path.to_owned(), lock: Some(lock), entries, layout, + original, _signals: signals, }) } @@ -155,25 +163,8 @@ impl LockedFile { /// Returns `ShadowError::Validation` if an entry holds a value that would /// corrupt the record -- in which case nothing is written -- and /// `ShadowError::IoPath` if the write fails. - pub fn commit(mut self) -> Result<(), ShadowError> { - let entries = &self.entries; - let layout = &self.layout; - let result = atomic::atomic_write(&self.path, |mut file| { - // `write_with_layout` takes `&mut W`, and `atomic_write` hands over - // a `&mut dyn Write`; borrowing it again makes `W` the fat pointer, - // which is sized, rather than the unsized `dyn Write`. - records::write_with_layout(entries, layout, &mut file, |entry, w| { - entry.validate_fields()?; - writeln!(w, "{entry}")?; - Ok(()) - }) - }); - - // Release before returning either way: an error path that held the - // lock until the process exited would block every concurrent tool for - // as long as the caller took to report it. - drop(self.lock.take()); - result + pub fn commit(self) -> Result<(), ShadowError> { + self.commit_or_remove_if(false) } /// Like [`LockedFile::commit`], but remove the file when nothing is left @@ -189,28 +180,70 @@ impl LockedFile { /// state anyone wants to reach by accident; those use `commit`. /// /// The file is only removed when it carried no comments or other preserved - /// lines either. A file that is all comments still says something. + /// lines either. A file that is all comments still says something. A file + /// that was already empty is left alone, since nothing changed. /// /// # Errors /// /// As [`LockedFile::commit`], plus `ShadowError::IoPath` if the file /// cannot be removed. - pub fn commit_or_remove(mut self) -> Result<(), ShadowError> { - if !self.entries.is_empty() || !self.layout.is_empty() { - return self.commit(); - } - let result = std::fs::remove_file(&self.path).or_else(|e| { - // Already gone is the state we wanted. - if e.kind() == std::io::ErrorKind::NotFound { - Ok(()) - } else { - Err(ShadowError::IoPath(e, self.path.clone())) - } - }); + pub fn commit_or_remove(self) -> Result<(), ShadowError> { + self.commit_or_remove_if(true) + } + + /// Render the entries, validating each one. + /// + /// Into a buffer rather than straight to the file, so a value that would + /// corrupt a record is caught while the file is still untouched. + fn render(&self) -> Result, ShadowError> { + let mut out = Vec::with_capacity(self.original.len()); + records::write_with_layout(&self.entries, &self.layout, &mut out, |entry, w| { + entry.validate_fields()?; + writeln!(w, "{entry}")?; + Ok(()) + })?; + Ok(out) + } + + /// Write the file, optionally unlinking it when nothing is left. + fn commit_or_remove_if(mut self, remove_when_empty: bool) -> Result<(), ShadowError> { + let result = self.write(remove_when_empty); + // Release either way: an error path that held the lock until the + // process exited would block every concurrent tool for as long as the + // caller took to report it. drop(self.lock.take()); result } + fn write(&mut self, remove_when_empty: bool) -> Result<(), ShadowError> { + let rendered = self.render()?; + + // Nothing changed: do not rewrite the file. A rewrite is not free -- + // it replaces the inode, moves the mtime, and for an unchanged empty + // file it would fail outright, since the atomic writer refuses to + // produce a zero-length file. Tools that only *might* change a file + // used to guard every write with their own "did anything change" flag. + if rendered == self.original { + return Ok(()); + } + + if remove_when_empty && self.entries.is_empty() && self.layout.is_empty() { + return std::fs::remove_file(&self.path).or_else(|e| { + // Already gone is the state we wanted. + if e.kind() == std::io::ErrorKind::NotFound { + Ok(()) + } else { + Err(ShadowError::IoPath(e, self.path.clone())) + } + }); + } + + atomic::atomic_write(&self.path, |file| { + file.write_all(&rendered)?; + Ok(()) + }) + } + /// Release the lock and discard the changes. /// /// The same as dropping the value; useful where the intent is worth @@ -227,6 +260,81 @@ impl Drop for LockedFile { } } +// --------------------------------------------------------------------------- +// Committing several files together +// --------------------------------------------------------------------------- + +/// A locked file that can be validated and committed without the caller +/// knowing which record type it holds. +/// +/// [`commit_all`] needs a heterogeneous list -- `/etc/group` and +/// `/etc/gshadow` hold different types and have to be written together -- so +/// the operations it needs are behind a trait object. +pub trait Commit { + /// Everything that can be checked before any file is touched. + /// + /// # Errors + /// + /// Returns `ShadowError::Validation` naming the offending field. + fn validate(&self) -> Result<(), ShadowError>; + + /// Write this file and release its lock. + /// + /// # Errors + /// + /// Returns `ShadowError::IoPath` if the write fails. + fn commit_boxed(self: Box) -> Result<(), ShadowError>; + + /// The file this will write. + fn path(&self) -> &Path; +} + +impl Commit for LockedFile { + fn validate(&self) -> Result<(), ShadowError> { + self.render().map(|_| ()) + } + + fn commit_boxed(self: Box) -> Result<(), ShadowError> { + (*self).commit_or_remove_if(false) + } + + fn path(&self) -> &Path { + &self.path + } +} + +/// Commit several locked files, or none of them. +/// +/// `/etc/group` and `/etc/gshadow` have to agree: a group present in one and +/// absent from the other is a broken system, and every tool that touches a +/// group touches both. Committing them one at a time leaves a window in which +/// they disagree, and a failure in the second makes that permanent. +/// +/// **Every file is validated before any is written.** That closes the failure +/// this actually hits: a value that would corrupt a record is rejected while +/// nothing has been touched, instead of after the first file is already on +/// disk. A genuine I/O error partway through the writes can still leave the +/// set half applied; there is no journal, and a rollback that can itself fail +/// would not be an improvement. The window is reduced to the writes +/// themselves, each of which is a rename onto an already-fsynced file. +/// +/// The locks are all held until the last write finishes, so no other process +/// sees the intermediate state. +/// +/// # Errors +/// +/// Returns the first `ShadowError` from validation -- with nothing written -- +/// or from a write, naming the file it failed on. +pub fn commit_all(files: Vec>) -> Result<(), ShadowError> { + for file in &files { + file.validate()?; + } + for file in files { + file.commit_boxed()?; + } + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -344,4 +452,126 @@ mod tests { let (_d, path) = temp_passwd("alice:x:1000:1000::/home/alice:/bin/sh\nnot-a-record\n"); assert!(LockedFile::::open(&path).is_err()); } + + // ----------------------------------------------------------------------- + // Committing several files together + // ----------------------------------------------------------------------- + + use crate::group::GroupEntry; + + fn temp_group(dir: &Path, content: &str) -> PathBuf { + let path = dir.join("group"); + std::fs::write(&path, content).expect("write"); + path + } + + const GROUP: &str = "staff:x:1000:alice\nadmin:x:1001:\n"; + + #[test] + fn test_commit_all_writes_every_file() { + let dir = tempfile::tempdir().expect("tempdir"); + let passwd_path = dir.path().join("passwd"); + std::fs::write(&passwd_path, TWO).expect("write"); + let group_path = temp_group(dir.path(), GROUP); + + let mut passwd = LockedFile::::open(&passwd_path).expect("open passwd"); + let mut group = LockedFile::::open(&group_path).expect("open group"); + passwd.find_mut("alice").expect("alice").shell = "/bin/bash".into(); + group.find_mut("staff").expect("staff").members = vec!["alice".into(), "bob".into()]; + + commit_all(vec![Box::new(passwd), Box::new(group)]).expect("commit_all"); + + assert!( + std::fs::read_to_string(&passwd_path) + .expect("read") + .contains("/bin/bash") + ); + assert!( + std::fs::read_to_string(&group_path) + .expect("read") + .contains("staff:x:1000:alice,bob") + ); + } + + /// The failure this exists to prevent: a value that would corrupt one file + /// must stop the set before any of it is written, not after the first file + /// is already on disk. + #[test] + fn test_a_bad_value_in_the_second_file_writes_neither() { + let dir = tempfile::tempdir().expect("tempdir"); + let passwd_path = dir.path().join("passwd"); + std::fs::write(&passwd_path, TWO).expect("write"); + let group_path = temp_group(dir.path(), GROUP); + + let mut passwd = LockedFile::::open(&passwd_path).expect("open passwd"); + let mut group = LockedFile::::open(&group_path).expect("open group"); + passwd.find_mut("alice").expect("alice").shell = "/bin/bash".into(); + // A separator in a group name would shift every following field. + group.find_mut("admin").expect("admin").name = "ad:min".into(); + + assert!(commit_all(vec![Box::new(passwd), Box::new(group)]).is_err()); + assert_eq!( + std::fs::read_to_string(&passwd_path).expect("read"), + TWO, + "the first file was written even though the second was invalid" + ); + assert_eq!(std::fs::read_to_string(&group_path).expect("read"), GROUP); + } + + /// And the locks are released, so the next transaction can start. + #[test] + fn test_commit_all_releases_every_lock() { + let dir = tempfile::tempdir().expect("tempdir"); + let passwd_path = dir.path().join("passwd"); + std::fs::write(&passwd_path, TWO).expect("write"); + let group_path = temp_group(dir.path(), GROUP); + + let passwd = LockedFile::::open(&passwd_path).expect("open passwd"); + let group = LockedFile::::open(&group_path).expect("open group"); + commit_all(vec![Box::new(passwd), Box::new(group)]).expect("commit_all"); + + LockedFile::::open(&passwd_path).expect("passwd lock is free"); + LockedFile::::open(&group_path).expect("group lock is free"); + } + + /// A commit that would write the same bytes writes nothing: the inode is + /// not replaced and the mtime does not move. Tools that only *might* + /// change a file used to carry their own "did anything change" flag, and + /// an unchanged empty file would fail outright, since the atomic writer + /// refuses zero length. + #[test] + fn test_an_unchanged_commit_does_not_rewrite_the_file() { + use std::os::unix::fs::MetadataExt; + + let (_d, path) = temp_passwd(TWO); + let before = std::fs::metadata(&path).expect("stat").ino(); + + LockedFile::::open(&path) + .expect("open") + .commit() + .expect("commit"); + + assert_eq!( + std::fs::metadata(&path).expect("stat").ino(), + before, + "the file was rewritten even though nothing changed" + ); + assert_eq!(std::fs::read_to_string(&path).expect("read"), TWO); + } + + /// The same holds for an empty file, which is where it matters: writing it + /// would fail, and the callers that touch `/etc/gshadow` only sometimes + /// change it. + #[test] + fn test_committing_an_unchanged_empty_file_succeeds() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("gshadow"); + std::fs::write(&path, "").expect("write"); + + LockedFile::::open(&path) + .expect("open") + .commit() + .expect("an unchanged empty file must not be an error"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), ""); + } } diff --git a/src/uu/groupmod/src/groupmod.rs b/src/uu/groupmod/src/groupmod.rs index 97b9d5c..f64a904 100644 --- a/src/uu/groupmod/src/groupmod.rs +++ b/src/uu/groupmod/src/groupmod.rs @@ -14,14 +14,13 @@ use std::path::Path; use clap::{Arg, ArgAction, Command}; use uucore::error::{UError, UResult}; -use shadow_core::atomic; use shadow_core::audit; -use shadow_core::group::{self}; -use shadow_core::gshadow::{self}; -use shadow_core::lock::FileLock; +use shadow_core::group::GroupEntry; +use shadow_core::gshadow::GshadowEntry; use shadow_core::nscd; -use shadow_core::passwd; +use shadow_core::passwd::PasswdEntry; use shadow_core::sysroot::SysRoot; +use shadow_core::transaction::{self, Commit, LockedFile}; mod options { pub const GROUP: &str = "GROUP"; @@ -149,36 +148,42 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { }) .transpose()?; - // Block signals for the duration of the critical section so a SIGINT - // between lock acquisition and atomic_write cannot leave stale lock files. - let _signals = shadow_core::hardening::SignalBlocker::block_critical() - .map_err(|e| GroupmodError::CantUpdate(format!("cannot block signals: {e}")))?; - - // Lock order across the tools is passwd < group < gshadow < shadow, so - // take the passwd lock first when -g will need it (see below), never after - // group. This keeps the ordering acyclic with useradd/usermod. + // Lock order across the tools is passwd < group < gshadow < shadow, so the + // files are opened in that order and never in another. This keeps the + // ordering acyclic with useradd and usermod. Each transaction blocks + // signals for its lifetime and releases its lock on every path out. let group_path = root.group_path(); let passwd_path = root.passwd_path(); - // Only the -g path touches passwd, and only if it exists (a --prefix tree - // may not carry one). Guarded like the gshadow write below. - let update_passwd = parsed_gid.is_some() && passwd_path.exists(); - let passwd_lock = if update_passwd { - Some(FileLock::acquire(&passwd_path).map_err(|e| { - GroupmodError::CantUpdate(format!("cannot lock {}: {e}", passwd_path.display())) - })?) + let gshadow_path = root.gshadow_path(); + + let cant_update = |path: &std::path::Path| { + let display = path.display().to_string(); + move |e: shadow_core::error::ShadowError| { + GroupmodError::CantUpdate(format!("cannot open {display}: {e}")) + } + }; + + // Only the -g path touches passwd, and only if it exists: a --prefix tree + // may not carry one. + let mut passwd = if parsed_gid.is_some() && passwd_path.exists() { + Some(LockedFile::::open(&passwd_path).map_err(cant_update(&passwd_path))?) } else { None }; - let group_lock = FileLock::acquire(&group_path).map_err(|e| { - GroupmodError::CantUpdate(format!("cannot lock {}: {e}", group_path.display())) - })?; + let mut groups = + LockedFile::::open(&group_path).map_err(cant_update(&group_path))?; - let (mut entries, group_layout) = group::read_group_with_layout(&group_path).map_err(|e| { - GroupmodError::CantUpdate(format!("cannot read {}: {e}", group_path.display())) - })?; + // gshadow is only touched by a rename or a password change. + let touches_gshadow = new_name.is_some() || new_password.is_some(); + let mut gshadow = if gshadow_path.exists() && touches_gshadow { + Some(LockedFile::::open(&gshadow_path).map_err(cant_update(&gshadow_path))?) + } else { + None + }; // Find the target group. + let entries = groups.entries_mut(); let idx = entries .iter() .position(|g| g.name == *group_name) @@ -195,7 +200,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .iter() .any(|g| g.gid == gid && g.name != *group_name) { - drop(group_lock); return Err(GroupmodError::GidInUse(format!("GID '{gid}' already exists")).into()); } entries[idx].gid = gid; @@ -207,7 +211,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .iter() .any(|g| g.name == *name && g.name != *group_name) { - drop(group_lock); return Err(GroupmodError::NameInUse(format!("group '{name}' already exists")).into()); } entries[idx].name.clone_from(name); @@ -235,98 +238,50 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } } - let modified_gid = entries[idx].gid; + // Without a gshadow file the password belongs in the group file, which is + // where a system with no gshadow keeps it; otherwise -p was a silent no-op. + if gshadow.is_none() + && let Some(pw) = new_password + { + entries[idx].passwd.clone_from(pw); + } - // Write /etc/group. - atomic::atomic_write(&group_path, |f| { - group::write_group_with_layout(&entries, &group_layout, f) - }) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot write {}: {e}", group_path.display())) - })?; + let modified_gid = entries[idx].gid; // groupmod(8): "Users who use the group as their primary group are updated - // to keep the group as their primary group." Do it while we still hold the - // passwd lock we took above, then release group then passwd. + // to keep the group as their primary group." if let Some(new_gid_val) = parsed_gid && new_gid_val != old_gid - && update_passwd + && let Some(passwd) = passwd.as_mut() { - let (mut pw_entries, passwd_layout) = passwd::read_passwd_with_layout(&passwd_path) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot read {}: {e}", passwd_path.display())) - })?; - let mut changed = false; - for e in &mut pw_entries { + for e in passwd.entries_mut() { if e.gid == old_gid { e.gid = new_gid_val; - changed = true; } } - if changed { - atomic::atomic_write(&passwd_path, |f| { - passwd::write_passwd_with_layout(&pw_entries, &passwd_layout, f) - }) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot write {}: {e}", passwd_path.display())) - })?; - } } - drop(group_lock); - drop(passwd_lock); - - // Update /etc/gshadow. - let gshadow_path = root.gshadow_path(); - // Without a gshadow file the password belongs in the group file, which is - // where a system with no gshadow keeps it; otherwise -p was a silent no-op. - if !gshadow_path.exists() - && let Some(pw) = new_password - { - let (mut regroup, layout) = group::read_group_with_layout(&group_path).map_err(|e| { - GroupmodError::CantUpdate(format!("cannot read {}: {e}", group_path.display())) - })?; - if let Some(g) = regroup - .iter_mut() - .find(|g| g.name == *new_name.unwrap_or(group_name)) - { - g.passwd.clone_from(pw); + if let Some(gs) = gshadow.as_mut().and_then(|f| f.find_mut(group_name)) { + if let Some(name) = new_name { + gs.name.clone_from(name); } - atomic::atomic_write(&group_path, |f| { - group::write_group_with_layout(®roup, &layout, f) - }) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot write {}: {e}", group_path.display())) - })?; - } - if gshadow_path.exists() && (new_name.is_some() || new_password.is_some()) { - let gs_lock = FileLock::acquire(&gshadow_path).map_err(|e| { - GroupmodError::CantUpdate(format!("cannot lock {}: {e}", gshadow_path.display())) - })?; - - let (mut gs_entries, gshadow_layout) = gshadow::read_gshadow_with_layout(&gshadow_path) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot read {}: {e}", gshadow_path.display())) - })?; - - if let Some(gs) = gs_entries.iter_mut().find(|g| g.name == *group_name) { - if let Some(name) = new_name { - gs.name.clone_from(name); - } - if let Some(pw) = new_password { - gs.passwd.clone_from(pw); - } + if let Some(pw) = new_password { + gs.passwd.clone_from(pw); } + } - atomic::atomic_write(&gshadow_path, |f| { - gshadow::write_gshadow_with_layout(&gs_entries, &gshadow_layout, f) - }) - .map_err(|e| { - GroupmodError::CantUpdate(format!("cannot write {}: {e}", gshadow_path.display())) - })?; - - drop(gs_lock); + // Every file is validated before any is written, so a value that would + // corrupt one of them cannot leave the set half applied. + let mut files: Vec> = Vec::new(); + if let Some(passwd) = passwd { + files.push(Box::new(passwd)); + } + files.push(Box::new(groups)); + if let Some(gshadow) = gshadow { + files.push(Box::new(gshadow)); } + transaction::commit_all(files) + .map_err(|e| GroupmodError::CantUpdate(format!("cannot write: {e}")))?; nscd::invalidate_cache("group"); diff --git a/src/uu/grpck/src/grpck.rs b/src/uu/grpck/src/grpck.rs index 0576fe3..63d2018 100644 --- a/src/uu/grpck/src/grpck.rs +++ b/src/uu/grpck/src/grpck.rs @@ -22,12 +22,11 @@ use std::path::{Path, PathBuf}; use clap::{Arg, ArgAction, Command}; use uucore::error::{UError, UResult}; -use shadow_core::atomic; -use shadow_core::group::{self, GroupEntry}; +use shadow_core::group::GroupEntry; use shadow_core::gshadow::{self, GshadowEntry}; -use shadow_core::lock::FileLock; use shadow_core::nscd; use shadow_core::sysroot::SysRoot; +use shadow_core::transaction::{self, Commit, LockedFile}; mod options { pub const READ_ONLY: &str = "read-only"; @@ -297,54 +296,32 @@ fn check_group_gshadow_consistency( /// sort would require a significantly different parser that tracks raw /// lines alongside parsed entries. This matches GNU `grpck -s` behavior. fn sort_and_write(group_path: &Path, gshadow_path: &Path) -> UResult<()> { - let group_lock = FileLock::acquire(group_path) - .map_err(|e| GrpckError::CantLock(format!("cannot lock {}: {e}", group_path.display())))?; - - // Re-read under the lock: the entries checked above were read before it. - // The layout keeps comments, blank lines and NIS compat lines, each - // anchored to the entry it preceded, so a comment follows its group. - let (mut sorted_groups, group_layout) = - group::read_group_with_layout(group_path).map_err(|e| { - GrpckError::CantUpdate(format!("cannot read {}: {e}", group_path.display())) - })?; - let original = sorted_groups.clone(); - sorted_groups.sort_by_key(|g| g.gid); - - if sorted_groups == original { - drop(group_lock); - return Ok(()); - } - - atomic::atomic_write(group_path, |f| { - group::write_group_with_layout(&sorted_groups, &group_layout, f) - }) - .map_err(|e| GrpckError::CantUpdate(format!("cannot update {}: {e}", group_path.display())))?; - - // Sort gshadow to match the new group order. + // The transaction re-reads under the lock: the entries checked above were + // read before it. The layout keeps comments, blank lines and NIS compat + // lines, each anchored to the entry it preceded, so a comment follows its + // group. A commit that would write the same bytes writes nothing. + let mut group_file = LockedFile::::open(group_path) + .map_err(|e| GrpckError::CantLock(format!("cannot open {}: {e}", group_path.display())))?; + group_file.entries_mut().sort_by_key(|g| g.gid); + let sorted_groups = group_file.entries().to_vec(); + + // group and gshadow are written together: a sort that reordered one and + // not the other is exactly the "members differ" state grpck reports. + let mut files: Vec> = vec![Box::new(group_file)]; if gshadow_path.exists() { - let gs_lock = FileLock::acquire(gshadow_path).map_err(|e| { - GrpckError::CantLock(format!("cannot lock {}: {e}", gshadow_path.display())) + let mut gshadow_file = LockedFile::::open(gshadow_path).map_err(|e| { + GrpckError::CantLock(format!("cannot open {}: {e}", gshadow_path.display())) })?; - - let (gshadow_entries, gshadow_layout) = gshadow::read_gshadow_with_layout(gshadow_path) - .map_err(|e| { - GrpckError::CantUpdate(format!("cannot read {}: {e}", gshadow_path.display())) - })?; - - if !gshadow_entries.is_empty() { - let sorted_gshadow = sort_gshadow_by_group(&sorted_groups, &gshadow_entries); - atomic::atomic_write(gshadow_path, |f| { - gshadow::write_gshadow_with_layout(&sorted_gshadow, &gshadow_layout, f) - }) - .map_err(|e| { - GrpckError::CantUpdate(format!("cannot update {}: {e}", gshadow_path.display())) - })?; + if !gshadow_file.entries().is_empty() { + let sorted = sort_gshadow_by_group(&sorted_groups, gshadow_file.entries()); + *gshadow_file.entries_mut() = sorted; } - - drop(gs_lock); + files.push(Box::new(gshadow_file)); } - drop(group_lock); + transaction::commit_all(files) + .map_err(|e| GrpckError::CantUpdate(format!("cannot update: {e}")))?; + nscd::invalidate_cache("group"); Ok(()) diff --git a/src/uu/pwck/src/pwck.rs b/src/uu/pwck/src/pwck.rs index 90b885f..1577281 100644 --- a/src/uu/pwck/src/pwck.rs +++ b/src/uu/pwck/src/pwck.rs @@ -18,11 +18,11 @@ use std::path::{Path, PathBuf}; use clap::{Arg, ArgAction, Command}; use shadow_core::group::{self, GroupEntry}; -use shadow_core::lock::FileLock; -use shadow_core::passwd::{self, PasswdEntry}; +use shadow_core::nscd; +use shadow_core::passwd::PasswdEntry; use shadow_core::shadow::{self, ShadowEntry}; use shadow_core::sysroot::SysRoot; -use shadow_core::{atomic, nscd}; +use shadow_core::transaction::{self, Commit, LockedFile}; use uucore::error::{UError, UResult}; @@ -260,55 +260,44 @@ fn sort_and_write(passwd_path: &Path, shadow_path: &Path, read_only: bool) -> UR return Ok(()); } - let passwd_lock = FileLock::acquire(passwd_path) - .map_err(|e| PwckError::CantLock(format!("cannot lock {}: {e}", passwd_path.display())))?; + // The transaction re-reads under the lock: the entries checked above were + // read before it, so sorting those would overwrite anything that changed + // in between. The layout keeps comments, blank lines and NIS compat lines, + // each anchored to the entry it preceded, so a comment follows its + // account. A commit that would write the same bytes writes nothing. + let mut passwd_file = LockedFile::::open(passwd_path) + .map_err(|e| PwckError::CantLock(format!("cannot open {}: {e}", passwd_path.display())))?; + passwd_file.entries_mut().sort_by_key(|e| e.uid); + let sorted_passwd = passwd_file.entries().to_vec(); - // Re-read under the lock: the entries checked above were read before it, - // so sorting those would overwrite anything that changed in between. The - // layout keeps comments, blank lines and NIS compat lines, each anchored - // to the entry it preceded, so a comment follows its account. - let (mut sorted_passwd, passwd_layout) = - passwd::read_passwd_with_layout(passwd_path).map_err(|e| { - PwckError::CantUpdate(format!("cannot read {}: {e}", passwd_path.display())) - })?; - let original = sorted_passwd.clone(); - sorted_passwd.sort_by_key(|e| e.uid); - - if sorted_passwd == original { - drop(passwd_lock); - return Ok(()); - } - - atomic::atomic_write(passwd_path, |f| { - passwd::write_passwd_with_layout(&sorted_passwd, &passwd_layout, f) - }) - // pwck(8) exit 6 is "can not sort"; this is the write that performs it. - .map_err(|e| PwckError::CantSort(format!("cannot sort {}: {e}", passwd_path.display())))?; + let mut files: Vec> = vec![Box::new(passwd_file)]; if shadow_path.exists() { - let shadow_lock = FileLock::acquire(shadow_path).map_err(|e| { - PwckError::CantLock(format!("cannot lock {}: {e}", shadow_path.display())) + let mut shadow_file = LockedFile::::open(shadow_path).map_err(|e| { + PwckError::CantLock(format!("cannot open {}: {e}", shadow_path.display())) })?; - - let (shadow_entries, shadow_layout) = shadow::read_shadow_with_layout(shadow_path) - .map_err(|e| { - PwckError::CantUpdate(format!("cannot read {}: {e}", shadow_path.display())) - })?; - - if !shadow_entries.is_empty() { - let sorted_shadow = sort_shadow_by_passwd(&sorted_passwd, &shadow_entries); - atomic::atomic_write(shadow_path, |f| { - shadow::write_shadow_with_layout(&sorted_shadow, &shadow_layout, f) - }) - .map_err(|e| { - PwckError::CantUpdate(format!("cannot update {}: {e}", shadow_path.display())) - })?; + if !shadow_file.entries().is_empty() { + let sorted = sort_shadow_by_passwd(&sorted_passwd, shadow_file.entries()); + *shadow_file.entries_mut() = sorted; } - - drop(shadow_lock); + files.push(Box::new(shadow_file)); } - drop(passwd_lock); + // pwck(8) keeps two codes here: 6 is "can not sort" and 5 is "can not + // update the files". The error names the file it failed on, so the two + // stay distinct even though both files are written by one call. + transaction::commit_all(files).map_err(|e| { + let on_shadow = matches!( + &e, + shadow_core::error::ShadowError::IoPath(_, p) if p == shadow_path + ); + if on_shadow { + PwckError::CantUpdate(format!("cannot update {}: {e}", shadow_path.display())) + } else { + PwckError::CantSort(format!("cannot sort {}: {e}", passwd_path.display())) + } + })?; + nscd::invalidate_cache("passwd"); Ok(()) diff --git a/src/uu/useradd/src/useradd.rs b/src/uu/useradd/src/useradd.rs index ffae786..5a653ca 100644 --- a/src/uu/useradd/src/useradd.rs +++ b/src/uu/useradd/src/useradd.rs @@ -19,17 +19,16 @@ use std::path::Path; use clap::{Arg, ArgAction, Command}; -use shadow_core::atomic; use shadow_core::audit; -use shadow_core::group::{self, GroupEntry}; -use shadow_core::gshadow::{self, GshadowEntry}; -use shadow_core::lock::FileLock; +use shadow_core::group::GroupEntry; +use shadow_core::gshadow::GshadowEntry; use shadow_core::login_defs::{self, LoginDefs}; use shadow_core::nscd; -use shadow_core::passwd::{self, PasswdEntry}; -use shadow_core::shadow::{self, ShadowEntry}; +use shadow_core::passwd::PasswdEntry; +use shadow_core::shadow::ShadowEntry; use shadow_core::skel; use shadow_core::sysroot::SysRoot; +use shadow_core::transaction::{self, Commit, LockedFile}; use shadow_core::uid_alloc; use shadow_core::validate; @@ -556,23 +555,32 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { let signals = shadow_core::hardening::SignalBlocker::block_critical() .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot block signals: {e}")))?; - // Acquire locks BEFORE reading so concurrent useradd cannot - // silently overwrite entries added between our read and write. + // Each transaction locks before reading, so a concurrent useradd cannot + // silently overwrite an entry added between our read and our write. They + // are opened in the project's lock order: passwd, then group, then + // gshadow. Every early return below drops them, releasing the locks with + // the files untouched. let passwd_path = opts.root.passwd_path(); - let passwd_lock = FileLock::acquire(&passwd_path) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot lock passwd: {e}")))?; - let group_path = opts.root.group_path(); - let group_lock = FileLock::acquire(&group_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("cannot lock group: {e}")))?; + let gshadow_path = opts.root.gshadow_path(); - // Step 3: Read passwd under lock and check username not already in use. - let (passwd_entries, passwd_layout) = passwd::read_passwd_with_layout(&passwd_path) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; + let mut passwd_file = LockedFile::::open(&passwd_path) + .map_err(|e| UseraddError::CannotUpdatePasswd(format!("cannot open passwd: {e}")))?; + let mut group_file = LockedFile::::open(&group_path) + .map_err(|e| UseraddError::CannotUpdateGroup(format!("cannot open group: {e}")))?; + // A fresh --prefix tree may carry no gshadow; useradd does not create one. + let mut gshadow_file = if gshadow_path.exists() { + Some( + LockedFile::::open(&gshadow_path).map_err(|e| { + UseraddError::CannotUpdateGroup(format!("cannot open gshadow: {e}")) + })?, + ) + } else { + None + }; - if passwd_entries.iter().any(|e| e.name == opts.login) { - drop(group_lock); - drop(passwd_lock); + // Step 3: Check the username is not already in use. + if passwd_file.find(&opts.login).is_some() { return Err( UseraddError::UsernameInUse(format!("user '{}' already exists", opts.login)).into(), ); @@ -587,34 +595,19 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { // Step 5: Determine UID. A prefixed run manages another system's files, so // the local name service is not consulted for it. let scope = uid_alloc::Scope::for_prefix(opts.root.is_prefixed()); - let uid = determine_uid(opts, &passwd_entries, &defs, scope)?; - - // Step 6: Read group entries under lock (needed for GID resolution and - // user group creation). - let (mut group_entries, group_layout) = group::read_group_with_layout(&group_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; + let uid = determine_uid(opts, passwd_file.entries(), &defs, scope)?; // Step 7: Determine primary GID. - let (gid, new_group) = determine_gid(opts, uid, &group_entries, &defs, scope)?; - - // Step 8: Read gshadow entries. - let gshadow_path = opts.root.gshadow_path(); - let (mut gshadow_entries, gshadow_layout) = if gshadow_path.exists() { - gshadow::read_gshadow_with_layout(&gshadow_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))? - } else { - (Vec::new(), gshadow::Layout::default()) - }; + let (gid, new_group) = determine_gid(opts, uid, group_file.entries(), &defs, scope)?; // Step 9: Validate supplementary groups exist. for grp_name in &opts.groups { // -G takes the same forms as -g: a group name or a GID. - let known = group_entries + let known = group_file + .entries() .iter() .any(|g| g.name == *grp_name || grp_name.parse::().is_ok_and(|id| g.gid == id)); if !known { - drop(group_lock); - drop(passwd_lock); return Err( UseraddError::GroupNotExist(format!("group '{grp_name}' does not exist")).into(), ); @@ -635,29 +628,26 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { }); // ------------------------------------------------------------------- - // Begin mutations. From here, partial state is left on failure - // (matching GNU behavior). Locks are held throughout. + // Begin mutations. Nothing is written until every file has been changed + // in memory and validated, so a value that would corrupt one of them + // cannot leave the account half created. // ------------------------------------------------------------------- - // Step 11: Create user group if needed (group lock already held). + // Step 11: Create the user group if needed. if let Some(ref new_grp) = new_group { - write_new_group(&group_path, &mut group_entries, &group_layout, new_grp)?; - if gshadow_path.exists() { - // Acquire gshadow lock — group.lock does NOT protect gshadow. - let _gs_lock = FileLock::acquire(&gshadow_path).map_err(|e| { - UseraddError::CannotUpdateGroup(format!("cannot lock gshadow: {e}")) - })?; - write_new_gshadow( - &gshadow_path, - &mut gshadow_entries, - &gshadow_layout, - new_grp, - )?; + group_file.entries_mut().push(new_grp.clone()); + if let Some(gshadow_file) = gshadow_file.as_mut() { + gshadow_file.entries_mut().push(GshadowEntry { + name: new_grp.name.clone(), + passwd: "!".to_string(), + admins: Vec::new(), + members: Vec::new(), + }); } } - // Step 12: Write /etc/passwd entry (lock already held). - let passwd_entry = PasswdEntry { + // Step 12: Add the /etc/passwd entry. + passwd_file.entries_mut().push(PasswdEntry { name: opts.login.clone(), passwd: "x".to_string(), uid, @@ -665,8 +655,13 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { gecos: opts.comment.clone(), home: home_dir.clone(), shell: opts.shell.clone(), - }; - write_passwd_entry(&passwd_path, &passwd_entries, &passwd_layout, &passwd_entry)?; + }); + + let mut files: Vec> = vec![Box::new(passwd_file), Box::new(group_file)]; + if let Some(gshadow_file) = gshadow_file { + files.push(Box::new(gshadow_file)); + } + transaction::commit_all(files).map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; // Step 13: Write /etc/shadow entry (passwd+group locks still held). let shadow_path = opts.root.shadow_path(); @@ -700,11 +695,10 @@ fn do_useradd(opts: &UseraddOptions) -> UResult<()> { }; write_shadow_entry(&shadow_path, &shadow_entry)?; - // Release locks and signal blocker now that passwd, group, and shadow writes are complete. - // Subsequent steps (subid, supplementary groups, home creation) are individually - // crash-safe and may be long-running, so signals should be interruptible. - drop(group_lock); - drop(passwd_lock); + // The transactions above released their locks when they committed. + // Subsequent steps (subid, supplementary groups, home creation) are + // individually crash-safe and may be long-running, so signals become + // interruptible again here. drop(signals); // Step 14: Allocate subordinate UID/GID ranges for rootless containers. @@ -885,90 +879,17 @@ fn resolve_group(gid_arg: &str, group_entries: &[GroupEntry]) -> Result, - layout: &group::Layout, - new_group: &GroupEntry, -) -> UResult<()> { - group_entries.push(new_group.clone()); - - atomic::atomic_write(group_path, |f| { - group::write_group_with_layout(group_entries, layout, f) - }) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - Ok(()) -} - -/// Append a new gshadow entry to `/etc/gshadow`. -/// -/// Caller must hold the gshadow file lock (or the group file lock -/// if gshadow is protected by the same lock scheme). -fn write_new_gshadow( - gshadow_path: &Path, - gshadow_entries: &mut Vec, - layout: &gshadow::Layout, - new_group: &GroupEntry, -) -> UResult<()> { - gshadow_entries.push(GshadowEntry { - name: new_group.name.clone(), - passwd: "!".to_string(), - admins: Vec::new(), - members: Vec::new(), - }); - - atomic::atomic_write(gshadow_path, |f| { - gshadow::write_gshadow_with_layout(gshadow_entries, layout, f) - }) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - Ok(()) -} - -/// Append a new passwd entry to `/etc/passwd`. -/// -/// Caller must hold the passwd file lock. -fn write_passwd_entry( - passwd_path: &Path, - existing: &[PasswdEntry], - layout: &passwd::Layout, - new_entry: &PasswdEntry, -) -> UResult<()> { - let mut entries: Vec = existing.to_vec(); - entries.push(new_entry.clone()); - - atomic::atomic_write(passwd_path, |f| { - passwd::write_passwd_with_layout(&entries, layout, f) - }) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - - Ok(()) -} - -/// Append a new shadow entry to `/etc/shadow` with proper locking. +/// A shadow file that does not exist yet is created: a fresh `--prefix` tree +/// carries none. fn write_shadow_entry(shadow_path: &Path, new_entry: &ShadowEntry) -> UResult<()> { - let _lock = FileLock::acquire(shadow_path) + let mut shadow = LockedFile::::open_or_empty(shadow_path) + .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; + shadow.entries_mut().push(new_entry.clone()); + shadow + .commit() .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - - // Read existing entries; if the file does not exist, start fresh. - let (mut entries, layout) = if shadow_path.exists() { - shadow::read_shadow_with_layout(shadow_path) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))? - } else { - (Vec::new(), shadow::Layout::default()) - }; - - entries.push(new_entry.clone()); - - atomic::atomic_write(shadow_path, |f| { - shadow::write_shadow_with_layout(&entries, &layout, f) - }) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - Ok(()) } @@ -990,13 +911,10 @@ fn add_to_supplementary_groups( group_path: &Path, gshadow_path: &Path, ) -> UResult<()> { - let _lock = FileLock::acquire(group_path) + let mut groups = LockedFile::::open(group_path) .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - let (mut entries, layout) = group::read_group_with_layout(group_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - for entry in &mut entries { + for entry in groups.entries_mut() { if group_requested(&opts.groups, &entry.name, entry.gid) && !entry.members.contains(&opts.login) { @@ -1004,32 +922,23 @@ fn add_to_supplementary_groups( } } - atomic::atomic_write(group_path, |f| { - group::write_group_with_layout(&entries, &layout, f) - }) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - // Also update gshadow if it exists. + // group and gshadow carry the same membership lists, so they are written + // together: a member in one and not the other is what grpck reports as + // "members differ". + let mut files: Vec> = vec![Box::new(groups)]; if gshadow_path.exists() { - let _gs_lock = FileLock::acquire(gshadow_path) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - let (mut gs_entries, gs_layout) = gshadow::read_gshadow_with_layout(gshadow_path) + let mut gshadow = LockedFile::::open(gshadow_path) .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; - - for entry in &mut gs_entries { + for entry in gshadow.entries_mut() { if gs_group_requested(&opts.groups, &entry.name) && !entry.members.contains(&opts.login) { entry.members.push(opts.login.clone()); } } - - atomic::atomic_write(gshadow_path, |f| { - gshadow::write_gshadow_with_layout(&gs_entries, &gs_layout, f) - }) - .map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; + files.push(Box::new(gshadow)); } + transaction::commit_all(files).map_err(|e| UseraddError::CannotUpdateGroup(format!("{e}")))?; Ok(()) } @@ -1042,51 +951,36 @@ fn add_to_supplementary_groups( /// Skips the write if the user already has an entry in the file. /// Uses file locking and atomic writes for crash safety. fn append_subid_entry(path: &Path, name: &str, count: u64) -> UResult<()> { - use shadow_core::subid::{self, SubIdEntry}; + use shadow_core::subid::SubIdEntry; - let lock = FileLock::acquire(path).map_err(|e| { - UseraddError::CannotUpdatePasswd(format!("cannot lock {}: {e}", path.display())) + let mut file = LockedFile::::open(path).map_err(|e| { + uucore::show_error!("warning: cannot open {}: {e}", path.display()); + UseraddError::CannotUpdatePasswd(format!("cannot open {}: {e}", path.display())) })?; - let (mut entries, layout) = match subid::read_subid_with_layout(path) { - Ok(e) => e, - Err(e) => { - uucore::show_error!("warning: cannot read {}: {e}", path.display()); - return Err(UseraddError::CannotUpdatePasswd(format!( - "cannot read {}: {e}", - path.display() - )) - .into()); - } - }; - // Don't add a duplicate entry. - if entries.iter().any(|e| e.name == name) { - drop(lock); + if file.entries().iter().any(|e| e.name == name) { return Ok(()); } // Find next available range by starting after the highest existing end. // Clamp to at least 100_000 even if existing entries are below that threshold. - let start = entries + let start = file + .entries() .iter() .map(|e| e.start.saturating_add(e.count)) .max() .unwrap_or(100_000) .max(100_000); - entries.push(SubIdEntry { + file.entries_mut().push(SubIdEntry { name: name.to_string(), start, count, }); - atomic::atomic_write(path, |f| { - subid::write_subid_with_layout(&entries, &layout, f) - }) - .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; - - drop(lock); + file.commit() + .map_err(|e| UseraddError::CannotUpdatePasswd(format!("{e}")))?; Ok(()) } @@ -1884,6 +1778,8 @@ mod tests { (dir, root) } + use shadow_core::{group, gshadow, passwd, shadow}; + /// Skip tests that require root privileges. fn skip_unless_root() -> bool { !rustix::process::geteuid().is_root() @@ -1899,14 +1795,12 @@ mod tests { let defs = LoginDefs::load(&root.login_defs_path()).expect("defs"); - let (passwd_entries, passwd_layout) = - passwd::read_passwd_with_layout(&root.passwd_path()).expect("passwd"); - let _group_entries = group::read_group_file(&root.group_path()).expect("group"); + let mut passwd_file = LockedFile::::open(&root.passwd_path()).expect("passwd"); // Allocate UID. let (uid_min, uid_max) = uid_alloc::uid_range(&defs, false); let uid = uid_alloc::next_uid( - &passwd_entries, + passwd_file.entries(), uid_min, uid_max, uid_alloc::Scope::FilesOnly, @@ -1915,7 +1809,7 @@ mod tests { assert_eq!(uid, 1000); // Create passwd entry. - let new_entry = PasswdEntry { + passwd_file.entries_mut().push(PasswdEntry { name: "testuser".into(), passwd: "x".into(), uid, @@ -1923,15 +1817,8 @@ mod tests { gecos: "Test User".into(), home: "/home/testuser".into(), shell: "/bin/bash".into(), - }; - - write_passwd_entry( - &root.passwd_path(), - &passwd_entries, - &passwd_layout, - &new_entry, - ) - .expect("write passwd"); + }); + passwd_file.commit().expect("write passwd"); // Verify. let updated = passwd::read_passwd_file(&root.passwd_path()).expect("re-read"); @@ -1949,33 +1836,25 @@ mod tests { let (_dir, root) = setup_test_root(); - let (mut group_entries, group_layout) = - group::read_group_with_layout(&root.group_path()).expect("group"); - let (mut gshadow_entries, gshadow_layout) = - gshadow::read_gshadow_with_layout(&root.gshadow_path()).expect("gshadow"); + let mut group_file = LockedFile::::open(&root.group_path()).expect("group"); + let mut gshadow_file = + LockedFile::::open(&root.gshadow_path()).expect("gshadow"); - // Create user group. - let new_group = GroupEntry { + // Create the user group in both files, committed together. + group_file.entries_mut().push(GroupEntry { name: "newuser".into(), passwd: "x".into(), gid: 1000, members: Vec::new(), - }; - - write_new_group( - &root.group_path(), - &mut group_entries, - &group_layout, - &new_group, - ) - .expect("write group"); - write_new_gshadow( - &root.gshadow_path(), - &mut gshadow_entries, - &gshadow_layout, - &new_group, - ) - .expect("write gshadow"); + }); + gshadow_file.entries_mut().push(GshadowEntry { + name: "newuser".into(), + passwd: "!".into(), + admins: Vec::new(), + members: Vec::new(), + }); + transaction::commit_all(vec![Box::new(group_file), Box::new(gshadow_file)]) + .expect("write group and gshadow"); // Verify group. let updated_groups = group::read_group_file(&root.group_path()).expect("re-read"); @@ -2026,21 +1905,14 @@ mod tests { let (_dir, root) = setup_test_root(); // Add a "wheel" group. - let (mut group_entries, group_layout) = - group::read_group_with_layout(&root.group_path()).expect("group"); - let wheel = GroupEntry { + let mut group_file = LockedFile::::open(&root.group_path()).expect("group"); + group_file.entries_mut().push(GroupEntry { name: "wheel".into(), passwd: "x".into(), gid: 10, members: Vec::new(), - }; - write_new_group( - &root.group_path(), - &mut group_entries, - &group_layout, - &wheel, - ) - .expect("add wheel"); + }); + group_file.commit().expect("add wheel"); // Now add "testuser" to "wheel" and "users". let opts = UseraddOptions { diff --git a/src/uu/userdel/src/userdel.rs b/src/uu/userdel/src/userdel.rs index 2a3435b..a4aa330 100644 --- a/src/uu/userdel/src/userdel.rs +++ b/src/uu/userdel/src/userdel.rs @@ -15,13 +15,13 @@ use clap::{Arg, ArgAction, Command}; use uucore::error::{UError, UResult}; use shadow_core::audit; -use shadow_core::group::{self}; -use shadow_core::gshadow::{self}; -use shadow_core::lock::FileLock; +use shadow_core::group::GroupEntry; +use shadow_core::gshadow::GshadowEntry; +use shadow_core::nscd; use shadow_core::passwd::{self, PasswdEntry}; use shadow_core::shadow::ShadowEntry; use shadow_core::sysroot::SysRoot; -use shadow_core::{atomic, nscd}; +use shadow_core::transaction::{self, Commit, LockedFile, Record}; mod options { pub const FORCE: &str = "force"; @@ -351,123 +351,66 @@ fn safe_remove_home( // Helpers // --------------------------------------------------------------------------- -trait HasName { - fn name(&self) -> &str; -} - -impl HasName for PasswdEntry { - fn name(&self) -> &str { - &self.name - } -} - -impl HasName for ShadowEntry { - fn name(&self) -> &str { - &self.name - } -} - -/// Remove an entry by name from a file (passwd or shadow format). +/// Remove an entry by name from a record file. +/// +/// This used to be a hand-rolled line filter with its own idea of which lines +/// were comments, next to the parser that already knows. The transaction locks +/// first, parses properly, and puts the preserved lines back where they were. fn remove_entry_from_file(path: &Path, login: &str, file_label: &str) -> Result<(), String> where - T: std::str::FromStr + std::fmt::Display + HasName, - T::Err: std::fmt::Display, + T: Record, { - let lock = FileLock::acquire(path).map_err(|e| format!("cannot lock {file_label}: {e}"))?; - - let content = std::fs::read_to_string(path) - .map_err(|e| format!("cannot read {}: {e}", path.display()))?; + let mut file = + LockedFile::::open(path).map_err(|e| format!("cannot open {file_label}: {e}"))?; - let mut found = false; - let mut kept_lines = Vec::new(); - - for line in content.lines() { - let trimmed = line.trim_start(); - if trimmed.is_empty() || trimmed.starts_with('#') { - kept_lines.push(line.to_string()); - continue; - } - - if let Ok(entry) = line.parse::() - && entry.name() == login - { - found = true; - continue; // skip this entry - } - kept_lines.push(line.to_string()); - } - - if !found { - drop(lock); + let before = file.entries().len(); + file.entries_mut().retain(|e| e.name() != login); + if file.entries().len() == before { + // Dropping releases the lock with the file untouched. return Err(format!("user '{login}' does not exist in {file_label}")); } - atomic::atomic_write(path, |f| { - for line in &kept_lines { - writeln!(f, "{line}")?; - } - Ok(()) - }) - .map_err(|e| format!("cannot write {}: {e}", path.display()))?; - - drop(lock); - Ok(()) + file.commit() + .map_err(|e| format!("cannot write {}: {e}", path.display())) } /// Remove a username from all group membership lists in /etc/group. fn remove_from_group_members(path: &Path, login: &str) -> Result<(), String> { - let lock = FileLock::acquire(path).map_err(|e| format!("cannot lock group file: {e}"))?; - - let (mut entries, layout) = group::read_group_with_layout(path) - .map_err(|e| format!("cannot read {}: {e}", path.display()))?; + let mut file = + LockedFile::::open(path).map_err(|e| format!("cannot open group file: {e}"))?; let mut changed = false; - for entry in &mut entries { + for entry in file.entries_mut() { let before = entry.members.len(); entry.members.retain(|m| m != login); - if entry.members.len() != before { - changed = true; - } + changed |= entry.members.len() != before; } - if changed { - atomic::atomic_write(path, |f| { - group::write_group_with_layout(&entries, &layout, f) - }) - .map_err(|e| format!("cannot write {}: {e}", path.display()))?; + if !changed { + return Ok(()); } - - drop(lock); - Ok(()) + file.commit() + .map_err(|e| format!("cannot write {}: {e}", path.display())) } /// Remove a username from all gshadow membership and admin lists. fn remove_from_gshadow_members(path: &Path, login: &str) -> Result<(), String> { - let lock = FileLock::acquire(path).map_err(|e| format!("cannot lock gshadow file: {e}"))?; - - let (mut entries, layout) = gshadow::read_gshadow_with_layout(path) - .map_err(|e| format!("cannot read {}: {e}", path.display()))?; + let mut file = LockedFile::::open(path) + .map_err(|e| format!("cannot open gshadow file: {e}"))?; let mut changed = false; - for entry in &mut entries { - let before_m = entry.members.len(); - let before_a = entry.admins.len(); + for entry in file.entries_mut() { + let before = (entry.members.len(), entry.admins.len()); entry.members.retain(|m| m != login); entry.admins.retain(|a| a != login); - if entry.members.len() != before_m || entry.admins.len() != before_a { - changed = true; - } + changed |= (entry.members.len(), entry.admins.len()) != before; } - if changed { - atomic::atomic_write(path, |f| { - gshadow::write_gshadow_with_layout(&entries, &layout, f) - }) - .map_err(|e| format!("cannot write {}: {e}", path.display()))?; + if !changed { + return Ok(()); } - - drop(lock); - Ok(()) + file.commit() + .map_err(|e| format!("cannot write {}: {e}", path.display())) } /// Remove the user's private group — the group named after the login that @@ -489,19 +432,19 @@ fn remove_user_private_group( return Ok(()); } - let group_lock = - FileLock::acquire(&group_path).map_err(|e| format!("cannot lock group file: {e}"))?; - let (mut entries, group_layout) = group::read_group_with_layout(&group_path) - .map_err(|e| format!("cannot read group: {e}"))?; + // Every early return below drops the transaction, releasing the lock with + // the file untouched. + let mut groups = LockedFile::::open(&group_path) + .map_err(|e| format!("cannot open group file: {e}"))?; - let Some(idx) = entries.iter().position(|g| g.name == login) else { + let Some(idx) = groups.entries().iter().position(|g| g.name == login) else { return Ok(()); }; // Keep it if other users still belong to it. - if !entries[idx].members.is_empty() { + if !groups.entries()[idx].members.is_empty() { return Ok(()); } - let gid = entries[idx].gid; + let gid = groups.entries()[idx].gid; // Keep it if it is a private group whose GID is not the user's primary // one (then it is not really this user's), or another user's primary // group, unless forced. @@ -512,28 +455,24 @@ fn remove_user_private_group( return Ok(()); } - entries.remove(idx); - write_group_or_empty(&group_path, &entries, &group_layout) - .map_err(|e| format!("cannot write group: {e}"))?; - drop(group_lock); + groups.entries_mut().remove(idx); - // Mirror the removal in gshadow. + // The group and its gshadow row are removed together: a group in one file + // and not the other is a broken system, and validating both before writing + // either keeps a bad value from leaving them out of step. let gshadow_path = root.gshadow_path(); + let mut files: Vec> = vec![Box::new(groups)]; if gshadow_path.exists() { - let gs_lock = - FileLock::acquire(&gshadow_path).map_err(|e| format!("cannot lock gshadow: {e}"))?; - if let Ok((mut gs, gshadow_layout)) = gshadow::read_gshadow_with_layout(&gshadow_path) { - let before = gs.len(); - gs.retain(|g| g.name != login); - if gs.len() != before { - write_gshadow_or_empty(&gshadow_path, &gs, &gshadow_layout) - .map_err(|e| format!("cannot write gshadow: {e}"))?; - } + let mut gshadow = LockedFile::::open(&gshadow_path) + .map_err(|e| format!("cannot open gshadow: {e}"))?; + let before = gshadow.entries().len(); + gshadow.entries_mut().retain(|g| g.name != login); + if gshadow.entries().len() != before { + files.push(Box::new(gshadow)); } - drop(gs_lock); } - Ok(()) + transaction::commit_all(files).map_err(|e| format!("cannot write: {e}")) } /// Remove every subordinate-ID row owned by `login` from a subuid/subgid file. @@ -547,53 +486,15 @@ fn remove_subid_rows(path: &Path, login: &str) { if !path.exists() { return; } - let Ok(lock) = FileLock::acquire(path) else { + let Ok(mut file) = LockedFile::::open(path) else { return; }; - if let Ok((mut entries, layout)) = subid::read_subid_with_layout(path) { - let before = entries.len(); - entries.retain(|e| e.name != login); - if entries.len() != before { - if entries.is_empty() && layout.is_empty() { - let _ = std::fs::remove_file(path); - } else { - let _ = atomic::atomic_write(path, |f| { - subid::write_subid_with_layout(&entries, &layout, f) - }); - } - } - } - drop(lock); -} - -/// Write group entries, unlinking the file instead if the result is empty -/// (the atomic writer refuses a zero-length file, but an empty group file is -/// valid — and only reached in a fully torn-down `--prefix` tree). -fn write_group_or_empty( - path: &Path, - entries: &[group::GroupEntry], - layout: &group::Layout, -) -> Result<(), shadow_core::error::ShadowError> { - if entries.is_empty() && layout.is_empty() { - let _ = std::fs::remove_file(path); - Ok(()) - } else { - atomic::atomic_write(path, |f| group::write_group_with_layout(entries, layout, f)) - } -} - -fn write_gshadow_or_empty( - path: &Path, - entries: &[gshadow::GshadowEntry], - layout: &gshadow::Layout, -) -> Result<(), shadow_core::error::ShadowError> { - if entries.is_empty() && layout.is_empty() { - let _ = std::fs::remove_file(path); - Ok(()) - } else { - atomic::atomic_write(path, |f| { - gshadow::write_gshadow_with_layout(entries, layout, f) - }) + let before = file.entries().len(); + file.entries_mut().retain(|e| e.name != login); + if file.entries().len() != before { + // An absent subid file means "no ranges", so the last row going leaves + // nothing to write. + let _ = file.commit_or_remove(); } } diff --git a/src/uu/usermod/src/usermod.rs b/src/uu/usermod/src/usermod.rs index 10d54ce..481a352 100644 --- a/src/uu/usermod/src/usermod.rs +++ b/src/uu/usermod/src/usermod.rs @@ -15,13 +15,13 @@ use clap::{Arg, ArgAction, Command}; use uucore::error::{UError, UResult}; use shadow_core::audit; -use shadow_core::group::{self}; -use shadow_core::gshadow::{self}; -use shadow_core::lock::FileLock; -use shadow_core::passwd::{self}; -use shadow_core::shadow::{self}; +use shadow_core::group::{self, GroupEntry}; +use shadow_core::gshadow::GshadowEntry; +use shadow_core::passwd::PasswdEntry; +use shadow_core::shadow::{self, ShadowEntry}; use shadow_core::sysroot::SysRoot; -use shadow_core::{atomic, nscd, validate}; +use shadow_core::transaction::{self, Commit, LockedFile}; +use shadow_core::{nscd, validate}; mod options { pub const COMMENT: &str = "comment"; @@ -145,14 +145,13 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Modify /etc/passwd. let group_path_for_lookup = root.group_path(); let passwd_path = root.passwd_path(); - let lock = FileLock::acquire(&passwd_path) - .map_err(|e| UsermodError::CantUpdate(format!("cannot lock: {e}")))?; - - let (mut entries, passwd_layout) = passwd::read_passwd_with_layout(&passwd_path) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; + // The transaction locks, then reads, and releases on every path out -- + // including each early return below, where the file is left untouched. + let mut passwd_file = LockedFile::::open(&passwd_path) + .map_err(|e| UsermodError::CantUpdate(format!("cannot open passwd: {e}")))?; + let entries = passwd_file.entries_mut(); let Some(idx) = entries.iter().position(|e| e.name == *login) else { - drop(lock); return Err(UsermodError::UserNotFound(format!("user '{login}' does not exist")).into()); }; @@ -164,7 +163,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // Check UID collision before mutating. if let Some(&uid) = matches.get_one::(options::UID) { if entries.iter().any(|e| e.uid == uid && e.name != *login) { - drop(lock); return Err(UsermodError::UidInUse(format!("UID {uid} already in use")).into()); } entries[idx].uid = uid; @@ -194,7 +192,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { groups.iter().find(|g| g.name == *group_arg).map(|g| g.gid) }; let Some(gid) = resolved else { - drop(lock); return Err( UsermodError::GroupNotFound(format!("group '{group_arg}' does not exist")).into(), ); @@ -211,7 +208,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { .iter() .any(|e| e.name == *new_name && e.name != *login) { - drop(lock); return Err( UsermodError::NameInUse(format!("user '{new_name}' already exists")).into(), ); @@ -221,11 +217,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let new_uid = entries[idx].uid; - atomic::atomic_write(&passwd_path, |f| { - passwd::write_passwd_with_layout(&entries, &passwd_layout, f) - }) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; - drop(lock); + passwd_file + .commit() + .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; // Restore signals before potentially long-running recursive chown. drop(signals); @@ -257,14 +251,10 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { || new_password.is_some() || login_changing) { - let slock = FileLock::acquire(&shadow_path) - .map_err(|e| UsermodError::CantUpdate(format!("cannot lock shadow: {e}")))?; - - let (mut se, shadow_layout) = shadow::read_shadow_with_layout(&shadow_path) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; + let mut shadow_file = LockedFile::::open(&shadow_path) + .map_err(|e| UsermodError::CantUpdate(format!("cannot open shadow: {e}")))?; - let Some(s) = se.iter_mut().find(|e| e.name == *login) else { - drop(slock); + let Some(s) = shadow_file.find_mut(login) else { return Err(UsermodError::CantUpdate(format!( "user '{login}' not found in shadow file" )) @@ -277,7 +267,6 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { s.lock(); } if do_unlock && !s.unlock() { - drop(slock); return Err(UsermodError::BadArgument(format!( "unlocking '{login}' would leave the account without a password" )) @@ -299,60 +288,46 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { s.name.clone_from(new_name); } - atomic::atomic_write(&shadow_path, |f| { - shadow::write_shadow_with_layout(&se, &shadow_layout, f) - }) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; - drop(slock); + shadow_file + .commit() + .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; } // Rename user in group membership lists when --login changes the name. if let Some(new_name) = new_login { let group_path = root.group_path(); if group_path.exists() { - let glock = FileLock::acquire(&group_path) - .map_err(|e| UsermodError::CantUpdate(format!("cannot lock group: {e}")))?; + let mut group_file = LockedFile::::open(&group_path) + .map_err(|e| UsermodError::CantUpdate(format!("cannot open group: {e}")))?; - let (mut ge, group_layout) = group::read_group_with_layout(&group_path) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; - - let mut changed = false; - for g in &mut ge { + for g in group_file.entries_mut() { if let Some(m) = g.members.iter_mut().find(|m| **m == *login) { m.clone_from(new_name); - changed = true; } } - // Mirror the rename in gshadow's member and admin lists. + // Mirror the rename in gshadow's member and admin lists. Leaving + // it behind is exactly what grpck reports as "members differ", so + // the two files are committed together. + let mut files: Vec> = vec![Box::new(group_file)]; let gshadow_path = root.gshadow_path(); - if gshadow_path.exists() - && let Ok((mut gs, gs_layout)) = gshadow::read_gshadow_with_layout(&gshadow_path) - { - let mut gs_changed = false; - for g in &mut gs { + if gshadow_path.exists() { + let mut gshadow_file = + LockedFile::::open(&gshadow_path).map_err(|e| { + UsermodError::CantUpdateGroup(format!("cannot open gshadow: {e}")) + })?; + for g in gshadow_file.entries_mut() { for m in g.members.iter_mut().chain(g.admins.iter_mut()) { if *m == *login { m.clone_from(new_name); - gs_changed = true; } } } - if gs_changed { - atomic::atomic_write(&gshadow_path, |f| { - gshadow::write_gshadow_with_layout(&gs, &gs_layout, f) - }) - .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; - } + files.push(Box::new(gshadow_file)); } - if changed { - atomic::atomic_write(&group_path, |f| { - group::write_group_with_layout(&ge, &group_layout, f) - }) - .map_err(|e| UsermodError::CantUpdate(format!("{e}")))?; - } - drop(glock); + transaction::commit_all(files) + .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; } } @@ -375,22 +350,19 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { // added a member that no longer exists. let member = new_login.unwrap_or(login); - let glock = FileLock::acquire(&group_path) - .map_err(|e| UsermodError::CantUpdateGroup(format!("cannot lock group: {e}")))?; - - let (mut ge, group_layout) = group::read_group_with_layout(&group_path) - .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; + let mut group_file = LockedFile::::open(&group_path) + .map_err(|e| UsermodError::CantUpdateGroup(format!("cannot open group: {e}")))?; // Validate every requested group first: -G takes names or GIDs, // and each must exist (usermod(8) exit 6). let mut wanted: Vec = Vec::with_capacity(new_groups.len()); for gname in &new_groups { - let found = ge + let found = group_file + .entries() .iter() .find(|g| g.name == *gname || gname.parse::().is_ok_and(|id| g.gid == id)) .map(|g| g.name.clone()); let Some(name) = found else { - drop(glock); return Err(UsermodError::GroupNotFound(format!( "group '{gname}' does not exist" )) @@ -400,57 +372,44 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { } if !append { - for g in &mut ge { + for g in group_file.entries_mut() { g.members.retain(|m| m != login && m != member); } } for gname in &wanted { - if let Some(g) = ge.iter_mut().find(|g| g.name == *gname) + if let Some(g) = group_file.find_mut(gname) && !g.members.iter().any(|m| m == member) { g.members.push(member.clone()); } } - atomic::atomic_write(&group_path, |f| { - group::write_group_with_layout(&ge, &group_layout, f) - }) - .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; - drop(glock); - // /etc/gshadow carries the same membership lists; leaving it // behind is exactly what grpck reports as "members differ". + let mut files: Vec> = vec![Box::new(group_file)]; let gshadow_path = root.gshadow_path(); if gshadow_path.exists() { - let gshadow_guard = FileLock::acquire(&gshadow_path).map_err(|e| { - UsermodError::CantUpdateGroup(format!("cannot lock gshadow: {e}")) - })?; - let (mut gs, gs_layout) = gshadow::read_gshadow_with_layout(&gshadow_path) - .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; - let mut gs_changed = false; + let mut gshadow_file = + LockedFile::::open(&gshadow_path).map_err(|e| { + UsermodError::CantUpdateGroup(format!("cannot open gshadow: {e}")) + })?; if !append { - for g in &mut gs { - let before = g.members.len(); + for g in gshadow_file.entries_mut() { g.members.retain(|m| m != login && m != member); - gs_changed |= g.members.len() != before; } } for gname in &wanted { - if let Some(g) = gs.iter_mut().find(|g| g.name == *gname) + if let Some(g) = gshadow_file.find_mut(gname) && !g.members.iter().any(|m| m == member) { g.members.push(member.clone()); - gs_changed = true; } } - if gs_changed { - atomic::atomic_write(&gshadow_path, |f| { - gshadow::write_gshadow_with_layout(&gs, &gs_layout, f) - }) - .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; - } - drop(gshadow_guard); + files.push(Box::new(gshadow_file)); } + + transaction::commit_all(files) + .map_err(|e| UsermodError::CantUpdateGroup(format!("{e}")))?; } }