base::CommandLine: Added optional quoting of placeholder arguments.

base::CommandLine: Added optional quoting of placeholder arguments.

Added an optional parameter |quote_placeholder| to GetCommandLineString
and GetArgumentsString. If true, placeholder command-line arguments such
as "%1" will also be quoted. These need quoting in case the substituted
argument has a space in it. It was previously not possible to build a
CommandLine that would contain a quoted "%1" when converted to a string.

Only affects Windows.

BUG=416785
Committed: https://crrev.com/c974d5100e52cdbc33c0d687f80498f34b6f3e35
Cr-Commit-Position: refs/heads/master@{#297617}

[Matt Giuca, 2014-09-23 08:59:33]

mgiuca@chromium.org changed reviewers:
+ mark@chromium.org, msw@chromium.org

[Matt Giuca, 2014-09-23 08:59:34]

msw: Author of code being changed.
mark: OWNERS.

Not 100% convinced this is a good idea (since it affects all command line
strings), but I can't think of anything that would break if it had extra quotes
around it. Do you think there is a risk?

Without this, I will have to continue using strings to represent command lines
with placeholders in the registry code (which I am currently reworking). Which
would be a shame given this lovely CommandLine abstraction.

[Mark Mentovai, 2014-09-23 12:34:26]

This a pretty error-prone area. What if the replacement string has a " character
in it? Who’s responsible for dealing with that, or with backslashes?

I think we need to know more about how callers are doing replacements on command
line strings that are prepared by this function. Most likely, a better answer
would be to leave % alone in this function, and do replacements on a CommandLine
while it’s still in vector form, before QuoteForCommandLineToArgvW() is even
called.

[Matt Giuca, 2014-09-24 08:23:47]

The problem is that *we* (Chrome) aren't doing the replacing. I'm making a
string like:
"C:\Blah\chrome.exe" -- "%1"
(quotes are literally in the string)

and storing that string as a value in the Windows registry. Then Windows comes
along and substitutes a path in for %1.

So it can't be subbed while it's still in Command Line form. There is physically
no way I can find to make a CommandLine that will emit the above string. If I
put "%1" as an argument, it will emit it without quotes. If I put "\"%1\"" as an
argument, it will emit it double-quoted.

> What if the replacement string has a " character in it? Who’s responsible for
dealing with that, or with backslashes?

It only gets replaced with a filename, and filenames on Windows can't have
quotes or slashes in them. (Note that this only affects Windows; I should put
that in the CL description.)

[Mark Mentovai, 2014-09-24 12:36:49]

I’m not really comfortable with this, then. The quoting you’re doing makes sense
for stuff that gets dumped into the registry, but it’s dangerous as a
general-purpose replacement to arbitrary command line strings.

The only way I think this would be OK would be if you got these strings to put
into the registry from a different (new) method on CommandLine whose purpose was
to produce strings for the registry.

> It only gets replaced with a filename, and filenames on Windows can't have
> quotes or slashes in them.

Really? This would never see a full path with backslashes?

[Matt Giuca, 2014-09-25 00:19:40]

On 2014/09/24 12:36:49, Mark Mentovai wrote:
> I’m not really comfortable with this, then. The quoting you’re doing makes
sense
> for stuff that gets dumped into the registry, but it’s dangerous as a
> general-purpose replacement to arbitrary command line strings.
> 
> The only way I think this would be OK would be if you got these strings to put
> into the registry from a different (new) method on CommandLine whose purpose
was
> to produce strings for the registry.

OK, I'm happy to add a new method to CommandLine which does this (and refactor
the existing code to avoid duplication). If you think that's acceptable.

> > It only gets replaced with a filename, and filenames on Windows can't have
> > quotes or slashes in them.
> 
> Really? This would never see a full path with backslashes?

Oh, OK yeah I forgot about that. :) OK but your concern with slash is that it
may be followed by a quote, right? I don't think Windows will ever substitute a
string here that *ends* with a slash (because it has to end with a filename).
Either way, there's no use arguing about it, because Windows does the
substitution, not us. "%1" is the standard way to write a file handler command,
and if there's something wrong with it then it affects all associations in
Windows.

[Matt Giuca, 2014-09-25 01:19:25]

OK I did the change. Now all methods will continue behaving exactly as they were
in the default case, but there is now an optional Boolean to get the placeholder
quoting behaviour.

[Mark Mentovai, 2014-09-25 02:34:41]

I'm sure there are some cases where a command has a directory path
parameter. It's not unusual to wind up with a trailing backslash on a
directory name. Then you'd wind up with a backslash followed by a quote.
On Sep 24, 2014 8:19 PM, <mgiuca@chromium.org> wrote:

> On 2014/09/24 12:36:49, Mark Mentovai wrote:
>
>> I'm not really comfortable with this, then. The quoting you're doing makes
>>
> sense
>
>> for stuff that gets dumped into the registry, but it's dangerous as a
>> general-purpose replacement to arbitrary command line strings.
>>
>
>  The only way I think this would be OK would be if you got these strings
>> to put
>> into the registry from a different (new) method on CommandLine whose
>> purpose
>>
> was
>
>> to produce strings for the registry.
>>
>
> OK, I'm happy to add a new method to CommandLine which does this (and
> refactor
> the existing code to avoid duplication). If you think that's acceptable.
>
>  > It only gets replaced with a filename, and filenames on Windows can't
>> have
>> > quotes or slashes in them.
>>
>
>  Really? This would never see a full path with backslashes?
>>
>
> Oh, OK yeah I forgot about that. :) OK but your concern with slash is that
> it
> may be followed by a quote, right? I don't think Windows will ever
> substitute a
> string here that *ends* with a slash (because it has to end with a
> filename).
> Either way, there's no use arguing about it, because Windows does the
> substitution, not us. "%1" is the standard way to write a file handler
> command,
> and if there's something wrong with it then it affects all associations in
> Windows.
>
> https://codereview.chromium.org/595803002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Mentovai, 2014-09-25 02:36:58]

