findutils icon indicating copy to clipboard operation
findutils copied to clipboard

matches/exec: create path integrity check before execution

Open sami-daniel opened this issue 8 months ago • 5 comments

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

sami-daniel avatar Jun 01 '25 05:06 sami-daniel

Codecov Report

:x: Patch coverage is 87.77293% with 28 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 87.18%. Comparing base (d55e2f9) to head (38f994c). :warning: Report is 52 commits behind head on main.

Files with missing lines Patch % Lines
src/find/matchers/exec.rs 87.77% 26 Missing and 2 partials :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

codecov[bot] avatar Jun 01 '25 07:06 codecov[bot]

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

sylvestre avatar Jun 01 '25 08:06 sylvestre

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

done

sami-daniel avatar Jun 01 '25 21:06 sami-daniel

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.

sami-daniel avatar Jun 02 '25 11:06 sami-daniel

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.

sami-daniel avatar Jun 03 '25 12:06 sami-daniel