perl: harden external command invocations#21097
Closed
vszakats wants to merge 18 commits intocurl:masterfrom
Closed
perl: harden external command invocations#21097vszakats wants to merge 18 commits intocurl:masterfrom
vszakats wants to merge 18 commits intocurl:masterfrom
Conversation
This reverts commit f9f2ea27a42827be8dc255a7863d3dfaa8ba55b1. Too crazy and not fixing much
``` test 1275...[Verify capital letters after period in markdown files] /usr/bin/perl -I. -I/__w/curl/curl/tests returned 2, when expecting 0 1275: exit FAILED == Contents of files in the log/3/ directory after test 1275 === Start of file commands.log /usr/bin/perl -I. -I/__w/curl/curl/tests /__w/curl/curl/tests/test1275.pl /__w/curl/curl/tests/.. > log/3/stdout1275 2> log/3/stderr1275 === End of file commands.log === Start of file server.cmd Testnum 1275 === End of file server.cmd === Start of file stderr1275 Can't exec "git": No such file or directory at /__w/curl/curl/tests/test1275.pl line 31. Died at /__w/curl/curl/tests/test1275.pl line 31. ``` https://github.com/curl/curl/actions/runs/23556683852/job/68585280936?pr=21097
This comment was marked as resolved.
This comment was marked as resolved.
This reverts commit b95176b. It works, but eats resources better used elsewhere. This test is running the same regardless of platform and build settings, and it's already running in most jobs. Installing git in the few remaining ones is just waste.
There was a problem hiding this comment.
Pull request overview
This PR hardens several Perl scripts and test helpers by avoiding shell-interpreted command strings (backticks / open("|") / system("...")) and switching to list-form execution or Perl library calls, reducing injection risk and quoting pitfalls.
Changes:
- Replace shell-based command invocations with
open(..., '-|', ...)in multiple test scripts and tooling scripts. - Update test1707-related test definitions to match the new
test1707.plargument list (no intermediate output file). - Replace
mvshell calls withFile::Copy::move()inadddocsref.pl.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test1707.pl | Capture curl -h output via piped open() instead of shell redirection to a file. |
| tests/test1275.pl | Replace backticks git ls-files with piped open() (but currently can still silently no-op if git is missing). |
| tests/libtest/test307.pl | Use list-form piped open() for invoking curl to list engines. |
| tests/libtest/test1013.pl | Use list-form piped open() for invoking curl-config via sh. |
| tests/libtest/test1022.pl | Use list-form piped open() for invoking curl-config via sh. |
| tests/data/test1707 | Update test command line for the new test1707.pl args. |
| tests/data/test1708 | Update test command line for the new test1707.pl args. |
| tests/data/test1710 | Update test command line for the new test1707.pl args. |
| scripts/singleuse.pl | Harden git grep and nm invocations by using list-form piped open(). |
| scripts/checksrc-all.pl | Use list-form piped open() for git ls-files (but still contains a shell-based system() call). |
| docs/examples/adddocsref.pl | Replace mv shell commands with File::Copy::move(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In
adddocsref.pl,checksrc-all.pl,singleuse.pland tests 307,1013, 1022, 1275, 1707, 1708, 1710.
gitcommand is missing.Keep sliently skipping? or install git where missing? maybe make it not need git?