From: Aleksander Salwa <34712835+asalwa@users.noreply.github.com> Date: Sat, 20 Mar 2021 18:55:54 +0000 (+0100) Subject: win32: Fix handling of long command lines (#816) X-Git-Tag: v4.2.1~12 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ea433578505af6ae799cdc4dae81516ab0757bca;p=thirdparty%2Fccache.git 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 354c9ae32..b259ed0e9 100644 --- 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 a56387d1f..7e02a4b2a 100644 --- 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 ae47b2f33..b4c039e35 100644 --- 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 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(execv_argv.data())); - throw Fatal("execv of {} failed: {}", execv_argv[0], strerror(errno)); + execute_noreturn(const_cast(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 ec8703391..326d17bce 100644 --- 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(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(argv)); +} #endif std::string diff --git a/src/execute.hpp b/src/execute.hpp index 9814a73c6..628ceb327 100644 --- 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 28aa68778..6115c5ba5 100644 --- 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 # 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 eb7dc8f59..f836171c6 100644 --- 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();