chown: preserve non-UTF-8 account names by limadog9 · Pull Request #15502 · uutils/coreutils

limadog9 · 2026-10-18T18:54:27Z

Preserve account-name bytes through owner and --from parsing and account lookups, including the login-group shorthand. Test successful ownership changes with synthetic non-UTF-8 accounts.

Closes #9958.

sylvestre · 2026-10-18T21:45:46Z

Mostly the test is left: please make it simpler and make it actually check that --from matched.

sylvestre · 2026-10-18T21:45:53Z

+        let mut owner_arg = Arg::new(add_arg_if_not_reference)
+            .value_name(add_arg_if_not_reference)
+            .required(true);
+        if !groups_only {


why not for chgrp too?

sylvestre · 2026-10-18T21:45:59Z

+
+    let ts = TestScenario::new(util_name!());
+    let library = ts.fixtures.plus("accounts.so");
+    let compilation = Command::new("cc")


compiling a C shim with cc in a test is a lot of machinery, do we really need it?
maybe we can just check that the error is now "invalid user" and not the clap UTF-8 error

sylvestre · 2026-10-18T21:46:02Z

+    }
+    ts.ucmd()
+        .env("LD_PRELOAD", &library)
+        .arg("--from")


the ids map to our own, so this passes even if --from never matches
could you please use -v and check the "retained"/"changed" output?

sylvestre · 2026-10-18T21:46:07Z

+        .arg("file")
+        .succeeds();
+    assert_eq!(ts.fixtures.metadata("file").uid(), unsafe {
+        libc::geteuid()


unsafe without SAFETY, and the same metadata is read twice, could be simpler

sylvestre · 2026-10-19T18:25:22Z

 /// this to `':'`. If a separator is given but the group is omitted
 /// (as in `"owner:"`), the group is set to the owner's login group.
-fn parse_spec(spec: &str, sep: char) -> UResult<(Option<u32>, Option<u32>)> {
+fn parse_spec(spec: impl AsRef<OsStr>, sep: char) -> UResult<(Option<u32>, Option<u32>)> {


why impl AsRef<OsStr>? &OsStr should be enough

sylvestre · 2026-10-19T18:25:39Z

+    use std::ffi::OsStr;
+    use std::os::unix::ffi::OsStrExt;
+
+    for from in [false, true] {


two plain new_ucmd!() calls would be easier to read than this loop with if from three times

sylvestre · 2026-10-19T18:25:44Z

thanks for tackling this!
Almost there: the chgrp test could be simpler, and chown --from needs a non-UTF-8 case (details inline).

sylvestre · 2026-10-19T18:26:05Z

+    use std::os::unix::ffi::OsStrExt;
+    for (spec, message) in [
+        (b"unknown_\xff".as_slice(), "invalid user:"),
+        (b":unknown_\xff".as_slice(), "invalid group:"),


could you please add a --from case with a non-UTF-8 value too?

Devel08 · 2026-10-21T16:24:47Z

+        .arg("--from")
+        .arg(OsStr::from_bytes(b"unknown_\xff"))
+        .arg(getegid().to_string())
+        .arg("file")


I think that we can merge those .arg()s with .args(&[...])

Preserve account-name bytes through owner and --from parsing and account lookups, including the login-group shorthand. Test successful ownership changes with synthetic non-UTF-8 accounts. Closes uutils#9958.

codspeed · 2026-10-21T21:26:35Z

Merging this PR will regress 1 benchmark

1 warning, 1 note

⚠️ 120 benchmarks are dominated by system calls

System calls cannot be simulated consistently and are left out of the measure. The reported value understates the real cost. Switch to the Walltime instrument to measure them.

Learn more about system calls

See affected benchmarks

ℹ️ 1 benchmark has no flamegraph

The flamegraph wasn't generated: most of the time was spent in runtime internals, which would leave nothing useful to analyze.

See affected benchmarks

⚡ 3 improved benchmarks
❌ 1 regressed benchmark
✅ 397 untouched benchmarks
⏩ 70 skipped benchmarks¹

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Memory	`cp_recursive_deep_tree[(120, 4)]`	532.6 KB	699.2 KB	-23.82%
⚡	CPU Simulation	`complex_relative_date`	386.2 µs	277.7 µs	+39.05%
⚡	CPU Simulation	`true_consecutive_calls`	350.2 ns	294.7 ns	+18.85%
⚡	Memory	`du_deep_tree[(100, 3)]`	82.4 KB	76.5 KB	+7.68%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.

_{Comparing limadog9:fix/chown-non-utf8-account-names-9958 (bb136fd) with main (4939cd4)²}

70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
No successful run was found on main (2f8b194) during the generation of this report, so 4939cd4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

xtqqczze · 2026-10-21T21:48:58Z

Fuzzing failures tracked by #15567.

limadog9 force-pushed the fix/chown-non-utf8-account-names-9958 branch from 5c8b168 to a6e4746 Compare October 18, 2026 19:59

sylvestre reviewed Oct 18, 2026

View reviewed changes

limadog9 force-pushed the fix/chown-non-utf8-account-names-9958 branch from a6e4746 to cfa4fbf Compare October 18, 2026 22:35

sylvestre reviewed Oct 19, 2026

View reviewed changes

limadog9 force-pushed the fix/chown-non-utf8-account-names-9958 branch from cfa4fbf to 69165ea Compare October 19, 2026 19:28

Devel08 reviewed Oct 21, 2026

View reviewed changes

chown: preserve non-UTF-8 account names

bb136fd

Preserve account-name bytes through owner and --from parsing and account lookups, including the login-group shorthand. Test successful ownership changes with synthetic non-UTF-8 accounts. Closes uutils#9958.

limadog9 force-pushed the fix/chown-non-utf8-account-names-9958 branch from 69165ea to bb136fd Compare October 21, 2026 19:45

Navigation Menu

Repository navigation

chown: preserve non-UTF-8 account names - #15502

chown: preserve non-UTF-8 account names#15502
limadog9 wants to merge 1 commit into
uutils:mainfrom
limadog9:fix/chown-non-utf8-account-names-9958

limadog9 commented Oct 18, 2026

Uh oh!

sylvestre commented Oct 18, 2026

Uh oh!

sylvestre Oct 18, 2026

Uh oh!

sylvestre Oct 18, 2026

Uh oh!

sylvestre Oct 18, 2026

Uh oh!

sylvestre Oct 18, 2026

Uh oh!

sylvestre Oct 19, 2026

Uh oh!

sylvestre Oct 19, 2026

Uh oh!

sylvestre commented Oct 19, 2026

Uh oh!

sylvestre Oct 19, 2026

Uh oh!

Devel08 Oct 21, 2026

Uh oh!

codspeed Bot commented Oct 21, 2026

Uh oh!

xtqqczze commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

limadog9 commented Oct 18, 2026

Uh oh!

sylvestre commented Oct 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sylvestre commented Oct 19, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 21, 2026

Merging this PR will regress 1 benchmark

Performance Changes

Footnotes

Uh oh!

xtqqczze commented Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants