Add a unit test for CommandLine that makes sure that GetProgram will not retu...

Add a unit test for CommandLine that makes sure that GetProgram will not return a quoted string and that (on Windows) the program part of a command line string will always be quoted.

BUG=none
TEST=Run the ProgramQuotes test.  Should be no visible changes.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=67583

[Evan Martin, 2010-11-15 21:01:08]

http://codereview.chromium.org/4949004/diff/1/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/4949004/diff/1/base/command_line.cc#newcode128
base/command_line.cc:128: program_ = TrimQuotes(program.value());
I think this is wrong.  A FilePath is a platonic ideal of a path, where there is
no conception of quoting.  If program.value() contains quotes, that is
representing that the underlying file has a quote character in it.

What are you trying to do?

[tommi (sloooow) - chröme, 2010-11-15 21:16:23]

The intent is to make sure the CommandLine class always quotes the program
part of the command line.
The installer is now using the class for its command lines and if the
CommandLine class were to incorrectly quote, or not quote the program, MSI
will get confused and uninstallation/installation will break.

Currently, the FilePath class doesn't do any checking for quotes and there
are no checks in CommandLine either.

At least on Windows, file and directory names cannot have a quotes, so I
could make the change Windows only if you prefer?

On Mon, Nov 15, 2010 at 4:01 PM, <evan@chromium.org> wrote:

>
> http://codereview.chromium.org/4949004/diff/1/base/command_line.cc
> File base/command_line.cc (right):
>
>
> http://codereview.chromium.org/4949004/diff/1/base/command_line.cc#newcode128
> base/command_line.cc:128: program_ = TrimQuotes(program.value());
> I think this is wrong.  A FilePath is a platonic ideal of a path, where
> there is no conception of quoting.  If program.value() contains quotes,
> that is representing that the underlying file has a quote character in
> it.
>
> What are you trying to do?
>
>
> http://codereview.chromium.org/4949004/
>

[Evan Martin, 2010-11-15 21:46:36]

+FilePath master to back up my rationale

If you're introducing this as a failsafe check, then yes, I think it should be
both Windows-specific and accompanied by DCHECKs.

Something like, on accepting the path:
if (path.value().find('"') != string::npos) {
  NOTREACHED();
  // Attempt to recover, in case we got quoting wrong somewhere else.
  StripQuotes(&path);
}

[Mark Mentovai, 2010-11-15 22:01:22]

I agree with Evan. The entirety of this change is Windows-specific. Please #if
it accordingly.

http://codereview.chromium.org/4949004/diff/6001/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/4949004/diff/6001/base/command_line.cc#newcode51
base/command_line.cc:51: // Trims the quotes from the beginning and end of a
path.
This should be Windows-specific. Put the function itself inside an #if to avoid
tempting people to use it on non-Windows. Having a program interpret quoting
like this at the command-line level is ludicrous on POSIX.

[Mark Mentovai, 2010-11-15 22:03:11]

I agree with Evan. The entirety of this change is Windows-specific. Please #if
it accordingly.

[tommi (sloooow) - chröme, 2010-11-15 22:12:18]

Thanks Mark and Evan.

I changed it so that only Windows should be affected.

[Evan Martin, 2010-11-15 22:21:57]

http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc#newcod...
base/command_line.cc:124: program_ = TrimQuotes(program.value());
To be clear, if there are quotes here something has gone horribly, wrong, right?

Can you have it test for quotes and hit a NOTREACHED() in that case?

I guess you can also remove that TODO then.
(But what if program.value() ends with a backslash?  I don't really understand
your threat model here...)

[tommi (sloooow) - chröme, 2010-11-15 22:41:23]

http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc
File base/command_line.cc (right):

http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc#newcod...
base/command_line.cc:124: program_ = TrimQuotes(program.value());
On 2010/11/15 22:21:57, Evan Martin wrote:
> To be clear, if there are quotes here something has gone horribly, wrong,
right?

I think so, yes.  However I cleaned up quite a bit of code last week that was
using it's own command line parsing with wstring but maybe there still is code
out there that uses its own command line parsing and somehow could give
CommandLine a quoted FilePath :-/  Unlikely though.

> 
> Can you have it test for quotes and hit a NOTREACHED() in that case?

Yes.

> 
> I guess you can also remove that TODO then.
> (But what if program.value() ends with a backslash?  I don't really understand
> your threat model here...)

I don't have a threat model, but that's an interesting point.  In that case the
file path could only represent a directory...

Do you want me to change it to:

program_ = WindowsStyleQuote(program, true);

where:

std::wstring WindowsStyleQuote(const std::wstring& arg, bool always_quote) {
  if (!always_quote && arg.find_first_of(L" \\\"") == std::wstring::npos) {
    // No quoting necessary.
    return arg;
  }

  // .. rest same as it currently is
}

[tommi (sloooow) - chröme, 2010-11-15 22:49:25]

(+robertshield for fyi)

