[fingolfin]

Claude writing here:

PKGMAN_Exec joins its arguments with spaces and runs the result with sh -c, so an argument with a space is split and one with $(...) or ; is executed. Archive names are the last component of a PackageInfo.g's ArchiveURL, so a crafted URL runs code at tar xf.

gap> PKGMAN_Exec(".", "printf", "[%s]", "a  b", "$(echo injected)").output;
"[a][b][injected]"          # before
"[a  b][$(echo injected)]"  # after

The shell now gets command and arguments as positional parameters (sh -c 'exec "$0" "$@" 2>&1' cmd arg...): it still merges stderr, but never parses them. #104 is unaffected.

Two callers passed "" to mean "no option" (non-GNU tar, hg status); they now pass nothing.

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.38%. Comparing base (4154bd3) to head (c594654).

Files with missing lines	Patch %	Lines
gap/archive.gi	75.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #158   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files          26       26           
  Lines        1313     1313           
=======================================
  Hits         1213     1213           
  Misses        100      100           

Files with missing lines	Coverage Δ
gap/PackageManager.gi	98.60% <100.00%> (-0.01%)	⬇️
gap/archive.gi	92.85% <75.00%> (+0.10%)	⬆️

🚀 New features to boost your workflow:

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

[mtorpey]

I'm having a bit of trouble with this. What is the problem that this PR is attempting to solve, and what is the intention of Concatenation(["-c", "exec \"$0\" \"$@\" 2>&1", fullcmd], on line 302?

[fingolfin]

Claude replying here:

PKGMAN_Exec builds one string, cmd arg1 arg2 ... 2>&1, and runs it with sh -c, so the shell re-parses every argument. An argument with a space is split in two; one with $(...) or ; is executed:

gap> PKGMAN_Exec(".", "printf", "[%s]", "a  b", "$(echo injected)").output;
"[a][b][injected]"          # master
"[a  b][$(echo injected)]"  # this PR

This can happen in practice. For example, a user whose home directory is /Users/Jane Doe/ gets package paths with a space in them, and tar or git is then handed two arguments instead of one.

It is also a potential security issue. The archive name passed to tar xf is the last component of the package's ArchiveURL, which comes from downloaded metadata or from a URL the user was given. An ArchiveURL ending in $(...) would run arbitrary commands on the user's machine during installation.

On the new line: sh -c SCRIPT NAME ARG1 ARG2 ... runs SCRIPT with $0 set to NAME and $1, $2, ... to the arguments. SCRIPT is the fixed string exec "$0" "$@" 2>&1, and "$@" expands to each argument as exactly one word. So the shell runs the command with the arguments as given and only adds the 2>&1; it never parses what we pass. exec saves a fork.

Calling Process directly, without sh, would be simpler, but then stderr is no longer captured (gap-system/gap#4657) and goes to the terminal; git clone, for example, writes its messages there.

I have rebased this onto the new master.

[mtorpey]

Makes sense, and I added a test that now passes because of this.