Skip to content

Conversation

@mattsu2020
Copy link
Contributor

Modify path control to enable secure access

related
#9541

- Update chmod recursive logic to use directory file descriptors instead of full paths for subdirectories
- Improves performance, avoids path length issues, and ensures dirfd-relative openat calls
- Add test to verify strace output shows no AT_FDCWD with multi-component paths
Added a spell-checker ignore directive in the chmod test file to suppress false positives for legitimate technical terms used in Unix API calls.
@github-actions
Copy link

github-actions bot commented Dec 3, 2025

GNU testsuite comparison:

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


let log_path = at.plus_as_string("strace.log");

let status = Command::new("strace")
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whether to guarantee the detailed condition of “safely traversing while holding directory FD”
Otherwise, using scripts is not a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

the issue with the current test is that we aren't never certain that it will fail :)
as it is skipped easily


    // Skip test if strace is not available
    if Command::new("strace").arg("-V").output().is_err() {
        eprintln!("strace not found; skipping test_chmod_recursive_uses_dirfd_for_subdirs");
        return;
    }

…instead of skip

Previously, the test_chmod_recursive_uses_dirfd_for_subdirs test skipped gracefully if strace
was unavailable, without failing. This change enforces the strace dependency by failing the
test immediately if strace is not installed or runnable, ensuring the test runs reliably
in environments where it is expected to pass, and preventing silent skips.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

Add installation of strace tool on Ubuntu runners in both individual build/test and feature build/test jobs. This enables tracing system calls during execution, aiding in debugging and performance analysis within the CI/CD pipeline. Updated existing apt-get commands and added conditional steps for Linux-only installations.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

Install strace on ubuntu-latest runners across multiple jobs to enable system call tracing for testing purposes, ensuring compatibility with tests that require this debugging tool. This includes updating package lists in existing installation steps.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

…e-build

Modified the pre-build command to install strace utility for debugging and added -y flag to apt-get install to skip prompts, ensuring non-interactive builds.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

Detect package manager (apt vs apk) to install tzdata and strace
in both Debian/Ubuntu and Alpine *-musl targets. Added fallback
warning for unsupported managers. This ensures strace is available
for targets using Alpine, which doesn't have apt-get.
@sylvestre
Copy link
Contributor

I would prefer you just update the script
No needed to install strace for all jobs

…line strings

Replace escaped multi-line string with triple-quoted string for better readability in Cross.toml.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

Install strace utility in the WSL2 environment to support tracing system calls during testing. Minor update to Cross.toml spell-checker ignore list for consistency with change.
@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)

Updated the WSL2 workflow step to use root shell (wsl-bash-root) for installing strace, removing sudo calls and adding DEBIAN_FRONTEND=noninteractive to prevent prompts. This improves CI reliability by ensuring direct root access and automated, interrupt-free package installation.
Fix WSL2 GitHub Actions workflow by installing strace as the user instead of root for better permission handling, and add "noninteractive" to the spell-checker ignore comment for consistency with the new apt-get command. This ensures the tool is available in the testing environment without unnecessary privilege escalation.
@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

Remove strace package installation from multiple GitHub Actions workflow files (CICD.yml, l10n.yml, wsl2.yml). Strace was historically installed in Ubuntu jobs for debugging system calls, but it's no longer required for the tests and builds, reducing CI setup time and dependencies.
- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.第二个测试产品**ci: add strace installation and fix spell-checker comments in CI files**

- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.
@codspeed-hq
Copy link

codspeed-hq bot commented Dec 5, 2025

CodSpeed Performance Report

Merging #9554 will not alter performance

Comparing mattsu2020:chmod_fix (d950e93) with main (bed7012)

Summary

✅ 126 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 Dec 5, 2025

GNU testsuite comparison:

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

mattsu2020 and others added 2 commits December 5, 2025 18:12
Add a check in check-safe-traversal.sh to ensure recursive chmod operations use dirfd-relative openat calls instead of AT_FDCWD with multi-component paths, preventing potential race conditions. Ignore the corresponding Rust test as it is now covered by this shell script guard.
@mattsu2020
Copy link
Contributor Author

I would prefer you just update the script No needed to install strace for all jobs

Modified util/check-safe-traversal.sh

@github-actions
Copy link

github-actions bot commented Dec 5, 2025

GNU testsuite comparison:

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

