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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56230#1354829 , @Hui wrote:

> I think the key problem here is to make sure the argument will be treated as 
> a single argument to the process launcher.


Can you elaborate on that? I still don't see what is the problem with the 
solution I proposed.

> To be specific to this case only, could we just provide a quote char to 
> argument log file path and log channels on Windows?
> 
> The downside is one more #if is introduced.

Yes, but this is not a problem specific to this code. Plenty of code appends 
strings to the Args class which may contain spaces all of those would be wrong 
if they make it's way to the windows process launcher, so it's logical to fix 
the problem there, as it all affected code paths have to go through there and 
it alone has enough information to make the right decision on how to quote 
things.


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

https://reviews.llvm.org/D56230



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In 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



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D56230#1355247 , @labath wrote:

> 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.


Btw, there is already code for doing this in llvm 
(`llvm::sys::flattenWindowsCommandLine`), so we can just steal the 
implementation from there.


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

https://reviews.llvm.org/D56230



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits