[PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

In D56230#1354851 <https://reviews.llvm.org/D56230#1354851>, @zturner wrote:

> I almost feel like the `Args` class could be made smarter here.  Because all of the proposed solutions will still not work correctly on Linux.  For example, why is this Windows specific?   `Args::GetCommandString()` is very dumb and just loops over each one appending to a string.  So if your log file name contains spaces on Linux, you would still have a problem no?


There won't be any problem with space (or any other character for that matter) on linux, because we don't use `GetCommandString` on linux. We don't use it for two reasons:
a) we don't need to -- we just pass the individual strings in the Args array as an `argv` vector into `execve`
b) `GetCommandString` is fundamentally broken. It uses random quoting rules which aren't even deterministically reversible. It will probably be right in most situations when the string contains spaces, but things will blow up as soon as the string starts to contain quote chars and backslashes.

> Currently the signature of `AppendArgument()` takes an argument string, and a quote char, which defaults to `'\0'` which means no quote.  But there is only one call-site of that entire function out of many hundreds of other calls to the function, most of which just pass nothing.  This means that all of those other call-sites are broken if their argument contains a space (many are string literals though, so it won't be a problem there).
> 
> But why don't we just split this into two functions:
> 
>   Args::QuoteAndAppendArgument(const Twine &arg);
>   Args::AppendArgument(const Twine &arg);
> 
> 
> and the first one will auto-detect the quoting style to use based on the host operating system.  This way we would fix the problem for all platforms, not just Windows, and we could also use the function to fix potentially many other bugs at all the callsites where we append unquoted arguments.

I fundamentally disagree with this approach. There's a reason why most call sites do not use the quote_char, and that's because it's impossible to use correctly. The quoting character, and quoting rules are in general not known at the place where we build the Args vector. Specifically, they are not a property of the host system. They're a property of the entity you are about to pass the quoted string to. So you need to quote the string differently depending on whether you are going to pass it to:

- a posix-style syscall like execve
- windows CreateProcess
- the lldb builtin interpreter
- windows cmd.exe
- a posix shell (there might be even variations between the shells here)

There's no way all of this can be encompassed in a single char variable. Using the "host" rules would be the right solution here, but it would be wrong pretty much everywhere else, because you never know if those Args are going to end up being transmitted over the network and executed on a different system with different rules.

That's why I think the only reasonable solution here is to ignore the quote char here like everyone else is doing already (we should eventually remove it). Instead we should have separate functions like `QuoteForCreateProcess`, `QuoteForLLDBInterpreter`, ... (I don't care whether they live in the Args class or not), and have each consumer responsible for picking the right one. So, `ProcessLauncherWindows` would call `QuoteForCreateProcess`, which would apply any transformations necessary, pass the result to `CreateProcess` and be done.

For example, for a `Args` vector like `lldb-server`, `gdb-remote`, `--log-channels=foo\\\   \\\"""   '''`, `whatever`, `QuoteForCreateProcess` would return
`lldb-server gdb-remote "--log-channels=foo\\\   \\\\\\\"\"\"   '''" whatever` (there are other ways to quote this too). Passing this string to CreateProcess will result in the original vector being available to the `main` function of lldb-server, which is what this code (and all other code that works with the `Args` class) expects.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56230/new/

https://reviews.llvm.org/D56230