I don't think we should do this with a boolean, it should be it's own
distinctively-named method. It's too easy to misunderstand a boolean
argument. Let's add a new method with a nice descriptive name.
On Sep 24, 2014 10:34 PM, "Mark Mentovai" <mark@chromium.org> wrote:

> I'm sure there are some cases where a command has a directory path
> parameter. It's not unusual to wind up with a trailing backslash on a
> directory name. Then you'd wind up with a backslash followed by a quote.
> On Sep 24, 2014 8:19 PM, <mgiuca@chromium.org> wrote:
>
>> On 2014/09/24 12:36:49, Mark Mentovai wrote:
>>
>>> I'm not really comfortable with this, then. The quoting you're doing
>>> makes
>>>
>> sense
>>
>>> for stuff that gets dumped into the registry, but it's dangerous as a
>>> general-purpose replacement to arbitrary command line strings.
>>>
>>
>>  The only way I think this would be OK would be if you got these strings
>>> to put
>>> into the registry from a different (new) method on CommandLine whose
>>> purpose
>>>
>> was
>>
>>> to produce strings for the registry.
>>>
>>
>> OK, I'm happy to add a new method to CommandLine which does this (and
>> refactor
>> the existing code to avoid duplication). If you think that's acceptable.
>>
>>  > It only gets replaced with a filename, and filenames on Windows can't
>>> have
>>> > quotes or slashes in them.
>>>
>>
>>  Really? This would never see a full path with backslashes?
>>>
>>
>> Oh, OK yeah I forgot about that. :) OK but your concern with slash is
>> that it
>> may be followed by a quote, right? I don't think Windows will ever
>> substitute a
>> string here that *ends* with a slash (because it has to end with a
>> filename).
>> Either way, there's no use arguing about it, because Windows does the
>> substitution, not us. "%1" is the standard way to write a file handler
>> command,
>> and if there's something wrong with it then it affects all associations in
>> Windows.
>>
>> https://codereview.chromium.org/595803002/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Mentovai, 2014-09-25 03:40:44]

Trying to give you fast turnaround because I know our workdays don’t really
coincide very well.

https://codereview.chromium.org/595803002/diff/20001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/595803002/diff/20001/base/command_line.h#newc...
base/command_line.h:113: StringType GetArgumentsString(bool quote_placeholders)
const;
Overloading’s discouraged.

Let’s just give this one a different name as I suggested. I think it’s OK to
make it Windows-only.

[Matt Giuca, 2014-09-25 04:34:09]

Patchset #3 (id:40001) has been deleted

[Matt Giuca, 2014-09-25 04:35:30]

OK fair enough. (Although I dislike having to move two medium-sized methods and
be the owner for them.)

https://codereview.chromium.org/595803002/diff/20001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/595803002/diff/20001/base/command_line.h#newc...
base/command_line.h:113: StringType GetArgumentsString(bool quote_placeholders)
const;
On 2014/09/25 03:40:43, Mark Mentovai wrote:
> Overloading’s discouraged.
> 
> Let’s just give this one a different name as I suggested. I think it’s OK to
> make it Windows-only.

Done.

[Mark Mentovai, 2014-09-25 13:14:16]

LGTM otherwise

https://codereview.chromium.org/595803002/diff/60001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/595803002/diff/60001/base/command_line.h#newc...
base/command_line.h:107: // argument with a '%' in it.
The comment here and on the other method should say that this is really intended
for use in the registry, and because of the limitations on who does the
expansion and on how it’s not possible to reliably quote arbitrary data, it
should be avoided in any other case except where an established interface that
we don’t control dictates its use.

[Matt Giuca, 2014-09-26 03:41:40]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-26 03:42:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595803002/80001

[Matt Giuca, 2014-09-26 03:49:05]

https://codereview.chromium.org/595803002/diff/60001/base/command_line.h
File base/command_line.h (right):

https://codereview.chromium.org/595803002/diff/60001/base/command_line.h#newc...
base/command_line.h:107: // argument with a '%' in it.
On 2014/09/25 13:14:15, Mark Mentovai wrote:
> The comment here and on the other method should say that this is really
intended
> for use in the registry, and because of the limitations on who does the
> expansion and on how it’s not possible to reliably quote arbitrary data, it
> should be avoided in any other case except where an established interface that
> we don’t control dictates its use.

Done.

[commit-bot: I haz the power, 2014-09-26 03:50:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-26 03:50:42]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Matt Giuca, 2014-09-30 00:56:35]

Ugh, presubmit warnings preventing submit.

Path of least resistance is just to fix them:
https://codereview.chromium.org/602253006/

[Matt Giuca, 2014-10-01 08:22:14]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-10-01 08:22:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/595803002/100001

[commit-bot: I haz the power, 2014-10-01 09:24:58]

Committed patchset #5 (id:100001) as 3c3ea9125d277c8bfff66e581b4291749339583e

[commit-bot: I haz the power, 2014-10-01 09:25:32]

Patchset 5 (id:??) landed as
https://crrev.com/c974d5100e52cdbc33c0d687f80498f34b6f3e35
Cr-Commit-Position: refs/heads/master@{#297617}