tools: source the "Permission denied" message from the OS (#159)#171
tools: source the "Permission denied" message from the OS (#159)#171pierre-warnier wants to merge 4 commits into
Conversation
The manual root-privilege guards across the account tools printed a hardcoded "Permission denied." string. Route that text through libc's strerror instead (via std::io::Error::from_raw_os_error), so the wording comes from the host OS and is translated by glibc on localized systems — the same way GNU coreutils renders system errors (e.g. cat: Is a directory). This also keeps the string out of our source tree, shrinking the residual clean-room surface that motivated #159. Add shadow_core::os_error with strerror(errno) and permission_denied(), and convert the 13 caller_is_root() guards in chage, chpasswd, groupadd, groupdel, groupmod, passwd, useradd, userdel, usermod to use it. Scope: this covers the errno-mappable class only. True I/O errors already render the OS strerror via ShadowError::Io/IoPath. Domain errors (e.g. "group already exists") have no OS equivalent and stay in-tree. The gettext-catalog idea was evaluated and declined in #159 (the English msgid must remain in source as the lookup key, so it does not remove the exposure). Note: output changes from "Permission denied." to the OS text "Permission denied" (no trailing period; translated off-English). No tests asserted the old literal.
There was a problem hiding this comment.
Pull request overview
This PR aims to make the “permission denied” message for root-required operations come from the host OS (via errno→message) rather than being hardcoded, aligning these account-management utilities more closely with coreutils-style error rendering and enabling localization via the system libc.
Changes:
- Introduces
shadow_core::os_errorwith helpers for errno-backed OS error text (strerror(errno)andpermission_denied()). - Replaces multiple hardcoded
"Permission denied."strings in root-check guards across several tools withshadow_core::os_error::permission_denied(). - Adds a unit test in
shadow_core::os_errorto validate the helper’s basic shape.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shadow-core/src/os_error.rs | Adds OS-sourced error-text helpers and a unit test. |
| src/shadow-core/src/lib.rs | Exposes the new os_error module from shadow-core. |
| src/uu/useradd/src/useradd.rs | Switches root-check error output to os_error::permission_denied(). |
| src/uu/userdel/src/userdel.rs | Switches root-check error string to os_error::permission_denied(). |
| src/uu/usermod/src/usermod.rs | Switches root-check error string to os_error::permission_denied(). |
| src/uu/passwd/src/passwd.rs | Switches several permission-denied errors to os_error::permission_denied(). |
| src/uu/groupadd/src/groupadd.rs | Switches root-check error output to os_error::permission_denied(). |
| src/uu/groupdel/src/groupdel.rs | Switches root-check error output to os_error::permission_denied(). |
| src/uu/groupmod/src/groupmod.rs | Switches root-check error output to os_error::permission_denied(). |
| src/uu/chpasswd/src/chpasswd.rs | Switches permission-denied error to os_error::permission_denied(). |
| src/uu/chage/src/chage.rs | Switches permission-denied errors to os_error::permission_denied(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
io::Error::from_raw_os_error(e).to_string() renders as "Permission denied (os error 13)" — Rust appends its own suffix to the libc text. Strip it so the message is the bare OS string (matching GNU/coreutils output), and add a regression test that the suffix is absent. Addresses review feedback on #171.
| /// For example `EACCES` renders as "Permission denied" on English locales | ||
| /// and the translated equivalent elsewhere. On targets whose libc does not | ||
| /// translate (musl), this is the untranslated English text. | ||
| #[must_use] |
There was a problem hiding this comment.
https://docs.rs/uucore/0.9.0/uucore/error/fn.strip_errno.html
we already have this function
There was a problem hiding this comment.
Good call — switched to uucore::error::strip_errno (it's already in 0.8, so no version bump). Added uucore to shadow-core's deps; it was already in the tree via the tool crates. Thanks!
Replace the local " (os error N)" suffix stripping with uucore's existing strip_errno helper, per review on #171. Adds uucore (already in the dependency tree via the tool crates) to shadow-core.
| // `Display` appends Rust's own " (os error N)" suffix. `strip_errno` (the | ||
| // same helper uucore uses for its own I/O errors) removes it, leaving the | ||
| // bare OS message — matching GNU/coreutils output. | ||
| uucore::error::strip_errno(&std::io::Error::from_raw_os_error(errno)) |
There was a problem hiding this comment.
i think you should just remove strerror and call strip_errno everywhere directly
a function with only one call isn't super interesting IMHO
There was a problem hiding this comment.
Fair point. Removing it.
There was a problem hiding this comment.
Done — removed strerror and inlined it into permission_denied(), which now calls uucore::error::strip_errno directly. Also validated the result on glibc (Debian/Fedora) and musl (Alpine) locally since the Docker matrix is skipped on PRs.
Per review on #171, inline the one caller: permission_denied() now calls uucore::error::strip_errno directly. Also disambiguate the module doc now that the local strerror fn is gone (it refers to libc's strerror).
| /// own I/O errors) drops the " (os error N)" suffix that `io::Error`'s | ||
| /// `Display` appends, leaving the bare OS message — matching coreutils output. | ||
| #[must_use] | ||
| pub fn permission_denied() -> String { |
There was a problem hiding this comment.
i am not sure we need this function either, just call the previous line
There was a problem hiding this comment.
Happy to — one consideration: permission_denied() is called from 13 sites across 9 tools (the root-check guard in chage, chpasswd, groupadd, groupdel, groupmod, passwd, useradd, userdel, usermod). Inlining means repeating uucore::error::strip_errno(&std::io::Error::from_raw_os_error(libc::EACCES)) at each one.
Do you still prefer inlining it everywhere, or would you rather a different shape — e.g. argument-less PermissionDenied error variants that render the OS message in their own Display? Happy to go whichever way you think reads best.
Summary
Toward #159 / "do like coreutils": route the account tools' permission-check message through libc
strerrorinstead of a hardcoded literal, so the wording comes from the host OS and is translated by glibc on localized systems — the same mechanism behindcat: /tmp: Is a directory(and its localized forms).shadow_core::os_error::permission_denied()— returns the OS message forEACCES, built fromstd::io::Error::from_raw_os_errorand run throughuucore::error::strip_errno(the helper uucore uses for its own I/O errors) to drop Rust's(os error N)suffix. Nounsafe.caller_is_root()guards (chage,chpasswd,groupadd,groupdel,groupmod,passwd,useradd,userdel,usermod) from"Permission denied."toos_error::permission_denied().Why this slice of #159
@pierre-warnier's spike on #159 showed the gettext-catalog approach doesn't remove GPL exposure (the English
msgidmust stay in source as the lookup key). The narrower errno→strerroridea @sylvestre and @oech3 pointed at is sound and is what coreutils does; this PR applies it to the one class where we still hardcoded an OS-equivalent string.Already-correct, left unchanged:
strerrorviaShadowError::Io/IoPath.Behavior change
Output changes from
Permission denied.(trailing period) to the OS textPermission denied(no period; localized off-English). No test or e2e assertion depends on the old literal. Flagging in case the period is considered part of drop-in compat — happy to adjust.Test plan
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace— green on Debianos_errorunit test (non-empty + no(os error N)suffix) also run on Alpine (musl) and Fedora locally, since the Docker matrix is push-gated and skipped on PRs(os error N)regressioncc @sylvestre @oech3 — implements the "extract from OS" direction from #159.
Refs #159.