-
Notifications
You must be signed in to change notification settings - Fork 15.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix for grpc.tools #17995 & protobuf #7474 (handle UTF-8 paths in argumentfile) #10200
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,6 +47,7 @@ | |
#include <google/protobuf/stubs/common.h> | ||
#include <google/protobuf/stubs/substitute.h> | ||
#include <google/protobuf/message.h> | ||
#include <google/protobuf/io/io_win32.h> | ||
|
||
namespace google { | ||
namespace protobuf { | ||
|
@@ -113,7 +114,7 @@ void Subprocess::Start(const std::string& program, SearchMode search_mode) { | |
} | ||
|
||
// Setup STARTUPINFO to redirect handles. | ||
STARTUPINFOA startup_info; | ||
STARTUPINFOW startup_info; | ||
ZeroMemory(&startup_info, sizeof(startup_info)); | ||
startup_info.cb = sizeof(startup_info); | ||
startup_info.dwFlags = STARTF_USESTDHANDLES; | ||
|
@@ -125,17 +126,30 @@ void Subprocess::Start(const std::string& program, SearchMode search_mode) { | |
GOOGLE_LOG(FATAL) << "GetStdHandle: " << Win32ErrorMessage(GetLastError()); | ||
} | ||
|
||
// get wide string version of program as the path may contain non-ascii characters | ||
std::wstring wprogram; | ||
if (!io::win32::strings::utf8_to_wcs(program.c_str(), &wprogram)) { | ||
GOOGLE_LOG(FATAL) << "utf8_to_wcs: " << Win32ErrorMessage(GetLastError()); | ||
} | ||
|
||
// Invoking cmd.exe allows for '.bat' files from the path as well as '.exe'. | ||
std::string command_line = "cmd.exe /c \"" + program + "\""; | ||
|
||
// get wide string version of command line as the path may contain non-ascii characters | ||
std::wstring wcommand_line; | ||
if (!io::win32::strings::utf8_to_wcs(command_line.c_str(), &wcommand_line)) { | ||
GOOGLE_LOG(FATAL) << "utf8_to_wcs: " << Win32ErrorMessage(GetLastError()); | ||
} | ||
|
||
// Using a malloc'ed string because CreateProcess() can mutate its second | ||
// parameter. | ||
char* command_line = | ||
portable_strdup(("cmd.exe /c \"" + program + "\"").c_str()); | ||
wchar_t *wcommand_line_copy = _wcsdup(wcommand_line.c_str()); | ||
|
||
// Create the process. | ||
PROCESS_INFORMATION process_info; | ||
|
||
if (CreateProcessA((search_mode == SEARCH_PATH) ? nullptr : program.c_str(), | ||
(search_mode == SEARCH_PATH) ? command_line : nullptr, | ||
if (CreateProcessW((search_mode == SEARCH_PATH) ? NULL : wprogram.c_str(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: change NULL -> nullptr everywhere in the diff (as that seems to be the current codestyle requirement in protobuf codebase?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated the code to use nullptr |
||
(search_mode == SEARCH_PATH) ? wcommand_line_copy : NULL, | ||
nullptr, // process security attributes | ||
nullptr, // thread security attributes | ||
TRUE, // inherit handles? | ||
|
@@ -155,7 +169,7 @@ void Subprocess::Start(const std::string& program, SearchMode search_mode) { | |
|
||
CloseHandleOrDie(stdin_pipe_read); | ||
CloseHandleOrDie(stdout_pipe_write); | ||
free(command_line); | ||
free(wcommand_line_copy); | ||
} | ||
|
||
bool Subprocess::Communicate(const Message& input, Message* output, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is another call to
portable_strdup
inSubProccess::Start
. Does that need to be updated as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. It is used in the non-Windows code and this is a Windows specific fix.