win32: Fix handling of long command lines (#816)

What is broken: handling of "execute" with long command parameters (see win32execute).

In more details:

* parameter lpCommandLine was formatted incorrectly (it has to contain app name too)
* temporary file with parameters to the compiler was formatted incorrectly (it
  should NOT contain app name; if it contains backslashes, then these
  backslashes need to be escaped as double-backslashes)
* wrong location (directory) of temporary files in win32execute
* premature deletion of temporary files in win32execute

diff --git a/src/Win32Util.cpp b/src/Win32Util.cpp
index 354c9ae32e08c9d009f954bb4b1180cb8542581f..b259ed0e98d159215fa61c0e9e1118df80863a89 100644 (file)
--- a/src/Win32Util.cpp
+++ b/src/Win32Util.cpp
@@ -55,7 +55,9 @@ error_message(DWORD error_code)
 }
 
 std::string
-argv_to_string(const char* const* argv, const std::string& prefix)
+argv_to_string(const char* const* argv,
+               const std::string& prefix,
+               bool escape_backslashes)
 {
   std::string result;
   size_t i = 0;
@@ -67,9 +69,11 @@ argv_to_string(const char* const* argv, const std::string& prefix)
     for (size_t j = 0; arg[j]; ++j) {
       switch (arg[j]) {
       case '\\':
-        ++bs;
-        break;
-      // Fallthrough.
+        if (!escape_backslashes) {
+          ++bs;
+          break;
+        }
+        // Fallthrough.
       case '"':
         bs = (bs << 1) + 1;
       // Fallthrough.

diff --git a/src/Win32Util.hpp b/src/Win32Util.hpp
index a56387d1f02ef07e153d22ada4b6b0cbfbc14549..7e02a4b2a2e9ebd6f83c6d59dab86261721fe838 100644 (file)
--- a/src/Win32Util.hpp
+++ b/src/Win32Util.hpp
@@ -30,7 +30,11 @@ std::string add_exe_suffix(const std::string& program);
 
 // Recreate a Windows command line string based on `argv`. If `prefix` is
 // non-empty, add it as the first argument.
-std::string argv_to_string(const char* const* argv, const std::string& prefix);
+// If escape_backslashes is true, emit additional backslash for each backslash
+// which is not preceding '"' and is not at the end of argv[i] either.
+std::string argv_to_string(const char* const* argv,
+                           const std::string& prefix,
+                           bool escape_backslashes = false);
 
 // Return the error message corresponding to `error_code`.
 std::string error_message(DWORD error_code);

diff --git a/src/ccache.cpp b/src/ccache.cpp
index ae47b2f33076af83f97d64abd1b0dd7671562793..b4c039e3597116f8575db7073496316075366faa 100644 (file)
--- a/src/ccache.cpp
+++ b/src/ccache.cpp
@@ -783,10 +783,10 @@ do_execute(Context& ctx,
     DEBUG_ASSERT(ctx.config.compiler_type() == CompilerType::gcc);
     args.erase_last("-fdiagnostics-color");
   }
-  int status = execute(args.to_argv().data(),
+  int status = execute(ctx,
+                       args.to_argv().data(),
                        std::move(tmp_stdout.fd),
-                       std::move(tmp_stderr.fd),
-                       &ctx.compiler_pid);
+                       std::move(tmp_stderr.fd));
   if (status != 0 && !ctx.diagnostics_color_failed
       && ctx.config.compiler_type() == CompilerType::gcc) {
     auto errors = Util::read_file(tmp_stderr.path);
@@ -2286,6 +2286,7 @@ cache_compilation(int argc, const char* const* argv)
   bool fall_back_to_original_compiler = false;
   Args saved_orig_args;
   nonstd::optional<mode_t> original_umask;
+  std::string saved_temp_dir;
 
   {
     Context ctx;
@@ -2321,10 +2322,12 @@ cache_compilation(int argc, const char* const* argv)
 
       LOG_RAW("Failed; falling back to running the real compiler");
 
+      saved_temp_dir = ctx.config.temporary_dir();
       saved_orig_args = std::move(ctx.orig_args);
       auto execv_argv = saved_orig_args.to_argv();
       LOG("Executing {}", Util::format_argv_for_logging(execv_argv.data()));
-      // Run execv below after ctx and finalizer have been destructed.
+      // Run execute_noreturn below after ctx and finalizer have been
+      // destructed.
     }
   }
 
@@ -2333,8 +2336,10 @@ cache_compilation(int argc, const char* const* argv)
       umask(*original_umask);
     }
     auto execv_argv = saved_orig_args.to_argv();
-    execv(execv_argv[0], const_cast<char* const*>(execv_argv.data()));
-    throw Fatal("execv of {} failed: {}", execv_argv[0], strerror(errno));
+    execute_noreturn(const_cast<char* const*>(execv_argv.data()),
+                     saved_temp_dir);
+    throw Fatal(
+      "execute_noreturn of {} failed: {}", execv_argv[0], strerror(errno));
   }
 
   return EXIT_SUCCESS;

diff --git a/src/execute.cpp b/src/execute.cpp
index ec8703391d760fa55011f65058fb948d73540447..326d17bce000af4c76e7649b280181dd69b6a711 100644 (file)
--- a/src/execute.cpp
+++ b/src/execute.cpp
@@ -36,10 +36,28 @@
 using nonstd::string_view;
 
 #ifdef _WIN32
+static int win32execute(const char* path,
+                        const char* const* argv,
+                        int doreturn,
+                        int fd_stdout,
+                        int fd_stderr,
+                        const std::string& temp_dir);
+
 int
-execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* /*pid*/)
+execute(Context& ctx, const char* const* argv, Fd&& fd_out, Fd&& fd_err)
+{
+  return win32execute(argv[0],
+                      argv,
+                      1,
+                      fd_out.release(),
+                      fd_err.release(),
+                      ctx.config.temporary_dir());
+}
+
+void
+execute_noreturn(const char* const* argv, const std::string& temp_dir)
 {
-  return win32execute(argv[0], argv, 1, fd_out.release(), fd_err.release());
+  win32execute(argv[0], argv, 0, -1, -1, temp_dir);
 }
 
 std::string
