@matheusbsbs / matheus-bagliano / commits / 8a470599f3

git-p4: pass command arguments as lists instead of using shell

In the majority of the subprocess calls where shell=True was used, it was only needed to parse command arguments by spaces. In each of these cases, the commands are now being passed in as lists instead of strings. This change aids the comprehensibility of the code. Constucting commands and arguments using strings risks bugs from unsanitized inputs, and the attendant complexity of properly quoting and escaping command arguments. Signed-off-by: Joel Holdsworth <jholdsworth@nvidia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Joel Holdsworth committed Jan 6, 2022 at 21:40 UTC 8a470599f340e2e2bcf96f0b267199de321edef2

1 file changed +43 -62

git-p4.py

+43 -62

@@ -96,10 +96,7 @@ def p4_build_cmd(cmd):

96 # Provide a way to not pass this option by setting git-p4.retries to 0

97 real_cmd += ["-r", str(retries)]

99 - if not isinstance(cmd, list):

100 - real_cmd = ' '.join(real_cmd) + ' ' + cmd

101 - else:

102 - real_cmd += cmd

99 + real_cmd += cmd

100

101 # now check that we can actually talk to the server

102 global p4_access_checked

@@ -721,12 +718,7 @@ def isModeExecChanged(src_mode, dst_mode):

718 def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,

719 errors_as_exceptions=False, *k, **kw):

720

724 - if not isinstance(cmd, list):

725 - cmd = "-G " + cmd

726 - else:

727 - cmd = ["-G"] + cmd

728 -

729 - cmd = p4_build_cmd(cmd)

721 + cmd = p4_build_cmd(["-G"] + cmd)

722 if verbose:

723 sys.stderr.write("Opening pipe: %s\n" % str(cmd))

724

@@ -849,7 +841,7 @@ def isValidGitDir(path):

841 return git_dir(path) != None

842

843 def parseRevision(ref):

852 - return read_pipe("git rev-parse %s" % ref, shell=True).strip()

844 + return read_pipe(["git", "rev-parse", ref]).strip()

845

846 def branchExists(ref):

847 rev = read_pipe(["git", "rev-parse", "-q", "--verify", ref],

@@ -955,13 +947,13 @@ def p4BranchesInGit(branchesAreInRemotes=True):

947

948 branches = {}

949

958 - cmdline = "git rev-parse --symbolic "

950 + cmdline = ["git", "rev-parse", "--symbolic"]

951 if branchesAreInRemotes:

960 - cmdline += "--remotes"

952 + cmdline.append("--remotes")

953 else:

962 - cmdline += "--branches"

954 + cmdline.append("--branches")

955

964 - for line in read_pipe_lines(cmdline, shell=True):

956 + for line in read_pipe_lines(cmdline):

957 line = line.strip()

958

959 # only import to p4/

@@ -1024,7 +1016,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", silent

1016

1017 originPrefix = "origin/p4/"

1018

1027 - for line in read_pipe_lines("git rev-parse --symbolic --remotes", shell=True):

1019 + for line in read_pipe_lines(["git", "rev-parse", "--symbolic", "--remotes"]):

1020 line = line.strip()

1021 if (not line.startswith(originPrefix)) or line.endswith("HEAD"):

1022 continue

@@ -1062,8 +1054,7 @@ def createOrUpdateBranchesFromOrigin(localRefPrefix = "refs/remotes/p4/", silent

1054 remoteHead, ','.join(settings['depot-paths'])))

1055

1056 if update:

1065 - system("git update-ref %s %s" % (remoteHead, originHead),

1066 - shell=True)

1057 + system(["git", "update-ref", remoteHead, originHead])

1058

1059 def originP4BranchesExist():

1060 return gitBranchExists("origin") or gitBranchExists("origin/p4") or gitBranchExists("origin/p4/master")

@@ -1177,7 +1168,7 @@ def getClientSpec():

1168 """Look at the p4 client spec, create a View() object that contains

1169 all the mappings, and return it."""

1170

1180 - specList = p4CmdList("client -o", shell=True)

