Fix broken escaping of commands in the build

[ldrumm]

Summary

A combination of broken escaping in CMake and in the python swig generation scripts meant that the swig generation step would fail whenever there were spaces or special characters in parameters passed to swig.

The fix for this in CMakeLists is to use the VERBATIM option on all COMMAND-based custom builders relying on CMake to properly escape each argument in the generated file.

Within the python swig scripts, the fix is to call subprocess.Popen with a list of raw argument strings rather than ones that are incorrectly manually escaped, then passed to a shell subprocess via subprocess.Popen(' '.join(params)). This also prevents nasty things happening such as accidental command-injection.

This allows us to have the swig / python executables in paths containing special chars and spaces, (or on shared storage on Win32, e.g \\some\path or C:\Program Files\swig\swig.exe).

[granata.enrico]

Chris, can you take a look at this? I am far from a CMake expert

[granata.enrico]

scripts/Python/prepare_binding_Python.py

229 ↗

(On Diff #78228)

This worries me a little bit.. Are we sure we are not in any way relying on the shell in executing the command line?

[ldrumm]

scripts/Python/prepare_binding_Python.py

229 ↗

(On Diff #78228)

The features of the shell are not used in this expression at all, and the environment is identical to the previous invocation.

[bryant]

CMakeLists.txt

46 ↗

(On Diff #78228)

You can reduce diff noise by leaving formatting changes for a separate patch.

scripts/CMakeLists.txt

38 ↗

(On Diff #78228)

Move this back up.

scripts/Python/prepare_binding_Python.py

222 ↗

(On Diff #78228)

You can reduce diff noise by limiting your changes to removing the %s. So,

python
    # Build the SWIG args list
        options.swig_executable,
        "-c++",
        "-shadow",
        "-python",
        "-threads",
        "-I" + os.path.normcase(
            os.path.join(options.src_root, "include")),
        "-I" + os.path.normcase("./."),
        "-D__STDC_LIMIT_MACROS",
        "-D__STDC_CONSTANT_MACROS"]
    if options.target_platform == "Darwin":
        command.append("-D__APPLE__")
    if options.generate_dependency_file:
        command.extend(["-MMD", " -MF", temp_dep_file_path])
    command.extend([
        "-outdir", config_build_dir,
        "-o", settings.output_file,
        settings.input_file
    ])
    logging.info("running swig with: %s", command)

229 ↗

(On Diff #78228)

shell=False for both python 2 and 3: https://docs.python.org/2/library/subprocess.html#subprocess.Popen ; https://docs.python.org/3/library/subprocess.html#subprocess.Popen , unless I've missed your meaning.

[bryant]

CMakeLists.txt

53 ↗

(On Diff #78228)

The indentation here could match the above.

[ldrumm]

scripts/Python/prepare_binding_Python.py

229 ↗

(On Diff #78228)

@bryant

shell=False for both python 2 and 3: https://docs.python.org/2/library/subprocess.html#subprocess.Popen ; https://docs.python.org/3/library/subprocess.html#subprocess.Popen , unless I've missed your meaning.

Yes - the intended behaviour is to not use the shell on any python

[ldrumm]

CMakeLists.txt

46 ↗

(On Diff #78228)

Seeing as I'm touching all these lines anyway, formatting seems appropriate. I tend to get pushback for formatting only commits, so I'd rather keep this as it is.

53 ↗

(On Diff #78228)

Good point. Will fix

[ldrumm]

scripts/Python/prepare_binding_Python.py

222 ↗

(On Diff #78228)

But logging.info is not a pretty printer - if the command fails for some reason we need to see why. repr allows this, and the diff noise is again minimal because that line is changing anyway and command is now a list, not a string

[ldrumm]

scripts/CMakeLists.txt

38 ↗

(On Diff #78228)

Will do

[bryant]

scripts/Python/prepare_binding_Python.py

222 ↗

(On Diff #78228)

Yes, that was a typo. Keep %r too.

[ldrumm]

scripts/Python/prepare_binding_Python.py

222 ↗

(On Diff #78228)

Sorry - I just realised you meant the whole block (I only caught on slowly beyond the logging call).

I don't see a problem with the current implementation - it has some technical advantages over the existing code:

• The list of args is built as a single expression, which is notionally nicer than modifying it piecemeal
• The definition of the optional constants upfront is done such that they are always defined one way or another
• os.path.normcase(os.path.join(options.src_root, "include")) is incorrect as there can be case-sensitive filesystems on win32
• os.path.normcase("./."), doesn't make sense on any OS I’m aware of (though I need to change this to os.path.curdir as it's possible some OS or filesystem doesn't use ".")

Given that I'm fixing at least 4 broken assumptions here, I feel that the cleanup is worth a little diff noise

[beanz]

The CMake all LGTM.

[ldrumm]

Addressed some unwanted whitespace changes as suggested by @bryant; use os.dir.curdir as instead of .

[granata.enrico]

I am not an Apple employee working on LLDB anymore

[ldrumm]

ping

lldb/trunk/CMakeLists.txt

 endif ()
 add_subdirectory(source)
 add_subdirectory(test)
 add_subdirectory(tools)
 add_subdirectory(unittests)
 add_subdirectory(lit)
 
 if (NOT LLDB_DISABLE_PYTHON)
-    # Add a Post-Build Event to copy over Python files and create the symlink to liblldb.so for the Python API(hardlink on Windows)
-    add_custom_target( finish_swig ALL
-        COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
-        "--srcRoot=${LLDB_SOURCE_DIR}"
-        "--targetDir=${LLDB_PYTHON_TARGET_DIR}"
-        "--cfgBldDir=${CMAKE_CURRENT_BINARY_DIR}/scripts"
-        "--prefix=${CMAKE_BINARY_DIR}"
-        "--cmakeBuildConfiguration=${CMAKE_CFG_INTDIR}"
-        "--lldbLibDir=lib${LLVM_LIBDIR_SUFFIX}" ${FINISH_EXTRA_ARGS}
+    # Add a Post-Build Event to copy over Python files and create the symlink
+    # to liblldb.so for the Python API(hardlink on Windows)
+    add_custom_target(finish_swig ALL
+        COMMAND
+           ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
+               --srcRoot=${LLDB_SOURCE_DIR}
+               --targetDir=${LLDB_PYTHON_TARGET_DIR}
+               --cfgBldDir=${CMAKE_CURRENT_BINARY_DIR}/scripts
+               --prefix=${CMAKE_BINARY_DIR}
+               --cmakeBuildConfiguration=${CMAKE_CFG_INTDIR}
+               --lldbLibDir=lib${LLVM_LIBDIR_SUFFIX}
+               ${FINISH_EXTRA_ARGS}
+        VERBATIM
         DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/scripts/finishSwigWrapperClasses.py
         DEPENDS ${CMAKE_CURRENT_BINARY_DIR}/scripts/lldb.py
         COMMENT "Python script sym-linking LLDB Python API")
     # We depend on liblldb being built before we can do this step.
     add_dependencies(finish_swig liblldb lldb-argdumper)
 
     # If we build the readline module, we depend on that happening
     # first.
if (NOT LLDB_DISABLE_PYTHON)
     # lldb.exe or any other executables that were linked with liblldb.
     if (WIN32 AND NOT "${PYTHON_DLL}" STREQUAL "")
         # When using the Visual Studio CMake generator the lldb binaries end up in Release/bin, Debug/bin etc.
         file(TO_NATIVE_PATH "${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin" LLDB_BIN_DIR)
         file(TO_NATIVE_PATH "${PYTHON_DLL}" PYTHON_DLL_NATIVE_PATH)
         add_custom_command(
             TARGET finish_swig
             POST_BUILD
-            COMMAND "${CMAKE_COMMAND}" -E copy ${PYTHON_DLL_NATIVE_PATH} ${LLDB_BIN_DIR}
+            COMMAND ${CMAKE_COMMAND} -E copy ${PYTHON_DLL_NATIVE_PATH} ${LLDB_BIN_DIR} VERBATIM
             COMMENT "Copying Python DLL to LLDB binaries directory.")
     endif ()
 endif ()

lldb/trunk/scripts/CMakeLists.txt

add_custom_command(
   OUTPUT ${LLDB_WRAP_PYTHON}
   OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/lldb.py
   DEPENDS ${SWIG_SOURCES}
   DEPENDS ${SWIG_INTERFACES}
   DEPENDS ${SWIG_HEADERS}
   DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/Python/prepare_binding_Python.py
   DEPENDS ${CMAKE_CURRENT_SOURCE_DIR}/Python/modify-python-lldb.py
   COMMAND ${PYTHON_EXECUTABLE} ${CMAKE_CURRENT_SOURCE_DIR}/prepare_bindings.py
-          ${framework_arg}
-          "--srcRoot=${LLDB_SOURCE_DIR}"
-          "--targetDir=${LLDB_PYTHON_TARGET_DIR}"
-          "--cfgBldDir=${CMAKE_CURRENT_BINARY_DIR}"
-          "--prefix=${CMAKE_BINARY_DIR}"
-          "--swigExecutable=${SWIG_EXECUTABLE}"
+      --srcRoot=${LLDB_SOURCE_DIR}
+      --targetDir=${LLDB_PYTHON_TARGET_DIR}
+      --cfgBldDir=${CMAKE_CURRENT_BINARY_DIR}
+      --prefix=${CMAKE_BINARY_DIR}
+      --swigExecutable=${SWIG_EXECUTABLE}
+  VERBATIM
   COMMENT "Python script building LLDB Python wrapper")
 set_source_files_properties(${LLDB_WRAP_PYTHON} PROPERTIES GENERATED 1)
 set_source_files_properties(${CMAKE_CURRENT_BINARY_DIR}/lldb.py PROPERTIES GENERATED 1)
 
 add_custom_target(swig_wrapper ALL DEPENDS ${LLDB_WRAP_PYTHON})
 
 # Install the LLDB python module on all operating systems (except Windows)
 if (NOT CMAKE_SYSTEM_NAME MATCHES "Windows")
   install(DIRECTORY ${SWIG_PYTHON_DIR} DESTINATION ${SWIG_INSTALL_DIR})
 endif()
 
 # build Python modules
 add_subdirectory(Python/modules)

lldb/trunk/scripts/Python/prepare_binding_Python.py

def do_swig_rebuild(options, dependency_file, config_build_dir, settings):
     @param config_build_dir used as the output directory used by swig
     @param settings the SwigSettings that specify a number of aspects used
     to configure building the Python binding with swig (mostly paths)
     """
     if options.generate_dependency_file:
         temp_dep_file_path = dependency_file + ".tmp"
 
     # Build the SWIG args list
-    command = [
+    is_darwin = options.target_platform == "Darwin"
+    gen_deps = options.generate_dependency_file
+    darwin_extras = ["-D__APPLE__"] if is_darwin else []
+    deps_args = ["-MMD", "-MF", temp_dep_file_path] if gen_deps else []
+    command = ([
             options.swig_executable,
             "-c++",
             "-shadow",
             "-python",
             "-threads",
-        "-I\"%s\"" % os.path.normcase(
-            os.path.join(options.src_root, "include")),
-        "-I\"%s\"" % os.path.normcase("./."),
+            "-I" + os.path.normpath(os.path.join(options.src_root, "include")),
+            "-I" + os.path.curdir,
             "-D__STDC_LIMIT_MACROS",
-        "-D__STDC_CONSTANT_MACROS"]
-    if options.target_platform == "Darwin":
-        command.append("-D__APPLE__")
-    if options.generate_dependency_file:
-        command.append("-MMD -MF \"%s\"" % temp_dep_file_path)
-    command.extend([
-        "-outdir", "\"%s\"" % config_build_dir,
-        "-o", "\"%s\"" % settings.output_file,
-        "\"%s\"" % settings.input_file
-    ])
-    logging.info("running swig with: %s", command)
+            "-D__STDC_CONSTANT_MACROS"
+        ]
+        + darwin_extras
+        + deps_args
+        + [
+            "-outdir", config_build_dir,
+            "-o", settings.output_file,
+            settings.input_file
+        ]
+    )
+    logging.info("running swig with: %r", command)
 
     # Execute swig
     process = subprocess.Popen(
-        ' '.join(command),
+        command,
         stdout=subprocess.PIPE,
         stderr=subprocess.PIPE,
-        shell=True)
+    )
     # Wait for SWIG process to terminate
     swig_stdout, swig_stderr = process.communicate()
     return_code = process.returncode
     if return_code != 0:
         logging.error(
             "swig failed with error code %d: stdout=%s, stderr=%s",
             return_code,
             swig_stdout,
def run_python_script(script_and_args):
 
     If the command returns anything non-zero, it is registered as
     an error and exits the program.
 
     @param script_and_args the python script to execute, along with
     the command line arguments to pass to it.
     """
     command = [sys.executable] + script_and_args
-    command_line = " ".join(command)
-    process = subprocess.Popen(command, shell=False)
+    process = subprocess.Popen(command)
     script_stdout, script_stderr = process.communicate()
     return_code = process.returncode
     if return_code != 0:
-        logging.error("failed to run '%s': %s", command_line, script_stderr)
+        logging.error("failed to run %r: %r", command, script_stderr)
         sys.exit(return_code)
     else:
-        logging.info("ran script '%s'", command_line)
+        logging.info("ran script %r'", command)
         if script_stdout is not None:
             logging.info("output: %s", script_stdout)
 
 
 def do_modify_python_lldb(options, config_build_dir):
     """Executes the modify-python-lldb.py script.
 
     @param options the parsed command line arguments