chroot: report ENAMETOOLONG instead of "no such directory" for long NEWROOT - #13881
chroot: report ENAMETOOLONG instead of "no such directory" for long NEWROOT#13881MsfPablo wants to merge 1 commit into
Conversation
…EWROOT GNU chroot reports 'File name too long' when NEWROOT exceeds NAME_MAX; uutils was reporting 'no such directory' because Path::is_dir() silently swallowed the ENAMETOOLONG error from metadata(). This change probes the path with std::fs::metadata() first and surfaces the real io::Error via a new CannotStat variant. Existing 'is not a directory' behavior is preserved. Fixes uutils#13156.
| options.chroot_target = resolved; | ||
| } | ||
|
|
||
| // Probe the path with `metadata` so the *original* I/O error reaches the |
There was a problem hiding this comment.
please make the comment shorter
| // `Path::is_dir()` swallows ENAMETOOLONG. GNU surfaces the real error. | ||
| let long_name = "A".repeat(256); | ||
| let expected = format!( | ||
| "chroot: cannot change root directory to '{long_name}': File name too long (os error 63)\n" |
There was a problem hiding this comment.
please use the strip_errno in the code to remove the (os error 63)
Merging this PR will degrade performance by 3.96%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | df_with_path |
573.7 µs | 704.6 µs | -18.58% |
| ⚡ | Simulation | du_max_depth_balanced_tree[(6, 4, 10)] |
65.2 ms | 62.4 ms | +4.49% |
| ⚡ | Simulation | du_summarize_balanced_tree[(5, 4, 10)] |
16.8 ms | 16.1 ms | +4.14% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing MsfPablo:fix-chroot-enametoolong (e649307) with main (822aa83)
Footnotes
-
314 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. ↩
| // would be misreported as "no such directory". | ||
| if let Err(e) = std::fs::metadata(&options.newroot) { | ||
| return Err(ChrootError::CannotStat(options.newroot, e).into()); | ||
| } |
There was a problem hiding this comment.
We should remove .is_dir() on many platforms instead of adding such hack to extract err from ctual chroot call instead.
| /// The given path could not be stat'd (e.g. ENAMETOOLONG, EACCES). | ||
| #[error("{}", translate!("chroot-error-cannot-stat", "dir" => _0.quote(), "err" => _1))] | ||
| CannotStat(PathBuf, #[source] Error), | ||
|
|
There was a problem hiding this comment.
Why is this needed even it is extracted from OS?
4 participants
Fixes #13156.
Problem
GNU
chrootreportsFile name too longwhen NEWROOT exceeds NAME_MAX (255 bytes on Linux). uutils was reportingno such directoryinstead.Cause
Path::is_dir()swallows any I/O error and reportsfalse. For a path longer than NAME_MAX the kernel returnsENAMETOOLONG, butis_dir()flattens it tofalse, so the existingNoSuchDirectorybranch always wins.Fix
Probe the path with
std::fs::metadata()first and surface the realio::Errorvia a newChrootError::CannotStatvariant. Theis_dir()check still runs afterward, so the existing "is not a directory" behavior is preserved unchanged.Tests
test_no_such_directory(existing, unchanged) — confirms path that exists but is not a directory still getsno such directory.test_filename_too_long(new) — confirms 256-char NEWROOT now reportsFile name too long (os error 63).cargo test -p coreutils --test tests --features chroot test_chroot→ 20 passed, 0 failed.🤖 Generated with Claude Code