1171 + specList = p4CmdList(["client", "-o"])

1172 if len(specList) != 1:

1173 die('Output from "client -o" is %d lines, expecting 1' %

1174 len(specList))

@@ -1206,7 +1197,7 @@ def getClientSpec():

1197 def getClientRoot():

1198 """Grab the client directory."""

1199

1209 - output = p4CmdList("client -o", shell=True)

1200 + output = p4CmdList(["client", "-o"])

1201 if len(output) != 1:

1202 die('Output from "client -o" is %d lines, expecting 1' % len(output))

1203

@@ -1461,7 +1452,7 @@ class P4UserMap:

1452 if self.myP4UserId:

1453 return self.myP4UserId

1454

1464 - results = p4CmdList("user -o", shell=True)

1455 + results = p4CmdList(["user", "-o"])

1456 for r in results:

1457 if 'User' in r:

1458 self.myP4UserId = r['User']

@@ -1486,7 +1477,7 @@ class P4UserMap:

1477 self.users = {}

1478 self.emails = {}

1479

1489 - for output in p4CmdList("users", shell=True):

1480 + for output in p4CmdList(["users"]):

1481 if "User" not in output:

1482 continue

1483 self.users[output["User"]] = output["FullName"] + " <" + output["Email"] + ">"

@@ -1684,7 +1675,7 @@ class P4Submit(Command, P4UserMap):

1675 die("Large file system not supported for git-p4 submit command. Please remove it from config.")

1676

1677 def check(self):

1687 - if len(p4CmdList("opened ...", shell=True)) > 0:

1678 + if len(p4CmdList(["opened", "..."])) > 0:

1679 die("You have files opened with perforce! Close them before starting the sync.")

1680

1681 def separate_jobs_from_description(self, message):

@@ -1788,7 +1779,7 @@ class P4Submit(Command, P4UserMap):

1779 # then gets used to patch up the username in the change. If the same

1780 # client spec is being used by multiple processes then this might go

1781 # wrong.

1791 - results = p4CmdList("client -o", shell=True) # find the current client

1782 + results = p4CmdList(["client", "-o"]) # find the current client

1783 client = None

1784 for r in results:

1785 if 'Client' in r:

@@ -1804,7 +1795,7 @@ class P4Submit(Command, P4UserMap):

1795

1796 def modifyChangelistUser(self, changelist, newUser):

1797 # fixup the user field of a changelist after it has been submitted.

1807 - changes = p4CmdList("change -o %s" % changelist, shell=True)

1798 + changes = p4CmdList(["change", "-o", changelist])

1799 if len(changes) != 1:

1800 die("Bad output from p4 change modifying %s to user %s" %

1801 (changelist, newUser))

@@ -1815,7 +1806,7 @@ class P4Submit(Command, P4UserMap):

1806 # p4 does not understand format version 3 and above

1807 input = marshal.dumps(c, 2)

1808

1818 - result = p4CmdList("change -f -i", stdin=input, shell=True)

1809 + result = p4CmdList(["change", "-f", "-i"], stdin=input)

1810 for r in result:

1811 if 'code' in r:

1812 if r['code'] == 'error':

@@ -1921,7 +1912,7 @@ class P4Submit(Command, P4UserMap):

1912 if "P4EDITOR" in os.environ and (os.environ.get("P4EDITOR") != ""):

1913 editor = os.environ.get("P4EDITOR")

1914 else:

1924 - editor = read_pipe("git var GIT_EDITOR", shell=True).strip()

1915 + editor = read_pipe(["git", "var", "GIT_EDITOR"]).strip()

1916 system(["sh", "-c", ('%s "$@"' % editor), editor, template_file])

1917

1918 # If the file was not saved, prompt to see if this patch should

@@ -1980,8 +1971,7 @@ class P4Submit(Command, P4UserMap):

1971 (p4User, gitEmail) = self.p4UserForCommit(id)

1972

1973 diff = read_pipe_lines(

1983 - "git diff-tree -r %s \"%s^\" \"%s\"" % (self.diffOpts, id, id),

1984 - shell=True)