@mattsu2020 mattsu2020 requested a review from sylvestre December 5, 2025 10:42
@sylvestre sylvestre merged commit 13ea1fc into uutils:main Dec 5, 2025
129 checks passed
@sylvestre
Copy link
Contributor

thanks :)

@sylvestre
Copy link
Contributor

if you are motivated, we also have #9542 and #9541

@collinfunk
Copy link

collinfunk commented Dec 5, 2025

This implementation will reach file descriptor limits in deep directories, FYI.

$ mkdir -p `python3 -c 'print("a/" * 1024 * 16)'`
$ ./target/debug/chmod -R 777 a
chmod: Too many open files
$ ./target/debug/chown -R collin:collin a
chown: cannot read directory '[long file name trimmed]': Too many open files

@mattsu2020 mattsu2020 deleted the chmod_fix branch December 6, 2025 02:32
mattsu2020 added a commit to mattsu2020/coreutils that referenced this pull request Dec 6, 2025
* feat(chmod): use dirfd for recursive subdirectory traversal

- Update chmod recursive logic to use directory file descriptors instead of full paths for subdirectories
- Improves performance, avoids path length issues, and ensures dirfd-relative openat calls
- Add test to verify strace output shows no AT_FDCWD with multi-component paths

* test(chmod): add spell-check ignore for dirfd, subdirs, openat, FDCWD

Added a spell-checker ignore directive in the chmod test file to suppress false positives for legitimate technical terms used in Unix API calls.

* test(chmod): enforce strace requirement in recursive test, fail fast instead of skip

Previously, the test_chmod_recursive_uses_dirfd_for_subdirs test skipped gracefully if strace
was unavailable, without failing. This change enforces the strace dependency by failing the
test immediately if strace is not installed or runnable, ensuring the test runs reliably
in environments where it is expected to pass, and preventing silent skips.

* ci: install strace in Ubuntu CI jobs for debugging system calls

Add installation of strace tool on Ubuntu runners in both individual build/test and feature build/test jobs. This enables tracing system calls during execution, aiding in debugging and performance analysis within the CI/CD pipeline. Updated existing apt-get commands and added conditional steps for Linux-only installations.

* ci: Add strace installation to Ubuntu-based CI workflows

Install strace on ubuntu-latest runners across multiple jobs to enable system call tracing for testing purposes, ensuring compatibility with tests that require this debugging tool. This includes updating package lists in existing installation steps.

* chore(build): install strace and prevent apt prompts in Cross.toml pre-build

Modified the pre-build command to install strace utility for debugging and added -y flag to apt-get install to skip prompts, ensuring non-interactive builds.

* feat(build): support Alpine-based cross images in pre-build

Detect package manager (apt vs apk) to install tzdata and strace
in both Debian/Ubuntu and Alpine *-musl targets. Added fallback
warning for unsupported managers. This ensures strace is available
for targets using Alpine, which doesn't have apt-get.

* refactor(build): improve pre-build script readability by using multi-line strings

Replace escaped multi-line string with triple-quoted string for better readability in Cross.toml.

* feat(ci): install strace in WSL2 GitHub Actions workflow

Install strace utility in the WSL2 environment to support tracing system calls during testing. Minor update to Cross.toml spell-checker ignore list for consistency with change.

* ci(wsl2): install strace as root with non-interactive apt-get

Updated the WSL2 workflow step to use root shell (wsl-bash-root) for installing strace, removing sudo calls and adding DEBIAN_FRONTEND=noninteractive to prevent prompts. This improves CI reliability by ensuring direct root access and automated, interrupt-free package installation.

* ci: Move strace installation to user shell and update spell ignore

Fix WSL2 GitHub Actions workflow by installing strace as the user instead of root for better permission handling, and add "noninteractive" to the spell-checker ignore comment for consistency with the new apt-get command. This ensures the tool is available in the testing environment without unnecessary privilege escalation.

* chore: ci: remove unused strace installation from CI workflows

Remove strace package installation from multiple GitHub Actions workflow files (CICD.yml, l10n.yml, wsl2.yml). Strace was historically installed in Ubuntu jobs for debugging system calls, but it's no longer required for the tests and builds, reducing CI setup time and dependencies.

* ci: add strace installation and fix spell-checker comments in CI files

- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.第二个测试产品**ci: add strace installation and fix spell-checker comments in CI files**

- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.

