Fix for grpc.tools #17995 & protobuf #7474 (handle UTF-8 paths in argumentfile)

[tonydnewell]

Fix for #7474 (and grpc/grpc#17995)

When calling the plugins in subprocess.cc Subprocess::Start() fix to use CreateProcessW instead of CreateProcessA to handle non-ascii paths.

See grpc/grpc#17995 (comment) for a summary of what the actual problem is and why the fix looks like the way it does.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[jtattermusch]

@acozzette can you please review? This is an important fix for gRPC's codegen support and you seems to be the author of the argumentFile parsing logic in protoc.

[jtattermusch]

@tonydnewell just to clarify:

• based on the analysis Grpc.Tools: Accented characters in nuget path breaks c# grpc tool compiler grpc/grpc#17995 (comment) and the changes made here, it seems that the only problematic part is if the protoc plugin's full path (grpc_csharp_plugin in our case) contains non-ASCII characters. IIUC if other arguments (e.g. the .proto file names) contain non-ascii characters in their path, it isn't currently a problem (did you verify that?), because the logic for handling UTF-8 file names for other params already exists. Only for the protoc plugin arg, things are different since the invocation of the protoc plugin happens through subprocess.cc
• Based on your investigation, can you tell whether the problem is only specific to when using the argument file (protoc.exe @test.rsp) as originally reported or if there are other ways of reproducing it (e.g. invoking protoc.exe with args on command line normally)?

[jtattermusch]

CC @jskeet

[jtattermusch]

nit: change NULL -> nullptr everywhere in the diff (as that seems to be the current codestyle requirement in protobuf codebase?)

[tonydnewell]

Updated the code to use nullptr

[tonydnewell]

@tonydnewell just to clarify:

• based on the analysis Grpc.Tools: Accented characters in nuget path breaks c# grpc tool compiler grpc/grpc#17995 (comment) and the changes made here, it seems that the only problematic part is if the protoc plugin's full path (grpc_csharp_plugin in our case) contains non-ASCII characters. IIUC if other arguments (e.g. the .proto file names) contain non-ascii characters in their path, it isn't currently a problem (did you verify that?), because the logic for handling UTF-8 file names for other params already exists. Only for the protoc plugin arg, things are different since the invocation of the protoc plugin happens through subprocess.cc

It is just the plugin path that is an issue (these are the paths used in subprocess.cc). All other paths/names seem to work ok as they use the functions in io_win32.cc which do the utf-8 to wide string conversion correctly.

I've tested with non-ASCII proto_path and non-ASCII .proto file name. Here is one of my test input rsp files:

--plugin=protoc-gen-grpc=E:\work\grpc\protoctest\test-Dré\tools\grpc_csharp_plugin.exe
--grpc_out=E:\work\grpc\protoctest\test-Dré\grpc_out
--proto_path=E:\work\grpc\protoctest\test-Dré
hélloworld.proto

• Based on your investigation, can you tell whether the problem is only specific to when using the argument file (protoc.exe @test.rsp) as originally reported or if there are other ways of reproducing it (e.g. invoking protoc.exe with args on command line normally)?

The reported problem was with using an utf-8 encoding in an argument file and this is what I've tested and is what Grpc.Tools uses.

In theory it should be the same with command line arguments as all that reading the argument file does (in ExpandArgumentFile) is push the lines read onto the same vector of command line arguments before the arguments are interpreted. However this does not seem to work - I think the arguments are getting changed before passing to main(int argc, char* argv[]). I've tried with cmd.exe (with chcp 65001), PowerShell and git bash - all fail to pass the argument correctly.

If we want to fix that too then may need to change to using wmain(int argc, wchar_t* argv[]) on Windows - but that has bigger impact as wchar_t will need to be passed everywhere. Maybe just document that if you want to use non-ASCII characters then pass the arguments in a file.

[tonydnewell]

As I said above to handle non-ascii arguments on the command line (for Windows) then wmain needs to be used. The code would need to be something like below. I've not included that in the fix as it is probably outside the scope of the issue being fixed:

In main.cc:

#ifndef _WIN32
int main(int argc, char* argv[]) {
  return PROTOBUF_NAMESPACE_ID::compiler::ProtobufMain(argc, argv);
}
#else
int wmain( int argc, wchar_t *wargv[ ]) {

  // convert all arguments to utf-8
  char **argv = (char **)malloc(sizeof(char *) * argc);
  for (int i = 0; i < argc; ++i) {
    std::string utf8str;
    if (!google::protobuf::io::win32::strings::wcs_to_utf8(wargv[i], &utf8str)) {
      //GOOGLE_LOG(FATAL) << "wcs_to_utf8: " << Win32ErrorMessage(GetLastError());
    }
    argv[i] = _strdup(utf8str.c_str());
  }

  // call ProtobufMain with the utf-8 arguments
  int sts = PROTOBUF_NAMESPACE_ID::compiler::ProtobufMain(argc, argv);

  // free memory allocated for arguments
  for (int i = 0; i < argc; ++i)
    free(argv[i]);
  free(argv);
  return sts;
}
#endif

[jtattermusch]

Thanks for the explanation, the fix makes sense to me. Looks like after this fix, all the command line arguments would work correctly with accents when passed via an argument file (e.g. @test.rsp) - most of them did already work correctly, but the plugin path didn't.

What's still a bit puzzling to me is why the command line argument don't seem to work with accents when regular command line arguments (not argument files) are used.
I found that that's something that should have been fixed long time ago by an old PR #4013 (which introduced most of the logic for converting args including as_winows_path).
As you said, fixing the argument processing for regular command line arguments isn't the goal here and should probably be fixed separately (if it indeed doesn't work), but it would still be good to understand what's wrong with it (it looks like it must have worked at some point, so perhaps there was a regression?).

Perhaps relevant:

• io_win32 does not handle non-ascii chars in paths. #3951 (comment) (the snippet can be used to determine what is actually being passed to main when the single-byte version of main is being used).

[fowles]

There is another call to portable_strdup in SubProccess::Start. Does that need to be updated as well?

[tonydnewell]

I don't think so. It is used in the non-Windows code and this is a Windows specific fix.

[jtattermusch]

Looks like this change didn't make it to protobuf's 21.x release. I'd therefore expect it to become available as part of 22.x release (AFAIKT that release cycle hasn't started yet).

[fowles]

If you would like to include this in the next 21.x repease, please send a PR that cherry picks this back

[tonydnewell]

@fowles

If you would like to include this in the next 21.x repease, please send a PR that cherry picks this back

PR #10721