From 46ec7ad73a74c76273fd7520fba324233579898a Mon Sep 17 00:00:00 2001 From: Anupam Mediratta Date: Thu, 24 Sep 2026 08:28:02 +0000 Subject: [PATCH] git-p4: avoid shell interpretation of commit ids in applyCommit applyCommit() builds a `git diff-tree ... | git apply ...` pipeline as a shell command string, interpolating the commit id and running it via os.system()/system(shell=True). The id usually comes from `git rev-list` output (safe, plain SHA-1s), but it can also come verbatim from the user-supplied `--commit` option, which is never validated (git-p4.py:2620-2631). A value such as `$(some-command)` passed to `--commit` is executed by the shell during command substitution, even though the value is wrapped in double quotes. Replace the shell pipeline with two argument-vector subprocess calls connected directly through a pipe, matching the pattern already used throughout this file (read_pipe, read_pipe_lines, p4_system). This removes the shell entirely, rather than relying on quoting the interpolated value. The shell-based call that applied the patch for real went through git-p4.py's own system() helper, which raises CalledProcessError on a non-zero exit status, so a failed apply aborted the submit. Keep that behaviour by giving the new helper the same ignore_error contract system() uses: it raises unless the caller asks for the status, and the two callers that test the status for themselves ask for it. Add a regression test exercising `git p4 submit --commit` with a shell metacharacter payload, verifying it is never interpreted. Signed-off-by: Anupam Mediratta Signed-off-by: Junio C Hamano --- git-p4.py | 35 ++++++++++++++++++++++--------- t/t9803-git-p4-shell-metachars.sh | 16 ++++++++++++++ 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/git-p4.py b/git-p4.py index c0ca7becaf..e716831554 100755 --- a/git-p4.py +++ b/git-p4.py @@ -465,6 +465,26 @@ def p4_system(cmd, *k, **kw): raise subprocess.CalledProcessError(retcode, real_cmd) +def diffTreeApply(id, applyArgs, ignore_error=False): + """Pipe `git diff-tree --full-index -p ` into `git apply ` + without a shell, so id can never be interpreted as shell syntax. Returns + the exit status of git apply, raising CalledProcessError on a non-zero + status unless ignore_error is set.""" + diffArgv = ["git", "diff-tree", "--full-index", "-p", id] + applyArgv = ["git", "apply"] + applyArgs + if verbose: + print("TryPatch: %s | %s" % (" ".join(diffArgv), " ".join(applyArgv))) + diffProc = subprocess.Popen(diffArgv, stdout=subprocess.PIPE) + applyProc = subprocess.Popen(applyArgv, stdin=diffProc.stdout) + diffProc.stdout.close() + applyProc.wait() + diffProc.wait() + retcode = applyProc.returncode + if retcode and not ignore_error: + raise subprocess.CalledProcessError(retcode, applyArgv) + return retcode + + def die_bad_access(s): die("failure accessing depot: {0}".format(s.rstrip())) @@ -2234,16 +2254,11 @@ class P4Submit(Command, P4UserMap): else: die("unknown modifier %s for %s" % (modifier, path)) - diffcmd = "git diff-tree --full-index -p \"%s\"" % (id) - patchcmd = diffcmd + " | git apply " - tryPatchCmd = patchcmd + "--check -" - applyPatchCmd = patchcmd + "--check --apply -" + tryPatchArgs = ["--check", "-"] + applyPatchArgs = ["--check", "--apply", "-"] patch_succeeded = True - if verbose: - print("TryPatch: %s" % tryPatchCmd) - - if os.system(tryPatchCmd) != 0: + if diffTreeApply(id, tryPatchArgs, ignore_error=True) != 0: fixed_rcs_keywords = False patch_succeeded = False print("Unfortunately applying the change failed!") @@ -2279,7 +2294,7 @@ class P4Submit(Command, P4UserMap): if fixed_rcs_keywords: print("Retrying the patch with RCS keywords cleaned up") - if os.system(tryPatchCmd) == 0: + if diffTreeApply(id, tryPatchArgs, ignore_error=True) == 0: patch_succeeded = True print("Patch succeesed this time with RCS keywords cleaned") @@ -2291,7 +2306,7 @@ class P4Submit(Command, P4UserMap): # # Apply the patch for real, and do add/delete/+x handling. # - system(applyPatchCmd, shell=True) + diffTreeApply(id, applyPatchArgs) for f in filesToChangeType: p4_edit(f, "-t", "auto") diff --git a/t/t9803-git-p4-shell-metachars.sh b/t/t9803-git-p4-shell-metachars.sh index 2913277013..ef8fd6e094 100755 --- a/t/t9803-git-p4-shell-metachars.sh +++ b/t/t9803-git-p4-shell-metachars.sh @@ -105,4 +105,20 @@ test_expect_success 'branch with shell char' ' ) ' +test_expect_success 'git p4 submit --commit does not execute shell metachars in commit id' ' + git p4 clone --dest="$git" //depot && + test_when_finished cleanup_git && + ( + cd "$git" && + git config git-p4.skipSubmitEditCheck true && + echo f3 >file3 && + git add file3 && + git commit -m "add file3" && + name='"'"'$(touch${IFS}injection-marker)'"'"' && + git branch "$name" HEAD && + P4EDITOR="test-tool chmtime +5" git p4 submit --commit "$name" + ) && + test_path_is_missing "$cli/injection-marker" +' + test_done