@@ -70,7 +88,8 @@ win32execute(const char* path,
              const char* const* argv,
              int doreturn,
              int fd_stdout,
-             int fd_stderr)
+             int fd_stderr,
+             const std::string& temp_dir)
 {
   PROCESS_INFORMATION pi;
   memset(&pi, 0x00, sizeof(pi));
@@ -109,10 +128,12 @@ win32execute(const char* path,
   std::string full_path = Win32Util::add_exe_suffix(path);
   std::string tmp_file_path;
   if (args.length() > 8192) {
-    TemporaryFile tmp_file(path);
+    TemporaryFile tmp_file(FMT("{}/cmd_args", temp_dir));
+    args = Win32Util::argv_to_string(argv + 1, sh, true);
     Util::write_fd(*tmp_file.fd, args.data(), args.length());
-    args = FMT("\"@{}\"", tmp_file.path);
+    args = FMT("{} @{}", full_path, tmp_file.path);
     tmp_file_path = tmp_file.path;
+    LOG("args from file {}", tmp_file.path);
   }
   BOOL ret = CreateProcess(full_path.c_str(),
                            const_cast<char*>(args.c_str()),
@@ -124,9 +145,6 @@ win32execute(const char* path,
                            nullptr,
                            &si,
                            &pi);
-  if (!tmp_file_path.empty()) {
-    Util::unlink_tmp(tmp_file_path);
-  }
   if (fd_stdout != -1) {
     close(fd_stdout);
     close(fd_stderr);
@@ -137,10 +155,17 @@ win32execute(const char* path,
         full_path,
         Win32Util::error_message(error),
         error);
+    if (!tmp_file_path.empty()) {
+      Util::unlink_tmp(tmp_file_path);
+    }
     return -1;
   }
   WaitForSingleObject(pi.hProcess, INFINITE);
 
+  if (!tmp_file_path.empty()) {
+    Util::unlink_tmp(tmp_file_path);
+  }
+
   DWORD exitcode;
   GetExitCodeProcess(pi.hProcess, &exitcode);
   CloseHandle(pi.hProcess);
@@ -156,20 +181,20 @@ win32execute(const char* path,
 // Execute a compiler backend, capturing all output to the given paths the full
 // path to the compiler to run is in argv[0].
 int
-execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* pid)
+execute(Context& ctx, const char* const* argv, Fd&& fd_out, Fd&& fd_err)
 {
   LOG("Executing {}", Util::format_argv_for_logging(argv));
 
   {
     SignalHandlerBlocker signal_handler_blocker;
-    *pid = fork();
+    ctx.compiler_pid = fork();
   }
 
-  if (*pid == -1) {
+  if (ctx.compiler_pid == -1) {
     throw Fatal("Failed to fork: {}", strerror(errno));
   }
 
-  if (*pid == 0) {
+  if (ctx.compiler_pid == 0) {
     // Child.
     dup2(*fd_out, STDOUT_FILENO);
     fd_out.close();
@@ -184,7 +209,7 @@ execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* pid)
   int status;
   int result;
 
-  while ((result = waitpid(*pid, &status, 0)) != *pid) {
+  while ((result = waitpid(ctx.compiler_pid, &status, 0)) != ctx.compiler_pid) {
     if (result == -1 && errno == EINTR) {
       continue;
     }
@@ -193,7 +218,7 @@ execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* pid)
 
   {
     SignalHandlerBlocker signal_handler_blocker;
-    *pid = 0;
+    ctx.compiler_pid = 0;
   }
 
   if (WEXITSTATUS(status) == 0 && WIFSIGNALED(status)) {
@@ -202,6 +227,12 @@ execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* pid)
 
   return WEXITSTATUS(status);
 }
+
+void
+execute_noreturn(const char* const* argv, const std::string& /* unused */)
+{
+  execv(argv[0], const_cast<char* const*>(argv));
+}
 #endif
 
 std::string

diff --git a/src/execute.hpp b/src/execute.hpp
index 9814a73c69a874179137c6a017426236b199a490..628ceb32753e529a01665474a50a146ef654c84e 100644 (file)
--- a/src/execute.hpp
+++ b/src/execute.hpp
@@ -26,7 +26,9 @@
 
 class Context;
 
-int execute(const char* const* argv, Fd&& fd_out, Fd&& fd_err, pid_t* pid);
+int execute(Context& ctx, const char* const* argv, Fd&& fd_out, Fd&& fd_err);
+
+void execute_noreturn(const char* const* argv, const std::string& temp_dir);
 
 // Find an executable named `name` in `$PATH`. Exclude any executables that are
 // links to `exclude_name`.
@@ -40,9 +42,4 @@ std::string find_executable_in_path(const std::string& name,
 
 #ifdef _WIN32
 std::string win32getshell(const std::string& path);
-int win32execute(const char* path,
-                 const char* const* argv,
-                 int doreturn,
-                 int fd_stdout,
-                 int fd_stderr);
 #endif

diff --git a/src/system.hpp b/src/system.hpp
index 28aa6877809214ff4e1ec4295ee903a3855c2c5a..6115c5ba5a214776c70de308ce5307342c3e6c0d 100644 (file)
--- a/src/system.hpp
+++ b/src/system.hpp
@@ -160,7 +160,9 @@ const mode_t S_IWUSR = mode_t(_S_IWRITE);
 #  define NOMINMAX 1
 #  include <windows.h>
 #  define mkdir(a, b) _mkdir(a)
-#  define execv(a, b) win32execute(a, b, 0, -1, -1)
+#  define execv(a, b)                                                          \
+    do_not_call_execv_on_windows // to protect against incidental use of MinGW
+                                 // execv
 #  define strncasecmp _strnicmp
 #  define strcasecmp _stricmp
 

diff --git a/unittest/test_Win32Util.cpp b/unittest/test_Win32Util.cpp
index eb7dc8f59020dacc33b126304b01926b1840f253..f836171c61c7977f3a7cb3917576a95fd70acdce 100644 (file)
--- a/unittest/test_Win32Util.cpp
+++ b/unittest/test_Win32Util.cpp
@@ -38,6 +38,23 @@ TEST_CASE("Win32Util::argv_to_string")
     CHECK(Win32Util::argv_to_string(argv, "")
           == R"("a" "b c" "\"d\"" "'e'" "\\\"h")");
   }
+  {
+    const char* const argv[] = {"a\\b\\c", nullptr};
+    CHECK(Win32Util::argv_to_string(argv, "") == R"("a\b\c")");
+  }
+  {
+    const char* const argv[] = {"a\\b\\c", nullptr};
+    CHECK(Win32Util::argv_to_string(argv, "", true) == R"("a\\b\\c")");
+  }
+  {
+    const char* const argv[] = {R"(a\b \"c\" \)", nullptr};
+    CHECK(Win32Util::argv_to_string(argv, "") == R"("a\b \\\"c\\\" \\")");
+  }
+  {
+    const char* const argv[] = {R"(a\b \"c\" \)", nullptr};
+    CHECK(Win32Util::argv_to_string(argv, "", true)
+          == R"("a\\b \\\"c\\\" \\")");
+  }
 }
 
 TEST_SUITE_END();