Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543503003:
Handle spaces and quotes in chrome_elf command line parser (Closed)
|
Created:
4 years ago by scottmg Modified:
4 years ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews, ananta Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle spaces and quotes in chrome_elf command line parser
This is the worst sort of procedural stateful goopy code. I followed the
pseudo-code from
http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line-into-individual-arguments/
and verified against calling CommandLineToArgvW() which is what we're
attempting to emulate here.
Two tests were removed from
InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't
believe they were correct (spaces after the equal sign), but maybe
Ananta can correct me on that.
R=grt@chromium.org
BUG=670012
Committed: https://crrev.com/e8882778ceb608911d96e7e485dbbff4db5c2b83
Cr-Commit-Position: refs/heads/master@{#436056}
Patch Set 1 #
Total comments: 4
Patch Set 2 : unsigned #Patch Set 3 : use wcs* #
Total comments: 30
Patch Set 4 : fixes, test cases from blogs, special argv[0] handling #Patch Set 5 : refactor argv0 a bit #
Total comments: 8
Patch Set 6 : . #
Messages
Total messages: 41 (28 generated)
Description was changed from ========== Handle spaces and quotes in chrome_elf command line parser R=grt@chromium.org BUG=670012 ========== to ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW(). Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign). R=grt@chromium.org BUG=670012 ==========
Description was changed from ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW(). Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign). R=grt@chromium.org BUG=670012 ========== to ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW(). Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign). R=grt@chromium.org BUG=670012 ==========
Description was changed from ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW(). Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign). R=grt@chromium.org BUG=670012 ========== to ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW() which is what we're attempting to emulate here. Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign), but maybe Ananta can correct me on that. R=grt@chromium.org BUG=670012 ==========
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm not sure this is perfect, but it's ... better.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Since you're writing C... (feel free to ignore!) https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... chrome/install_static/install_util.cc:615: while (p[0] == L' ' || p[0] == L'\t') wcsspn()? https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... chrome/install_static/install_util.cc:653: if (p[0] == 0 || ((p[0] == L' ' || p[0] == '\t') && wstrstr(L" \t", p[0])
thakis@chromium.org changed reviewers: + thakis@chromium.org
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... chrome/install_static/install_util.cc:615: while (p[0] == L' ' || p[0] == L'\t') On 2016/12/01 05:01:09, Nico wrote: > wcsspn()? Done. https://codereview.chromium.org/2543503003/diff/1/chrome/install_static/insta... chrome/install_static/install_util.cc:653: if (p[0] == 0 || ((p[0] == L' ' || p[0] == '\t') && On 2016/12/01 05:01:09, Nico wrote: > wstrstr(L" \t", p[0]) You mean wcschr right? Not sure if that's clearer or not, but sure.
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
such fun! https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:603: enum class SpecialChars { i think some comments in here are worthwhile. i've made a bunch of suggestions which you're free to take, modify, correct, or toss in favor of comments you think work better. // Indicates whether or not space and tab are interpreted as token separators. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:604: kInterpret, // Space or tab, if encountered, delimit a break between tokens. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:605: kIgnore, // Space or tab, if encountered, are part of the current token. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:628: // Count and advance past leading backslashes. nit: since this counts all backslashes, not only leading ones, i propose: // Count and advance past collections of backslashes, which have special meaning when followed by a double quote. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:636: for (int i = 1; i < num_backslashes; i += 2) tiny nit: a single append will be more performant that multiple: // Emit a backslash for each but the last pair found. if (num_backslashes) token.append((num_backslashes - 1) / 2, L'\\'); https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:640: token += L'"'; // An odd number of backslashes followed by a quotation mark is treated as pairs of protected backslashes, followed by a protected quotation mark. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:641: } else if (p[1] == L'"' && state == SpecialChars::kIgnore) { will this not be an OOB read for a string that ends with a double quote? https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:642: ++p; // Special case for consecutive double quotes while ignoring special chars: emit one for the pair and switch back to interpreting special chars. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:650: for (int i = 0; i < num_backslashes; ++i) // Emit backslashes that do not precede a double quote verbatim: token.append(num_backslashes, L'\\'); https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:653: (wcschr(L" \t", p[0]) && state == SpecialChars::kInterpret)) { nit: swap these two so that the cheap condition is evaluated first https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:657: } else { nit: omit "else" since the "if" branch breaks for consistency with the "no else after return" rule. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:676: for (size_t i = 1; i < as_array.size(); ++i) { nit for (const std::wstring& arg : as_array) { https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:678: if (arg.compare(0, switch_with_equal.size(), switch_with_equal) == 0) { nit: omit braces https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util_unittest.cc:95: TEST(InstallStaticTest, SpacesAndQuotesInCommandLineArguments) { would you be so kind as to add an expectation for each of the examples in the blog posts you cite?
Thanks Greg. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:603: enum class SpecialChars { On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > i think some comments in here are worthwhile. i've made a bunch of suggestions > which you're free to take, modify, correct, or toss in favor of comments you > think work better. > > // Indicates whether or not space and tab are interpreted as token separators. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:604: kInterpret, On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > // Space or tab, if encountered, delimit a break between tokens. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:605: kIgnore, On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > // Space or tab, if encountered, are part of the current token. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:628: // Count and advance past leading backslashes. On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > nit: since this counts all backslashes, not only leading ones, i propose: > // Count and advance past collections of backslashes, which have special meaning > when followed by a double quote. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:636: for (int i = 1; i < num_backslashes; i += 2) On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > tiny nit: a single append will be more performant that multiple: > // Emit a backslash for each but the last pair found. > if (num_backslashes) > token.append((num_backslashes - 1) / 2, L'\\'); Done. (num_backslashes / 2) though. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:640: token += L'"'; On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > // An odd number of backslashes followed by a quotation mark is treated as pairs > of protected backslashes, followed by a protected quotation mark. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:641: } else if (p[1] == L'"' && state == SpecialChars::kIgnore) { On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > will this not be an OOB read for a string that ends with a double quote? I think it's safe, because .c_str() is null terminated. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:642: ++p; On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > // Special case for consecutive double quotes while ignoring special chars: emit > one for the pair and switch back to interpreting special chars. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:650: for (int i = 0; i < num_backslashes; ++i) On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > // Emit backslashes that do not precede a double quote verbatim: > token.append(num_backslashes, L'\\'); Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:653: (wcschr(L" \t", p[0]) && state == SpecialChars::kInterpret)) { On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > nit: swap these two so that the cheap condition is evaluated first Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:657: } else { On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > nit: omit "else" since the "if" branch breaks for consistency with the "no else > after return" rule. Done. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:676: for (size_t i = 1; i < as_array.size(); ++i) { On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > nit > for (const std::wstring& arg : as_array) { I'd prefer that, but is there an easy way to do that from 1..size() rather than 0? I wish C++'s syntax was a bit more like Python's (slicing, enumerate(), range(), etc.) https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:678: if (arg.compare(0, switch_with_equal.size(), switch_with_equal) == 0) { On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > nit: omit braces Heh, done. I forgot to switch from mark@ back to grt@ review "local customs" (Mark always prefers those added. ;) https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... File chrome/install_static/install_util_unittest.cc (right): https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util_unittest.cc:95: TEST(InstallStaticTest, SpacesAndQuotesInCommandLineArguments) { On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > would you be so kind as to add an expectation for each of the examples in the > blog posts you cite? Done. The windowsinspired.com ones were primarily about the bizzaro special handling of argv[0], but since we're in here digging ourselves quite a lovely hole, I figured I might as well dig the whole hole while I'm at it. So I implemented that too. (The CommandLineToArgvW version, not the __argc/__argv CRT version... because why wouldn't they be different?!).
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by scottmg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@chromium.org changed reviewers: - thakis@chromium.org
I'll shut up now. I tried to uncheck "add as reviewer" when I left my first comment. trying to remove myself again – found another nit though, sorry :-P https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:656: int num_backslashes = 0; nit: int num_backslashes = wstrspn(p, L"\\"); p += num_backslashes;
lgtm w/ nits https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:636: for (int i = 1; i < num_backslashes; i += 2) On 2016/12/01 19:04:22, scottmg wrote: > On 2016/12/01 09:07:13, grt (UTC plus 1) wrote: > > tiny nit: a single append will be more performant that multiple: > > // Emit a backslash for each but the last pair found. > > if (num_backslashes) > > token.append((num_backslashes - 1) / 2, L'\\'); > > Done. (num_backslashes / 2) though. Oops. Cool. https://codereview.chromium.org/2543503003/diff/40001/chrome/install_static/i... chrome/install_static/install_util.cc:676: for (size_t i = 1; i < as_array.size(); ++i) { On 2016/12/01 19:04:22, scottmg wrote: > On 2016/12/01 09:07:12, grt (UTC plus 1) wrote: > > nit > > for (const std::wstring& arg : as_array) { > > I'd prefer that, but is there an easy way to do that from 1..size() rather than > 0? I wish C++'s syntax was a bit more like Python's (slicing, enumerate(), > range(), etc.) oh, 1, duh. apologies https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:612: // The first argument (the program) is handled differently than other is the the same? // The first argument (the program) is delimited by whitespace or quotes based // on its first character. int argv0_length = 0; if (p[0] == L'"') argv0_length = wcschr(++p, L'"'); else argv0_length = wcscspn(p, kSpaceTab); result.emplace_back(p, argv0_length); p += argv0_length + 1; https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:628: (state == SpecialChars::kInterpret && wcschr(L" \t", p[0]))) { since L" \t" appears a bunch, how about static constexpr wchar_t kSpaceTab[] = L" \t"; up above? https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:672: // Special case for consecutive double quotes in a string: emit one nit: it's for consecutive double quotes within a quoted string, right?
Thanks! https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... File chrome/install_static/install_util.cc (right): https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:612: // The first argument (the program) is handled differently than other On 2016/12/02 10:18:03, grt (UTC plus 1) wrote: > is the the same? > > // The first argument (the program) is delimited by whitespace or quotes based > // on its first character. > int argv0_length = 0; > if (p[0] == L'"') > argv0_length = wcschr(++p, L'"'); > else > argv0_length = wcscspn(p, kSpaceTab); > result.emplace_back(p, argv0_length); > p += argv0_length + 1; Almost! With a couple tweaks passes all the tests at least, so I'll call it close enough for our purposes. https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:628: (state == SpecialChars::kInterpret && wcschr(L" \t", p[0]))) { On 2016/12/02 10:18:03, grt (UTC plus 1) wrote: > since L" \t" appears a bunch, how about > static constexpr wchar_t kSpaceTab[] = L" \t"; > up above? Done. https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:656: int num_backslashes = 0; On 2016/12/02 01:02:33, Nico wrote: > nit: > > int num_backslashes = wstrspn(p, L"\\"); > p += num_backslashes; Thanks, done. I guess I just don't know C very well so I don't think of using the lettersoup string functions instead of a short loop when it's short. https://codereview.chromium.org/2543503003/diff/100001/chrome/install_static/... chrome/install_static/install_util.cc:672: // Special case for consecutive double quotes in a string: emit one On 2016/12/02 10:18:03, grt (UTC plus 1) wrote: > nit: it's for consecutive double quotes within a quoted string, right? Yup, done. I mean, isn't it totally obvious, and clear as mud? :)
The CQ bit was checked by scottmg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2543503003/#ps120001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480713960676030,
"parent_rev": "53c964a4a87d2af1505130311969ec6e476af4b5", "commit_rev":
"da099e1f3ee2757767ce244df70ee11ae1b4a9e8"}
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW() which is what we're attempting to emulate here. Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign), but maybe Ananta can correct me on that. R=grt@chromium.org BUG=670012 ========== to ========== Handle spaces and quotes in chrome_elf command line parser This is the worst sort of procedural stateful goopy code. I followed the pseudo-code from http://www.windowsinspired.com/how-a-windows-programs-splits-its-command-line... and verified against calling CommandLineToArgvW() which is what we're attempting to emulate here. Two tests were removed from InstallStaticTest.GetSwitchValueFromCommandLineTest because I don't believe they were correct (spaces after the equal sign), but maybe Ananta can correct me on that. R=grt@chromium.org BUG=670012 Committed: https://crrev.com/e8882778ceb608911d96e7e485dbbff4db5c2b83 Cr-Commit-Position: refs/heads/master@{#436056} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/e8882778ceb608911d96e7e485dbbff4db5c2b83 Cr-Commit-Position: refs/heads/master@{#436056} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