* test: add regression guard for recursive chmod dirfd-relative traversal

Add a check in check-safe-traversal.sh to ensure recursive chmod operations use dirfd-relative openat calls instead of AT_FDCWD with multi-component paths, preventing potential race conditions. Ignore the corresponding Rust test as it is now covered by this shell script guard.
romanstingler pushed a commit to romanstingler/coreutils that referenced this pull request Dec 14, 2025
* feat(chmod): use dirfd for recursive subdirectory traversal

- Update chmod recursive logic to use directory file descriptors instead of full paths for subdirectories
- Improves performance, avoids path length issues, and ensures dirfd-relative openat calls
- Add test to verify strace output shows no AT_FDCWD with multi-component paths

* test(chmod): add spell-check ignore for dirfd, subdirs, openat, FDCWD

Added a spell-checker ignore directive in the chmod test file to suppress false positives for legitimate technical terms used in Unix API calls.

* test(chmod): enforce strace requirement in recursive test, fail fast instead of skip

Previously, the test_chmod_recursive_uses_dirfd_for_subdirs test skipped gracefully if strace
was unavailable, without failing. This change enforces the strace dependency by failing the
test immediately if strace is not installed or runnable, ensuring the test runs reliably
in environments where it is expected to pass, and preventing silent skips.

* ci: install strace in Ubuntu CI jobs for debugging system calls

Add installation of strace tool on Ubuntu runners in both individual build/test and feature build/test jobs. This enables tracing system calls during execution, aiding in debugging and performance analysis within the CI/CD pipeline. Updated existing apt-get commands and added conditional steps for Linux-only installations.

* ci: Add strace installation to Ubuntu-based CI workflows

Install strace on ubuntu-latest runners across multiple jobs to enable system call tracing for testing purposes, ensuring compatibility with tests that require this debugging tool. This includes updating package lists in existing installation steps.

* chore(build): install strace and prevent apt prompts in Cross.toml pre-build

Modified the pre-build command to install strace utility for debugging and added -y flag to apt-get install to skip prompts, ensuring non-interactive builds.

* feat(build): support Alpine-based cross images in pre-build

Detect package manager (apt vs apk) to install tzdata and strace
in both Debian/Ubuntu and Alpine *-musl targets. Added fallback
warning for unsupported managers. This ensures strace is available
for targets using Alpine, which doesn't have apt-get.

* refactor(build): improve pre-build script readability by using multi-line strings

Replace escaped multi-line string with triple-quoted string for better readability in Cross.toml.

* feat(ci): install strace in WSL2 GitHub Actions workflow

Install strace utility in the WSL2 environment to support tracing system calls during testing. Minor update to Cross.toml spell-checker ignore list for consistency with change.

* ci(wsl2): install strace as root with non-interactive apt-get

Updated the WSL2 workflow step to use root shell (wsl-bash-root) for installing strace, removing sudo calls and adding DEBIAN_FRONTEND=noninteractive to prevent prompts. This improves CI reliability by ensuring direct root access and automated, interrupt-free package installation.

* ci: Move strace installation to user shell and update spell ignore

Fix WSL2 GitHub Actions workflow by installing strace as the user instead of root for better permission handling, and add "noninteractive" to the spell-checker ignore comment for consistency with the new apt-get command. This ensures the tool is available in the testing environment without unnecessary privilege escalation.

* chore: ci: remove unused strace installation from CI workflows

Remove strace package installation from multiple GitHub Actions workflow files (CICD.yml, l10n.yml, wsl2.yml). Strace was historically installed in Ubuntu jobs for debugging system calls, but it's no longer required for the tests and builds, reducing CI setup time and dependencies.

* ci: add strace installation and fix spell-checker comments in CI files

- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.第二个测试产品**ci: add strace installation and fix spell-checker comments in CI files**

- Install strace package in CICD workflow to support safe traversal verification for utilities like rm, chmod, chown, chgrp, mv, and du, enabling syscall tracing for testing.
- Clean up spell-checker ignore comments in wsl2.yml and Cross.toml by removing misplaced flags.

* test: add regression guard for recursive chmod dirfd-relative traversal

Add a check in check-safe-traversal.sh to ensure recursive chmod operations use dirfd-relative openat calls instead of AT_FDCWD with multi-component paths, preventing potential race conditions. Ignore the corresponding Rust test as it is now covered by this shell script guard.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants