From faf83bee58e17bcfb1df86c7f21525993e194c03 Mon Sep 17 00:00:00 2001 From: Olly Betts Date: Thu, 7 Jan 2016 00:20:03 +1300 Subject: [PATCH] Escape for CreateProcess(), not cmd.exe We now use wxProcess::Open() to run cavern, which uses wxExecute() which uses CreateProcess(). And use wxExecute() not wxSystem() to start an editor, so that the quoting needed there is the same as that needed for cavern. --- src/aven.cc | 81 ++++++++++++++++++++++++++++++++++++++++++-------------- src/cavernlog.cc | 60 ++++++++++++++++++++++++----------------- 2 files changed, 96 insertions(+), 45 deletions(-) diff --git a/src/aven.cc b/src/aven.cc index 9b0d26e0..7e294d97 100644 --- a/src/aven.cc +++ b/src/aven.cc @@ -4,7 +4,7 @@ // Main class for Aven. // // Copyright (C) 2001 Mark R. Shinwell. -// Copyright (C) 2002,2003,2004,2005,2006,2011,2013,2014,2015 Olly Betts +// Copyright (C) 2002,2003,2004,2005,2006,2011,2013,2014,2015,2016 Olly Betts // // This program is free software; you can redistribute it and/or modify // it under the terms of the GNU General Public License as published by @@ -48,6 +48,10 @@ #include #endif +#ifdef __WXMSW__ +#include +#endif + bool double_buffered = false; static const struct option long_opts[] = { @@ -97,28 +101,62 @@ static char ** utf8_argv; #ifdef __WXMSW__ bool Aven::Initialize(int& my_argc, wxChar **my_argv) { + const wxChar * cmd_line = GetCommandLineW(); + // Horrible bodge to handle therion's assumptions about the "Process" // file association. - if (my_argc == 5 && - wxStrcmp(my_argv[1], wxT("--quiet")) == 0 && - wxStrcmp(my_argv[2], wxT("--log")) == 0 && - wxStrncmp(my_argv[3], wxT("--output="), 9) == 0) { - wxString cavern = my_argv[0]; - int slash = max(cavern.Find(wxT('/'), true), - cavern.Find(wxT('\\'), true)); - // wxNOT_FOUND is -1, so this should work even if there's no slash. - cavern.replace(slash + 1, wxString::npos, wxT("cavern.exe")); - my_argv[0] = const_cast((const wxChar*)cavern.c_str()); - exit(wxExecute(my_argv, wxEXEC_SYNC)); + if (cmd_line) { + // None of these are valid aven command line options, so this is not + // going to be triggered accidentally. + wxChar * p = wxStrStr(cmd_line, "aven.exe\" --quiet --log --output="); + if (p) { + // Just change the command name in the command line string - that + // way the quoting should match what the C runtime expects. + wxString cmd(cmd_line, p - cmd_line); + cmd += "cavern"; + cmd += p + 4; + exit(wxExecute(cmd, wxEXEC_SYNC)); + } } - // wxWidgets passes us wxChars, which may be wide characters but cmdline - // wants UTF-8 so we need to convert. - utf8_argv = new char * [my_argc + 1]; - for (int i = 0; i < my_argc; ++i){ - utf8_argv[i] = strdup(wxString(my_argv[i]).mb_str()); + int utf8_argc; + { + // wxWidgets doesn't split up the command line in the standard way, so + // redo it ourselves using the standard API function. + // + // Warning: The returned array from this has no terminating NULL + // element. + wxChar ** new_argv = NULL; + if (cmd_line) + new_argv = CommandLineToArgvW(cmd_line, &utf8_argc); + bool failed = (new_argv == NULL); + if (failed) { + wxChar * p; + FormatMessage( + FORMAT_MESSAGE_ALLOCATE_BUFFER|FORMAT_MESSAGE_FROM_SYSTEM, + NULL, + GetLastError(), + 0, + (LPWSTR)&p, + 4096, + NULL); + wxString m = "CommandLineToArgvW failed: "; + m += p; + wxMessageBox(m, APP_NAME, wxOK | wxCENTRE | wxICON_EXCLAMATION); + LocalFree(p); + utf8_argc = my_argc; + new_argv = my_argv; + } + + // Convert wide characters to UTF-8. + utf8_argv = new char * [utf8_argc + 1]; + for (int i = 0; i < utf8_argc; ++i){ + utf8_argv[i] = strdup(wxString(new_argv[i]).mb_str()); + } + utf8_argv[utf8_argc] = NULL; + + if (!failed) LocalFree(new_argv); } - utf8_argv[my_argc] = NULL; msg_init(utf8_argv); pj_set_finder(msg_proj_finder); @@ -132,9 +170,12 @@ bool Aven::Initialize(int& my_argc, wxChar **my_argv) * * Part of aven --help */ cmdline_set_syntax_message(/*[SURVEY_FILE]*/269, 0, NULL); - cmdline_init(my_argc, utf8_argv, short_opts, long_opts, NULL, help, 0, 1); + cmdline_init(utf8_argc, utf8_argv, short_opts, long_opts, NULL, help, 0, 1); getopt_first_response = cmdline_getopt(); - return wxApp::Initialize(my_argc, my_argv); + + // The argc and argv arguments don't actually get used here. + int dummy_argc = 0; + return wxApp::Initialize(dummy_argc, NULL); } #else int main(int argc, char **argv) diff --git a/src/cavernlog.cc b/src/cavernlog.cc index 492c7ff0..92f79fbf 100644 --- a/src/cavernlog.cc +++ b/src/cavernlog.cc @@ -157,33 +157,39 @@ END_EVENT_TABLE() static wxString escape_for_shell(wxString s, bool protect_dash = false) { - size_t p = 0; #ifdef __WXMSW__ - // Correct quoting here is insane - you need to quote for CommandLineToArgV - // and then also for cmd.exe: + // Correct quoting rules are insane: + // // http://blogs.msdn.com/b/twistylittlepassagesallalike/archive/2011/04/23/everyone-quotes-arguments-the-wrong-way.aspx - bool needs_quotes = false; - while (p < s.size()) { - wxChar ch = s[p]; - if (ch < 127) { - if (ch == wxT('"')) { - s.insert(p, wxT("\\^")); - p += 2; - needs_quotes = true; - } else if (ch == ' ') { - needs_quotes = true; - } else if (strchr("()%<>&|^", ch)) { - s.insert(p, wxT('^')); - ++p; + // + // Thankfully wxExecute passes the command string to CreateProcess(), so + // at least we don't need to quote for cmd.exe too. + if (s.empty() || s.find_first_of(wxT(" \"\t\n\v")) != s.npos) { + // Need to quote. + s.insert(0, wxT('"')); + for (size_t p = 1; p < s.size(); ++p) { + size_t backslashes = 0; + while (s[p] == wxT('\\')) { + ++backslashes; + if (++p == s.size()) { + // Escape all the backslashes, since they're before + // the closing quote we add below. + s.append(backslashes, wxT('\\')); + goto done; + } + } + + if (s[p] == wxT('"')) { + // Escape any preceding backslashes and this quote. + s.insert(p, backslashes + 1, wxT('\\')); + p += backslashes + 1; } } - ++p; - } - if (needs_quotes) { - s.insert(0u, wxT("^\"")); - s += wxT("^\""); +done: + s.append(wxT('"')); } #else + size_t p = 0; if (protect_dash && !s.empty() && s[0u] == '-') { // If the filename starts with a '-', protect it from being // treated as an option by prepending "./". @@ -348,8 +354,10 @@ CavernLogWindow::OnLinkClicked(const wxHtmlLinkInfo &link) ++i; } } - if (wxSystem(cmd) >= 0) + + if (wxExecute(cmd, wxEXEC_ASYNC|wxEXEC_MAKE_GROUP_LEADER) >= 0) return; + wxString m; // TRANSLATORS: %s is replaced by the command we attempted to run. m.Printf(wmsg(/*Couldn’t run external command: “%s”*/17), cmd.c_str()); @@ -400,11 +408,13 @@ CavernLogWindow::process(const wxString &file) len += len; } /* Strange Win32 nastiness - strip prefix "\\?\" if present */ - if (wcsncmp(buf, L"\\\\?\\", 4) == 0) buf += 4; - wchar_t * slash = wcsrchr(buf, L'\\'); + wchar_t *start = buf; + if (wcsncmp(start, L"\\\\?\\", 4) == 0) start += 4; + wchar_t * slash = wcsrchr(start, L'\\'); if (slash) { - cmd.assign(buf, slash - buf + 1); + cmd.assign(start, slash - start + 1); } + osfree(buf); } cmd += L"cavern"; cmd = escape_for_shell(cmd, false); -- 2.11.4.GIT