fix: Use systemCall instead of shellCall wherever possible

Author: sfayer

src/DIRAC/Core/Security/VOMS.py

@@ -4,15 +4,14 @@
 from datetime import datetime
 import os
 import tempfile
-import shlex
 import shutil
 
 from DIRAC import S_OK, S_ERROR, gConfig, rootPath, gLogger
 from DIRAC.Core.Utilities import DErrno
 from DIRAC.Core.Security import Locations
 from DIRAC.Core.Security.ProxyFile import multiProxyArgument, deleteMultiProxy
 from DIRAC.Core.Security.X509Chain import X509Chain  # pylint: disable=import-error
-from DIRAC.Core.Utilities.Subprocess import shellCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 from DIRAC.Core.Utilities import List
 
 # This is a variable so it can be monkeypatched in tests
@@ -267,9 +266,9 @@ def setVOMSAttributes(self, proxy, attribute=None, vo=None):
 
         cmd = voms_init_cmd(vo, attribute, chain, proxyLocation, newProxyLocation, self.getVOMSESLocation())
 
-        result = shellCall(
+        result = systemCall(
             self._secCmdTimeout,
-            shlex.join(cmd),
+            cmd,
             env=os.environ
             | {
                 "X509_CERT_DIR": Locations.getCAsLocation(),
@@ -313,7 +312,7 @@ def vomsInfoAvailable(self):
         if not vpInfoCmd:
             return S_ERROR(DErrno.EVOMS, "Missing voms-proxy-info")
         cmd = f"{vpInfoCmd} -h"
-        result = shellCall(self._secCmdTimeout, cmd)
+        result = systemCall(self._secCmdTimeout, cmd.split())
         if not result["OK"]:
             return False
         status, _output, _error = result["Value"]

src/DIRAC/Core/Utilities/Grid.py

@@ -3,7 +3,7 @@
 """
 
 from DIRAC.Core.Utilities.ReturnValues import S_ERROR, S_OK
-from DIRAC.Core.Utilities.Subprocess import shellCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 
 
 def ldapsearchBDII(filt=None, attr=None, host=None, base=None, selectionString="Glue"):
@@ -34,8 +34,8 @@ def ldapsearchBDII(filt=None, attr=None, host=None, base=None, selectionString="
     if isinstance(attr, list):
         attr = " ".join(attr)
 
-    cmd = f'ldapsearch -x -LLL -o ldif-wrap=no -H ldap://{host} -b {base} "{filt}" {attr}'
-    result = shellCall(0, cmd)
+    cmd = ["ldapsearch", "-x", "-LLL", "-o", "ldif-wrap=no", "-H", f"ldap://{host}", "-b", base, filt, attr]
+    result = systemCall(0, cmd)
 
     response = []
 

src/DIRAC/Core/Utilities/Os.py

@@ -8,7 +8,7 @@
 
 import DIRAC
 from DIRAC.Core.Utilities import List
-from DIRAC.Core.Utilities.Subprocess import shellCall, systemCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 
 DEBUG = 0
 
@@ -36,19 +36,17 @@ def getDiskSpace(path=".", exclude=None):
 
     if not os.path.exists(path):
         return -1
-    comm = f"df -P -m {path} "
+    comm = ["df", "-P", "-m", path]
     if exclude:
-        comm += f"-x {exclude} "
-    comm += "| tail -1"
-    resultDF = shellCall(10, comm)
+        comm.extend(["-x", exclude])
+    resultDF = systemCall(10, comm)
     if not resultDF["OK"] or resultDF["Value"][0]:
         return -1
-    output = resultDF["Value"][1]
+    output = resultDF["Value"][1].strip().splitlines()[-1]
     if output.find(" /afs") >= 0:  # AFS disk space
-        comm = "fs lq | tail -1"
-        resultAFS = shellCall(10, comm)
+        resultAFS = systemCall(10, ["fs", "lq"])
         if resultAFS["OK"] and not resultAFS["Value"][0]:
-            output = resultAFS["Value"][1]
+            output = resultAFS["Value"][1].strip().splitlines()[-1]
             fields = output.split()
             quota = int(fields[1])
             used = int(fields[2])
@@ -67,8 +65,7 @@ def getDiskSpace(path=".", exclude=None):
 def getDirectorySize(path):
     """Get the total size of the given directory in MB"""
 
-    comm = f"du -s -m {path}"
-    result = shellCall(10, comm)
+    result = systemCall(10, ["du", "-s", "-m", path])
     if not result["OK"] or result["Value"][0] != 0:
         return 0
     output = result["Value"][1]

src/DIRAC/FrameworkSystem/Service/SystemAdministratorHandler.py

@@ -25,7 +25,7 @@
 from DIRAC.Core.Utilities import Os
 from DIRAC.Core.Utilities.Extensions import extensionsByPriority, getExtensionMetadata
 from DIRAC.Core.Utilities.File import mkLink
-from DIRAC.Core.Utilities.Subprocess import shellCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 from DIRAC.Core.Utilities.ThreadScheduler import gThreadScheduler
 from DIRAC.Core.Utilities.TimeUtilities import day, fromString, hour
 from DIRAC.FrameworkSystem.Client.ComponentInstaller import gComponentInstaller
@@ -471,7 +471,7 @@ def export_addOptionToDiracCfg(self, option, value):
 
     def export_executeCommand(self, command):
         """Execute a command locally and return its output"""
-        result = shellCall(60, command)
+        result = systemCall(60, command.split())
         return result
 
     types_checkComponentLog = [[str, list]]

src/DIRAC/Resources/Storage/RFIOStorage.py

@@ -8,7 +8,7 @@
 from DIRAC import gLogger, S_OK, S_ERROR
 from DIRAC.Resources.Storage.Utilities import checkArgumentFormat
 from DIRAC.Resources.Storage.StorageBase import StorageBase
-from DIRAC.Core.Utilities.Subprocess import shellCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 from DIRAC.Core.Utilities.List import breakListIntoChunks
 from DIRAC.Core.Utilities.File import getSize
 
@@ -47,10 +47,7 @@ def exists(self, path):
             return res
         urls = res["Value"]
         gLogger.debug(f"RFIOStorage.exists: Determining the existance of {len(urls)} files.")
-        comm = "nsls -d"
-        for url in urls:
-            comm = f" {comm} {url}"
-        res = shellCall(self.timeout, comm)
+        res = systemCall(self.timeout, ["nsls", "-d"] + urls)
         successful = {}
         failed = {}
         if res["OK"]:
@@ -83,10 +80,7 @@ def isFile(self, path):
         gLogger.debug(f"RFIOStorage.isFile: Determining whether {len(urls)} paths are files.")
         successful = {}
         failed = {}
-        comm = "nsls -ld"
-        for url in urls:
-            comm = f" {comm} {url}"
-        res = shellCall(self.timeout, comm)
+        res = systemCall(self.timeout, ["nsls", "-ld"] + urls)
         if not res["OK"]:
             return res
         returncode, stdout, stderr = res["Value"]
@@ -110,10 +104,7 @@ def isFile(self, path):
 
     def __getPathMetadata(self, urls):
         gLogger.debug(f"RFIOStorage.__getPathMetadata: Attempting to get metadata for {len(urls)} paths.")
-        comm = "nsls -ld"
-        for url in urls:
-            comm = f" {comm} {url}"
-        res = shellCall(self.timeout, comm)
+        res = systemCall(self.timeout, ["nsls", "-ld"] + urls)
         successful = {}
         failed = {}
         if not res["OK"]:
@@ -155,12 +146,12 @@ def __permissionsToInt(self, permissions):
     def __getFileMetadata(self, urls):
         gLogger.debug(f"RFIOStorage.__getPathMetadata: Attempting to get additional metadata for {len(urls)} files.")
         # Check whether the files that exist are staged
-        comm = f"stager_qry -S {self.spaceToken}"
+        cmd = ["stager_qry", "-S", self.spaceToken]
         successful = {}
         for pfn in urls:
             successful[pfn] = {}
-            comm = f"{comm} -M {pfn}"
-        res = shellCall(self.timeout, comm)
+            cmd.extend(["-M", pfn])
+        res = systemCall(self.timeout, cmd)
         if not res["OK"]:
             errStr = "RFIOStorage.__getFileMetadata: Completely failed to get cached status."
             gLogger.error(errStr, res["Message"])
@@ -177,10 +168,8 @@ def __getFileMetadata(self, urls):
                 successful[pfn]["Cached"] = False
 
         # Now for the files that exist get the tape segment (i.e. whether they have been migrated) and related checksum
-        comm = "nsls -lT --checksum"
-        for pfn in urls:
-            comm = f"{comm} {pfn}"
-        res = shellCall(self.timeout, comm)
+        cmd = ["nsls", "-lT", "--checksum"] + list(urls)
+        res = systemCall(self.timeout, cmd)
         if not res["OK"]:
             errStr = "RFIOStorage.__getFileMetadata: Completely failed to get migration status."
             gLogger.error(errStr, res["Message"])
@@ -240,8 +229,8 @@ def __getFile(self, src_url, dest_file):
         MIN_BANDWIDTH = 1024 * 100  # 100 KB/s
         timeout = int(remoteSize / MIN_BANDWIDTH + 300)
         gLogger.debug(f"RFIOStorage.getFile: Executing transfer of {src_url} to {dest_file}")
-        comm = f"rfcp {src_url} {dest_file}"
-        res = shellCall(timeout, comm)
+        comm = ["rfcp", src_url, dest_file]
+        res = systemCall(timeout, comm)
         if res["OK"]:
             returncode, _stdout, stderr = res["Value"]
             if returncode == 0:
@@ -317,8 +306,8 @@ def __putFile(self, src_file, dest_url, sourceSize):
         MIN_BANDWIDTH = 1024 * 100  # 100 KB/s
         timeout = sourceSize / MIN_BANDWIDTH + 300
         gLogger.debug(f"RFIOStorage.putFile: Executing transfer of {src_file} to {turl}")
-        comm = f"rfcp {src_file} '{turl}'"
-        res = shellCall(timeout, comm)
+        comm = ["rfcp", src_file, turl]
+        res = systemCall(timeout, comm)
         if res["OK"]:
             returncode, _stdout, stderr = res["Value"]
             if returncode == 0:
@@ -357,17 +346,17 @@ def removeFile(self, path):
         listOfLists = breakListIntoChunks(urls, 100)
         for urls in listOfLists:
             gLogger.debug(f"RFIOStorage.removeFile: Attempting to remove {len(urls)} files.")
-            comm = f"stager_rm -S {self.spaceToken}"
+            comm = ["stager_rm", "-S", self.spaceToken]
             for url in urls:
-                comm = f"{comm} -M {url}"
-            res = shellCall(100, comm)
+                comm.extend(["-M", url])
+            res = systemCall(100, comm)
             if res["OK"]:
                 returncode, _stdout, stderr = res["Value"]
                 if returncode in [0, 1]:
-                    comm = "nsrm -f"
+                    comm = ["nsrm", "-f"]
                     for url in urls:
-                        comm = f"{comm} {url}"
-                    res = shellCall(100, comm)
+                        comm.append(url)
+                    res = systemCall(100, comm)
                     if res["OK"]:
                         returncode, _stdout, stderr = res["Value"]
                         if returncode in [0, 1]:
@@ -461,10 +450,10 @@ def prestageFile(self, path):
             return res
         urls = res["Value"]
         userTag = f"{self.spaceToken}-{time.time()}"
-        comm = f"stager_get -S {self.spaceToken} -U {userTag} "
+        comm = ["stager_get", "-S", self.spaceToken, "-U", userTag]
         for url in urls:
-            comm = f"{comm} -M {url}"
-        res = shellCall(100, comm)
+            comm.extend(["-M", url])
+        res = systemCall(100, comm)
         successful = {}
         failed = {}
         if res["OK"]:
@@ -502,8 +491,8 @@ def prestageFileStatus(self, path):
                 requestFiles[requestID] = []
             requestFiles[requestID].append(url)
         for requestID, urls in requestFiles.items():
-            comm = f"stager_qry -S {self.spaceToken} -U {requestID} "
-            res = shellCall(100, comm)
+            comm = ["stager_qry", "-S", self.spaceToken, "-U", requestID]
+            res = systemCall(100, comm)
             if res["OK"]:
                 returncode, stdout, stderr = res["Value"]
                 if returncode in [0, 1]:
@@ -799,8 +788,8 @@ def createDirectory(self, path):
 
     def __makeDir(self, path):
         # First create a local file that will be used as a directory place holder in storage name space
-        comm = f"nsmkdir -m 775 {path}"
-        res = shellCall(100, comm)
+        comm = ["nsmkdir", "-m", "775", path]
+        res = systemCall(100, comm)
         if not res["OK"]:
             return res
         returncode, _stdout, stderr = res["Value"]
@@ -841,8 +830,8 @@ def removeDirectory(self, path, recursive=False):
         successful = {}
         failed = {}
         for url in urls:
-            comm = f"nsrm -r {url}"
-            res = shellCall(100, comm)
+            comm = ["nsrm", "-r", url]
+            res = systemCall(100, comm)
             if res["OK"]:
                 returncode, _stdout, stderr = res["Value"]
                 if returncode == 0:
@@ -880,8 +869,8 @@ def listDirectory(self, path):
                 failed[url] = errStr
 
         for directory in directories:
-            comm = f"nsls -l {directory}"
-            res = shellCall(self.timeout, comm)
+            comm = ["nsls", "-l", directory]
+            res = systemCall(self.timeout, comm)
             if res["OK"]:
                 returncode, stdout, stderr = res["Value"]
                 if not returncode == 0:

src/DIRAC/TransformationSystem/Client/TransformationCLI.py

@@ -6,7 +6,7 @@
 
 from DIRAC.Core.Base.CLI import CLI
 from DIRAC.Core.Base.API import API
-from DIRAC.Core.Utilities.Subprocess import shellCall
+from DIRAC.Core.Utilities.Subprocess import systemCall
 from DIRAC.TransformationSystem.Client import TransformationFilesStatus
 from DIRAC.TransformationSystem.Client.Transformation import Transformation
 from DIRAC.TransformationSystem.Client.TransformationClient import TransformationClient
@@ -73,8 +73,7 @@ def do_shell(self, args):
 
         usage !<shell_command>
         """
-        comm = args
-        res = shellCall(0, comm)
+        res = systemCall(0, args.split())
         if res["OK"] and res["Value"][0] == 0:
             _returnCode, stdOut, stdErr = res["Value"]
             print(f"{stdOut}\n{stdErr}")