I'm also fine with reverting the change altogether or maybe just add a
single DCHECK that the file path never starts/ends with a quote on windows.
 I think that's sufficient for our paranoia (robert?).

On Mon, Nov 15, 2010 at 5:41 PM, <tommi@chromium.org> wrote:

>
> http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc
> File base/command_line.cc (right):
>
>
>
http://codereview.chromium.org/4949004/diff/13001/base/command_line.cc#newcod...
> base/command_line.cc:124: program_ = TrimQuotes(program.value());
> On 2010/11/15 22:21:57, Evan Martin wrote:
>
>> To be clear, if there are quotes here something has gone horribly,
>>
> wrong, right?
>
> I think so, yes.  However I cleaned up quite a bit of code last week
> that was using it's own command line parsing with wstring but maybe
> there still is code out there that uses its own command line parsing and
> somehow could give CommandLine a quoted FilePath :-/  Unlikely though.
>
>
>
>  Can you have it test for quotes and hit a NOTREACHED() in that case?
>>
>
> Yes.
>
>
>
>  I guess you can also remove that TODO then.
>> (But what if program.value() ends with a backslash?  I don't really
>>
> understand
>
>> your threat model here...)
>>
>
> I don't have a threat model, but that's an interesting point.  In that
> case the file path could only represent a directory...
>
> Do you want me to change it to:
>
> program_ = WindowsStyleQuote(program, true);
>
> where:
>
> std::wstring WindowsStyleQuote(const std::wstring& arg, bool
> always_quote) {
>  if (!always_quote && arg.find_first_of(L" \\\"") ==
> std::wstring::npos) {
>    // No quoting necessary.
>    return arg;
>  }
>
>  // .. rest same as it currently is
>
> }
>
> http://codereview.chromium.org/4949004/
>

[Evan Martin, 2010-11-15 22:50:12]

On 2010/11/15 22:41:23, tommi wrote:
> > I guess you can also remove that TODO then.
> > (But what if program.value() ends with a backslash?  I don't really
understand
> > your threat model here...)
> 
> I don't have a threat model, but that's an interesting point.  In that case
the
> file path could only represent a directory...
> 
> Do you want me to change it to:
> 
> program_ = WindowsStyleQuote(program, true);
> 
> where:
> 
> std::wstring WindowsStyleQuote(const std::wstring& arg, bool always_quote) {
>   if (!always_quote && arg.find_first_of(L" \\\"") == std::wstring::npos) {
>     // No quoting necessary.
>     return arg;
>   }
> 
>   // .. rest same as it currently is
> }

That seems ok to me.  You could also do something simpler if you don't expect it
to come up.  Something like
while (input.endswith('\\')) {
  NOTREACHED();
  input.erase_last_char();
}
would also be a plausible recovery path

[Evan Martin, 2010-11-15 22:52:32]

Another idea:
- check for quotes or trailing backslash
- if so, NOTREACHED
- and then let it through verbatim or wrap more quotes around it, in the theory
that the old behavior of Chrome to always add was ok.

I think the scary part of this is that there might be some old registry setting
that relied upon the old behavior, so with more thought I think it's maybe
valuable to try to not change the behavior of the code on "invalid" inputs.

[tommi (sloooow) - chröme, 2010-11-29 20:36:21]

Hey Evan,

I got distracted by a few other things in the last couple of weeks but still
have this pending.  Going with your suggestion that the old behavior was always
OK, I have reverted all changes in command_line.cc and only have a small,
Windows only, unit test now.

On 2010/11/15 22:52:32, Evan Martin wrote:
> Another idea:
> - check for quotes or trailing backslash
> - if so, NOTREACHED
> - and then let it through verbatim or wrap more quotes around it, in the
theory
> that the old behavior of Chrome to always add was ok.
> 
> I think the scary part of this is that there might be some old registry
setting
> that relied upon the old behavior, so with more thought I think it's maybe
> valuable to try to not change the behavior of the code on "invalid" inputs.

[Evan Martin, 2010-11-29 20:39:55]

LGTM

http://codereview.chromium.org/4949004/diff/23001/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4949004/diff/23001/base/command_line_unittest....
base/command_line_unittest.cc:187: const FilePath
kProgram(FILE_PATH_LITERAL("Program"));
Since you're in a Windows-specific branch, L"Program" is also acceptable and
perhaps more readable.

[tommi (sloooow) - chröme, 2010-11-29 20:45:01]

Thanks.

http://codereview.chromium.org/4949004/diff/23001/base/command_line_unittest.cc
File base/command_line_unittest.cc (right):

http://codereview.chromium.org/4949004/diff/23001/base/command_line_unittest....
base/command_line_unittest.cc:187: const FilePath
kProgram(FILE_PATH_LITERAL("Program"));
On 2010/11/29 20:39:55, Evan Martin wrote:
> Since you're in a Windows-specific branch, L"Program" is also acceptable and
> perhaps more readable. 

Done.