1974 + ["git", "diff-tree", "-r"] + self.diffOpts + ["{}^".format(id), id])

1975 filesToAdd = set()

1976 filesToChangeType = set()

1977 filesToDelete = set()

@@ -2467,17 +2457,17 @@ class P4Submit(Command, P4UserMap):

2457 #

2458 if self.detectRenames:

2459 # command-line -M arg

2470 - self.diffOpts = "-M"

2460 + self.diffOpts = ["-M"]

2461 else:

2462 # If not explicitly set check the config variable

2463 detectRenames = gitConfig("git-p4.detectRenames")

2464

2465 if detectRenames.lower() == "false" or detectRenames == "":

2476 - self.diffOpts = ""

2466 + self.diffOpts = []

2467 elif detectRenames.lower() == "true":

2478 - self.diffOpts = "-M"

2468 + self.diffOpts = ["-M"]

2469 else:

2480 - self.diffOpts = "-M%s" % detectRenames

2470 + self.diffOpts = ["-M{}".format(detectRenames)]

2471

2472 # no command-line arg for -C or --find-copies-harder, just

2473 # config variables

@@ -2485,12 +2475,12 @@ class P4Submit(Command, P4UserMap):

2475 if detectCopies.lower() == "false" or detectCopies == "":

2476 pass

2477 elif detectCopies.lower() == "true":

2488 - self.diffOpts += " -C"

2478 + self.diffOpts.append("-C")

2479 else:

2490 - self.diffOpts += " -C%s" % detectCopies

2480 + self.diffOpts.append("-C{}".format(detectCopies))

2481

2482 if gitConfigBool("git-p4.detectCopiesHarder"):

2493 - self.diffOpts += " --find-copies-harder"

2483 + self.diffOpts.append("--find-copies-harder")

2484

2485 num_shelves = len(self.update_shelve)

2486 if num_shelves > 0 and num_shelves != len(commits):

@@ -3436,12 +3426,9 @@ class P4Sync(Command, P4UserMap):

3426 lostAndFoundBranches = set()

3427

3428 user = gitConfig("git-p4.branchUser")

3439 - if len(user) > 0:

3440 - command = "branches -u %s" % user

3441 - else:

3442 - command = "branches"

3429

3444 - for info in p4CmdList(command, shell=True):

3430 + for info in p4CmdList(

3431 + ["branches"] + (["-u", user] if len(user) > 0 else [])):

3432 details = p4Cmd(["branch", "-o", info["branch"]])

3433 viewIdx = 0

3434 while "View%s" % viewIdx in details:

@@ -3532,9 +3519,8 @@ class P4Sync(Command, P4UserMap):

3519 while True:

3520 if self.verbose:

3521 print("trying: earliest %s latest %s" % (earliestCommit, latestCommit))

3535 - next = read_pipe(

3536 - "git rev-list --bisect %s %s" % (latestCommit, earliestCommit),

3537 - shell=True).strip()

3522 + next = read_pipe(["git", "rev-list", "--bisect",

3523 + latestCommit, earliestCommit]).strip()

3524 if len(next) == 0:

3525 if self.verbose:

3526 print("argh")

@@ -3689,7 +3675,7 @@ class P4Sync(Command, P4UserMap):

3675 if self.hasOrigin:

3676 if not self.silent:

3677 print('Syncing with origin first, using "git fetch origin"')

3692 - system("git fetch origin", shell=True)

3678 + system(["git", "fetch", "origin"])

3679

3680 def importHeadRevision(self, revision):

3681 print("Doing initial import of %s from revision %s into %s" % (' '.join(self.depotPaths), revision, self.branch))

@@ -3856,8 +3842,8 @@ class P4Sync(Command, P4UserMap):

3842 if len(self.branch) == 0:

3843 self.branch = self.refPrefix + "master"

3844 if gitBranchExists("refs/heads/p4") and self.importIntoRemotes:

3859 - system("git update-ref %s refs/heads/p4" % self.branch, shell=True)

3860 - system("git branch -D p4", shell=True)

