matches/exec: create path integrity check before execution #553
Conversation
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
|
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.
|
Since we cannot test in the exec.rs file, I couldn't test (directly) the |
5f16fc5 to
744e55c
Compare
src/find/matchers/exec.rs
Outdated
| } | ||
| } | ||
| } else { | ||
| return Err("PATH environment variable is not defined.".into()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I didn't find any wrapper or high level API around this. I guess we should use the libc crate for it
There was a problem hiding this comment.
Yeah I wouldn't block this PR over that, it might be a bit of work to wrap libc nicely.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Haha, we don't need to use libc::confstr or do any further validation just remove the else branch
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hello again. Doing a little more research, I found this thread on Quora.
Basically, it says that everything depends on the shell and the OS. There's no way to predict it because it depends on several uncontrollable factors: the variable inheritance process, whether the path uses a fallback, etc. It might be safer to simply fail if the PATH isn't present. If Windows sets the PATH as the current directory, we'll find out by iterating through the PATH and checking it. This way, there's no way for it to run insecurely and we guarantee integrity.
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>
Fixes #518
Create function check_path_integrity which checks
the integrity of the path, with the following criteria:
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