Message ID | 1397936424-2290-2-git-send-email-mingw.android@gmail.com |
---|---|
State | New |
Headers | show |
Hello Ray, Patches to libiberty need to be cross-posted to binutils, gdb, and gcc ML. I did so for you now. ----- Original Message ----- > We only quote arguments that contain spaces, \n \t \v > or " characters to prevent wasting 2 characters per > argument of the CreateProcess() 32,768 limit. > > libiberty/ > * pex-win32.c (argv_to_cmdline): Don't quote > args unnecessarily The changes to changelog shouldn't be part of the patch itself. Just write into mail the changelog entry patch needs to have. Eg as: Changelog libiberty/ * pex-win32.c (argv_to_cmdline): Don't quote args unnecessarily > --- > libiberty/ChangeLog | 5 +++++ > libiberty/pex-win32.c | 48 +++++++++++++++++++++++++++++++++++++++--------- > 2 files changed, 44 insertions(+), 9 deletions(-) > > diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog > index d9a208b..f6a4f8f 100644 > --- a/libiberty/ChangeLog > +++ b/libiberty/ChangeLog > @@ -1,3 +1,8 @@ > +2014-04-14 Ray Donnelly <mingw.android@gmail.com> > + > + * pex-win32.c (argv_to_cmdline): Don't quote > + args unnecessarily. > + > 2014-04-17 Jakub Jelinek <jakub@redhat.com> > > PR sanitizer/56781 > diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c > index eae72c5..775b53c 100644 > --- a/libiberty/pex-win32.c > +++ b/libiberty/pex-win32.c > @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv) > char *p; > size_t cmdline_len; > int i, j, k; > + int needs_quotes; > > cmdline_len = 0; > for (i = 0; argv[i]; i++) > { > - /* We quote every last argument. This simplifies the problem; > - we need only escape embedded double-quotes and immediately > + /* We only quote arguments that contain spaces, \n \t \v or " > characters > + to prevent wasting 2 chars per argument of the CreateProcess 32k char > + limit We need only escape embedded double-quotes and immediately > preceeding backslash characters. A sequence of backslach characters > that is not follwed by a double quote character will not be > escaped. */ > + needs_quotes = 0; > for (j = 0; argv[i][j]; j++) > { > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > + argv[i][j] == '\t' || argv[i][j] == '"' ) > + { Here seems to be an intend issue. > + needs_quotes = 1; > + } > + > if (argv[i][j] == '"') > { > /* Escape preceeding backslashes. */ > @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv) > } > /* Trailing backslashes also need to be escaped because they will be > followed by the terminating quote. */ > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > - cmdline_len++; > + if (needs_quotes) > + { > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > + cmdline_len++; > + } > cmdline_len += j; > - cmdline_len += 3; /* for leading and trailing quotes and space */ > + /* for leading and trailing quotes and space */ > + cmdline_len += needs_quotes * 2 + 1; > } > cmdline = XNEWVEC (char, cmdline_len); > p = cmdline; > for (i = 0; argv[i]; i++) > { > - *p++ = '"'; > + needs_quotes = 0; > + for (j = 0; argv[i][j]; j++) > + { > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > + argv[i][j] == '\t' || argv[i][j] == '"' ) > + { > + needs_quotes = 1; > + break; > + } > + } > + > + if (needs_quotes) > + { > + *p++ = '"'; > + } > for (j = 0; argv[i][j]; j++) > { > if (argv[i][j] == '"') > @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv) > } > *p++ = argv[i][j]; > } > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > - *p++ = '\\'; > - *p++ = '"'; > + if (needs_quotes) > + { > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > + *p++ = '\\'; > + *p++ = '"'; > + } > *p++ = ' '; > } > p[-1] = '\0'; > -- > 1.9.2 Patch itself makes sense. Let see if there are additional comments. Regards, Kai
> Date: Sat, 19 Apr 2014 16:23:33 -0400 (EDT) > From: Kai Tietz <ktietz@redhat.com> > Cc: gcc-patches@gcc.gnu.org, ktietz70@gmail.com, "binutils@sourceware.org Development" <binutils@sourceware.org>, gdb-patches@sourceware.org > > > + /* We only quote arguments that contain spaces, \n \t \v or " characters > > + to prevent wasting 2 chars per argument of the CreateProcess 32k char > > + limit We need only escape embedded double-quotes and immediately > > preceeding backslash characters. A sequence of backslach characters > > that is not follwed by a double quote character will not be > > escaped. */ > > + needs_quotes = 0; > > for (j = 0; argv[i][j]; j++) > > { > > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > > + argv[i][j] == '\t' || argv[i][j] == '"' ) > > + { > Here seems to be an intend issue. > > + needs_quotes = 1; > > + } I think you can omit the \n case, since command arguments on Windows cannot possibly include newlines. Also, the comment speaks about \v, but I see no code to handle that (and am not sure you should bother in that case as well). > Patch itself makes sense. Yes, I agree.
> Changelog libiberty/ > * pex-win32.c (argv_to_cmdline): Don't quote > args unnecessarily Some minor comments... > > diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c > > index eae72c5..775b53c 100644 > > --- a/libiberty/pex-win32.c > > +++ b/libiberty/pex-win32.c > > @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv) > > char *p; > > size_t cmdline_len; > > int i, j, k; > > + int needs_quotes; > > > > cmdline_len = 0; > > for (i = 0; argv[i]; i++) > > { > > - /* We quote every last argument. This simplifies the problem; > > - we need only escape embedded double-quotes and immediately > > + /* We only quote arguments that contain spaces, \n \t \v or " > > characters > > + to prevent wasting 2 chars per argument of the CreateProcess 32k char > > + limit We need only escape embedded double-quotes and immediately Period missing after "limit". JIC, remember that we ask that periods be followed by 2 spaces. > > preceeding backslash characters. A sequence of backslach characters > > that is not follwed by a double quote character will not be > > escaped. */ > > + needs_quotes = 0; > > for (j = 0; argv[i][j]; j++) > > { > > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > > + argv[i][j] == '\t' || argv[i][j] == '"' ) GNU Coding Style requirement: Binary operators should be at the start of the next line, not at the end. For instance: if (argv[i][j] == ' ' || argv[i][j] == '\n' || argv[i][j] == '\t' || argv[i][j] == '"') Also, watch out for the extra space before ')' - please remove it. > > + { > Here seems to be an intend issue. > > + needs_quotes = 1; > > + } > > + > > if (argv[i][j] == '"') > > { > > /* Escape preceeding backslashes. */ > > @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv) > > } > > /* Trailing backslashes also need to be escaped because they will be > > followed by the terminating quote. */ > > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > - cmdline_len++; > > + if (needs_quotes) > > + { > > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > + cmdline_len++; > > + } > > cmdline_len += j; > > - cmdline_len += 3; /* for leading and trailing quotes and space */ > > + /* for leading and trailing quotes and space */ > > + cmdline_len += needs_quotes * 2 + 1; > > } > > cmdline = XNEWVEC (char, cmdline_len); > > p = cmdline; > > for (i = 0; argv[i]; i++) > > { > > - *p++ = '"'; > > + needs_quotes = 0; > > + for (j = 0; argv[i][j]; j++) > > + { > > + if (argv[i][j] == ' ' || argv[i][j] == '\n' || > > + argv[i][j] == '\t' || argv[i][j] == '"' ) Same as above. > > + { > > + needs_quotes = 1; > > + break; > > + } > > + } > > + > > + if (needs_quotes) > > + { > > + *p++ = '"'; > > + } > > for (j = 0; argv[i][j]; j++) > > { > > if (argv[i][j] == '"') > > @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv) > > } > > *p++ = argv[i][j]; > > } > > - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > - *p++ = '\\'; > > - *p++ = '"'; > > + if (needs_quotes) > > + { > > + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) > > + *p++ = '\\'; > > + *p++ = '"'; > > + } > > *p++ = ' '; > > } > > p[-1] = '\0'; > > -- > > 1.9.2 > > Patch itself makes sense. Let see if there are additional comments. > > Regards, > Kai
diff --git a/libiberty/ChangeLog b/libiberty/ChangeLog index d9a208b..f6a4f8f 100644 --- a/libiberty/ChangeLog +++ b/libiberty/ChangeLog @@ -1,3 +1,8 @@ +2014-04-14 Ray Donnelly <mingw.android@gmail.com> + + * pex-win32.c (argv_to_cmdline): Don't quote + args unnecessarily. + 2014-04-17 Jakub Jelinek <jakub@redhat.com> PR sanitizer/56781 diff --git a/libiberty/pex-win32.c b/libiberty/pex-win32.c index eae72c5..775b53c 100644 --- a/libiberty/pex-win32.c +++ b/libiberty/pex-win32.c @@ -340,17 +340,26 @@ argv_to_cmdline (char *const *argv) char *p; size_t cmdline_len; int i, j, k; + int needs_quotes; cmdline_len = 0; for (i = 0; argv[i]; i++) { - /* We quote every last argument. This simplifies the problem; - we need only escape embedded double-quotes and immediately + /* We only quote arguments that contain spaces, \n \t \v or " characters + to prevent wasting 2 chars per argument of the CreateProcess 32k char + limit We need only escape embedded double-quotes and immediately preceeding backslash characters. A sequence of backslach characters that is not follwed by a double quote character will not be escaped. */ + needs_quotes = 0; for (j = 0; argv[i][j]; j++) { + if (argv[i][j] == ' ' || argv[i][j] == '\n' || + argv[i][j] == '\t' || argv[i][j] == '"' ) + { + needs_quotes = 1; + } + if (argv[i][j] == '"') { /* Escape preceeding backslashes. */ @@ -362,16 +371,34 @@ argv_to_cmdline (char *const *argv) } /* Trailing backslashes also need to be escaped because they will be followed by the terminating quote. */ - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) - cmdline_len++; + if (needs_quotes) + { + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) + cmdline_len++; + } cmdline_len += j; - cmdline_len += 3; /* for leading and trailing quotes and space */ + /* for leading and trailing quotes and space */ + cmdline_len += needs_quotes * 2 + 1; } cmdline = XNEWVEC (char, cmdline_len); p = cmdline; for (i = 0; argv[i]; i++) { - *p++ = '"'; + needs_quotes = 0; + for (j = 0; argv[i][j]; j++) + { + if (argv[i][j] == ' ' || argv[i][j] == '\n' || + argv[i][j] == '\t' || argv[i][j] == '"' ) + { + needs_quotes = 1; + break; + } + } + + if (needs_quotes) + { + *p++ = '"'; + } for (j = 0; argv[i][j]; j++) { if (argv[i][j] == '"') @@ -382,9 +409,12 @@ argv_to_cmdline (char *const *argv) } *p++ = argv[i][j]; } - for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) - *p++ = '\\'; - *p++ = '"'; + if (needs_quotes) + { + for (k = j - 1; k >= 0 && argv[i][k] == '\\'; k--) + *p++ = '\\'; + *p++ = '"'; + } *p++ = ' '; } p[-1] = '\0';