3845 + system(["git", "update-ref", self.branch, "refs/heads/p4"])

3846 + system(["git", "branch", "-D", "p4"])

3847

3848 # accept either the command-line option, or the configuration variable

3849 if self.useClientSpec:

@@ -4060,7 +4046,7 @@ class P4Sync(Command, P4UserMap):

4046 # Cleanup temporary branches created during import

4047 if self.tempBranches != []:

4048 for branch in self.tempBranches:

4063 - read_pipe("git update-ref -d %s" % branch, shell=True)

4049 + read_pipe(["git", "update-ref", "-d", branch])

4050 os.rmdir(os.path.join(os.environ.get("GIT_DIR", ".git"), self.tempBranchLocation))

4051

4052 # Create a symbolic ref p4/HEAD pointing to p4/<branch> to allow

@@ -4092,7 +4078,7 @@ class P4Rebase(Command):

4078 def rebase(self):

4079 if os.system("git update-index --refresh") != 0:

4080 die("Some files in your working directory are modified and different than what is in your index. You can use git update-index <filename> to bring the index up to date or stash away all your changes with git stash.");

4095 - if len(read_pipe("git diff-index HEAD --", shell=True)) > 0:

4081 + if len(read_pipe(["git", "diff-index", "HEAD", "--"])) > 0:

4082 die("You have uncommitted changes. Please commit them before rebasing or stash them away with git stash.");

4083

4084 [upstream, settings] = findUpstreamBranchPoint()

@@ -4103,10 +4089,10 @@ class P4Rebase(Command):

4089 upstream = re.sub("~[0-9]+$", "", upstream)

4090

4091 print("Rebasing the current branch onto %s" % upstream)

4106 - oldHead = read_pipe("git rev-parse HEAD", shell=True).strip()

4107 - system("git rebase %s" % upstream, shell=True)

4108 - system("git diff-tree --stat --summary -M %s HEAD --" % oldHead,

4109 - shell=True)

4092 + oldHead = read_pipe(["git", "rev-parse", "HEAD"]).strip()

4093 + system(["git", "rebase", upstream])

4094 + system(["git", "diff-tree", "--stat", "--summary", "-M", oldHead,

4095 + "HEAD", "--"])

4096 return True

4097

4098 class P4Clone(P4Sync):

@@ -4183,7 +4169,7 @@ class P4Clone(P4Sync):

4169

4170 # auto-set this variable if invoked with --use-client-spec

4171 if self.useClientSpec_from_options:

4186 - system("git config --bool git-p4.useclientspec true", shell=True)

4172 + system(["git", "config", "--bool", "git-p4.useclientspec", "true"])

4173

4174 return True

4175

@@ -4314,10 +4300,7 @@ class P4Branches(Command):

4300 if originP4BranchesExist():

4301 createOrUpdateBranchesFromOrigin()

4302

4317 - cmdline = "git rev-parse --symbolic "

4318 - cmdline += " --remotes"

4319 -

4320 - for line in read_pipe_lines(cmdline, shell=True):

4303 + for line in read_pipe_lines(["git", "rev-parse", "--symbolic", "--remotes"]):

4304 line = line.strip()

4305

4306 if not line.startswith('p4/') or line == "p4/HEAD":

@@ -4402,11 +4385,9 @@ def main():

4385 cmd.gitdir = os.path.abspath(".git")

4386 if not isValidGitDir(cmd.gitdir):

4387 # "rev-parse --git-dir" without arguments will try $PWD/.git

4405 - cmd.gitdir = read_pipe(

4406 - "git rev-parse --git-dir", shell=True).strip()

4388 + cmd.gitdir = read_pipe(["git", "rev-parse", "--git-dir"]).strip()

4389 if os.path.exists(cmd.gitdir):

4408 - cdup = read_pipe(

4409 - "git rev-parse --show-cdup", shell=True).strip()

4390 + cdup = read_pipe(["git", "rev-parse", "--show-cdup"]).strip()

4391 if len(cdup) > 0:

4392 chdir(cdup);

4393