matches/exec: create path integrity check before execution #553

sami-daniel · 2025-06-01T05:31:59Z

Fixes #518

Create function check_path_integrity which checks
the integrity of the path, with the following criteria:

Only absolute paths
Only non-empty paths
Only directories in path

If the PATH env var does not pass the criteria, an error is returned warning about the specific path part that caused that error.

It was not possible to write tests for the changes, only to check if there were regressions. To test the changes, it would be necessary to change the PATH at test runtime. The only way to do this is through the operating system API wrapper provided in std::env::set_var. In order to use this API, you must ensure that no other thread is reading or writing from any place other than a single place, thus avoiding data races and concurrency.
Unfortunately, the command engine that findutils currently use to execute other commands together with find comes from std::process::Command, and it uses the PATH to find the command to be executed. lazy_static would not work in this case either. A possible solution would be to change the CI's to run only cargo test -- test-threads=1, but I think the tradeoff would not be worth it for such a simple change. Besides the fact that other tests are reading variables that we just changed, then probably many executables would not be found

codecov · 2025-06-01T07:47:46Z

Codecov Report

Attention: Patch coverage is 87.77293% with 28 lines in your changes missing coverage. Please review.

Project coverage is 87.18%. Comparing base (d55e2f9) to head (38f994c).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/find/matchers/exec.rs	87.77%	26 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #553      +/-   ##
==========================================
+ Coverage   87.15%   87.18%   +0.02%     
==========================================
  Files          31       31              
  Lines        6300     6529     +229     
  Branches      324      328       +4     
==========================================
+ Hits         5491     5692     +201     
- Misses        578      604      +26     
- Partials      231      233       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sylvestre · 2025-06-01T08:28:17Z

could you also please add tests (with error mgmt)? thanks

sami-daniel · 2025-06-01T21:46:47Z

could you also please add tests (with error mgmt)? thanks

done

sami-daniel · 2025-06-02T11:47:52Z

could you also please add tests (with error mgmt)? thanks

done

Ok, I will fix these errors that are occurring on Windows before sending another push.

Fixes uutils#518 Create function check_path_integrity which checks the integrity of the path, with the following criteria: - Only absolute paths - Only non-empty paths - Only directories in path If the PATH env var does not pass the criteria, an error is returned warning about the specific path part that caused that error. Tests: - Added tests for both SingleExecMatcher and MultiExecMatcher - Covered all PATH validation scenarios: * Valid absolute directories * Empty path segments * Relative path segments * File paths instead of directories - Ensured safe environment variable handling with unsafe blocks - Maintained consistent test patterns with existing serial tests - Verified correct error handling for invalid PATH configurations To avoid making the function that performs the check public, tests were also added that verify that there is no error with a valid PATH.

sami-daniel · 2025-06-03T12:01:12Z

Since we cannot test in the exec.rs file, I couldn't test (directly) the check_path_integrity function. So, the codecov will probably point out an error at this point. However, tests have been added to validate the correct behavior of the Matchers creation to work around this issue.

tests/exec_unit_tests.rs

src/find/matchers/exec.rs

tests/exec_unit_tests.rs

src/find/matchers/exec.rs

tavianator · 2025-06-11T13:53:40Z

src/find/matchers/exec.rs

+            }
+        }
+    } else {
+        return Err("PATH environment variable is not defined.".into());


This shouldn't be an error. If PATH is unset, there is a default value to use, which in C is confstr(_CS_PATH). Not sure what it should be on Windows.

I didn't find any wrapper or high level API around this. I guess we should use the libc crate for it

Yeah I wouldn't block this PR over that, it might be a bit of work to wrap libc nicely.

The problem is _CS_PATH may not be available on non-posix compliant system. I guess we should use libc::confstr(...) on Unix and on Windows we can use C:\Windows\system32;C\Windows as a PATH fallback.

Haha, we don't need to use libc::confstr or do any further validation just remove the else branch

Good point! Generally the default $PATH is only absolute paths, nothing to worry about. On Windows I think by default the current directory is searched.

I guess there are two possibilities.
First: PATH = None, it will not validate anything. There is no risk, because without PATH, there is no way to have something invalid.

Second: Windows, even without PATH, creates a "fake" PATH and runs the process. If there is an invalid path, it will fail the check. I think the point here is: What is the behavior of Command when it gets to it and the PATH is empty? Will it use confstr in Posix compliant and in Windows on will use current directory or will it not search because it does not have the PATH?

I think we can explore the internals of Command, but I did not find any documentation on this.

tavianator · 2025-06-11T13:56:21Z

src/find/matchers/exec.rs

+                "echo",
+                &["test"],
+                true,
+                Some(OsString::from_vec(vec![


Presumably you could use b"\x2F\x00\x75...".into_vec() or something like that. But this test is ignored anyway so maybe just delete it?

I think it works as a documentation about the expected behavior. Like, the engine should work even in environments that don't use utf8 as encoding. Let's suppose, on a machine where the PATH is stored as EBCDIC, the check should work normally. But since we can't test this directly...

renamed: check_path_integrity -> check_path_entries_absolute The function check_path_entries_absolute now only looks for paths that are not absolute or that are empty. In addition, if possible, if there is any error related to the PATH, show the exact segment where the error occurred. tests: The tests that used to reside in exec_unit_tests.rs are now in exec.rs, directly handling a fake of the PATH for testing, eliminating the need to mark tests with #[serial] or use unsafe { env::set_var(...) }. Co-authored-by: Tavian <tavianator@tavianator.com>

sami-daniel force-pushed the main branch from d13e458 to 06a27fb Compare June 2, 2025 21:11

sami-daniel force-pushed the main branch from 06a27fb to af3aee9 Compare June 3, 2025 11:47

tavianator suggested changes Jun 10, 2025

View reviewed changes

sami-daniel requested a review from tavianator June 10, 2025 18:00

tavianator reviewed Jun 10, 2025

View reviewed changes

src/find/matchers/exec.rs Outdated Show resolved Hide resolved

tavianator reviewed Jun 10, 2025

View reviewed changes

src/find/matchers/exec.rs Outdated Show resolved Hide resolved

sami-daniel force-pushed the main branch 5 times, most recently from 5f16fc5 to 744e55c Compare June 11, 2025 13:00

tavianator reviewed Jun 11, 2025

View reviewed changes

sami-daniel force-pushed the main branch from 744e55c to a06828b Compare June 11, 2025 23:38

sami-daniel force-pushed the main branch from a06828b to 38f994c Compare June 12, 2025 11:34

matches/exec: create path integrity check before execution #553

Are you sure you want to change the base?

matches/exec: create path integrity check before execution #553

Uh oh!

Conversation

sami-daniel commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sylvestre commented Jun 1, 2025

Uh oh!

sami-daniel commented Jun 1, 2025

Uh oh!

sami-daniel commented Jun 2, 2025

Uh oh!

sami-daniel commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

sami-daniel commented Jun 1, 2025 •

edited

Loading

codecov bot commented Jun 1, 2025 •

edited

Loading