Skip to content

chroot: use execvp directly instead of process::Command#9013

Merged
sylvestre merged 3 commits intouutils:mainfrom
mattsu2020:fix_chroot
Dec 26, 2025
Merged

chroot: use execvp directly instead of process::Command#9013
sylvestre merged 3 commits intouutils:mainfrom
mattsu2020:fix_chroot

Conversation

@mattsu2020
Copy link
Contributor

@mattsu2020 mattsu2020 commented Oct 25, 2025 •
edited
Loading

  • Replace process::Command execution with direct execvp call via libc for improved performance by avoiding process forking
  • Add validation to detect and error on commands containing null bytes
  • Capture and handle execution errors using the last OS error after execvp failure

fix this issue
#9010

@Arcterus
Copy link
Collaborator

Arcterus commented Oct 25, 2025 •
edited
Loading

Rather than using execvp() directly, it'd probably be better to rely on the Command::exec() function provided by CommandExt.

CommandExt actually provides a number of things we do here manually with libc, so we might be able to move e.g. the actual chroot() and uid/gid changes and so on to the stdlib.

@codspeed-hq
Copy link

codspeed-hq bot commented Oct 25, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #9013 will not alter performance

Comparing mattsu2020:fix_chroot (55a3100) with main (cc103ec)

Summary

✅ 127 untouched
⏩ 6 skipped1

Footnotes

  1. 6 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. ↩

@github-actions
Copy link

github-actions bot commented Oct 25, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@mattsu2020
Copy link
Contributor Author

mattsu2020 commented Oct 25, 2025

Rather than using execvp() directly, it'd probably be better to rely on the Command::exec() function provided by CommandExt.

CommandExt actually provides a number of things we do here manually with libc, so we might be able to move e.g. the actual chroot() and uid/gid changes and so on to the stdlib.

I made some revisions based on your feedback.

@github-actions
Copy link

github-actions bot commented Oct 25, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

@fulalas
Copy link

fulalas commented Oct 25, 2025

I confirm this change fixes #9010

Nice job! :)

@sylvestre
Copy link
Contributor

sylvestre commented Oct 25, 2025

would it be possible to add a test? thanks

@github-actions
Copy link

github-actions bot commented Oct 27, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)

@mattsu2020
Copy link
Contributor Author

mattsu2020 commented Oct 30, 2025

would it be possible to add a test? thanks

Done

@github-actions
Copy link

github-actions bot commented Nov 29, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)

@Ecordonnier
Copy link
Collaborator

Ecordonnier commented Dec 9, 2025

@mattsu2020 I suggest replacing the word "refactor" with "chroot" in the title of this PR.

@Ecordonnier Ecordonnier mentioned this pull request Dec 9, 2025
Merged
@fulalas fulalas mentioned this pull request Dec 9, 2025
Closed
@mattsu2020 mattsu2020 changed the title refactor: use execvp directly instead of process::Command chroot: use execvp directly instead of process::Command Dec 9, 2025
Ecordonnier
Ecordonnier reviewed Dec 9, 2025
View reviewed changes
src/uu/chroot/src/chroot.rs Outdated
use std::os::unix::process::CommandExt;
use std::path::{Path, PathBuf};
use std::process;
use std::process::Command as ProcessCommand;
Copy link
Collaborator

@Ecordonnier Ecordonnier Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to not mix cosmetic changes (ProcessCommand instead of process::Command) with functional fixes.

Copy link
Contributor Author

@mattsu2020 mattsu2020 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I restored it to its original state.

@github-actions
Copy link

github-actions bot commented Dec 9, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/cksum/b2sum is no longer failing!

@github-actions
Copy link

github-actions bot commented Dec 24, 2025

GNU testsuite comparison:

Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.

@sylvestre sylvestre merged commit 57d3fce into uutils:main Dec 26, 2025
127 checks passed
@mattsu2020 mattsu2020 deleted the fix_chroot branch December 26, 2025 23:30
@moonfruit moonfruit mentioned this pull request Feb 3, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Ecordonnier Ecordonnier Ecordonnier left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mattsu2020 @Arcterus @fulalas @sylvestre @Ecordonnier