GNOME Bugzilla – Bug 562907
g_shell_parse_argv() mishandles # (hash)
Last modified: 2012-09-13 08:23:10 UTC
g_shell_parse_argv() always treats unquoted # as if it was a comment start, though in real shell it does not necessarily start a comment: "If the previous character was part of a word, the current character shall be appended to that word.". Below is a test case. It should correctly parse strings into ['echo', 'a#b'] and ['echo', 'a#'] but it doesn't. #include <glib.h> static void parse (const char *string) { int i, argc; char **argv; GError *error = NULL; if (!g_shell_parse_argv (string, &argc, &argv, &error)) g_error ("oops: %s", error->message); g_print ("'%s' got parsed into:\n", string); for (i = 0; i < argc; ++i) g_print (" %s\n", argv[i]); g_strfreev (argv); } int main (void) { parse ("echo a#b"); parse ("echo a#"); return 0; }
Created attachment 221469 [details] [review] An attempt to fix the parser in glib
Review of attachment 221469 [details] [review]: ::: glib/gshell.c @@ +525,2 @@ case '#': + if (*(p-1)) { Access to p[-1] needs a check if we are at the beginning of the string. Also, I don't understand why we have this if here in the first place - you seem to handle '\0' in the switch anyway... Should add a testcases for # as the first/last character of the string @@ +534,3 @@ + default: + ensure_token (¤t_token); + g_string_append_c (current_token, *p); Please include a break; in the default case as well @@ +574,3 @@ + * error path if current_quote is NOT '#' + */ + if (current_quote && current_quote != '#') Should add a testcase demonstrating why this change is necessary
Thanks for the review. I'll see what I can fixup of those comments. I'm not that proficient in coding :)
Created attachment 221560 [details] Reworked patch Your comment 1 and 2 have been addressed in this patch. Comment 3: I'll need some pointers I think. I see a bunch of tests that check for 'invalid quoting' without the change from comment 3, this test would be ok: { "foo #bar", 0, { NULL }, G_SHELL_ERROR_BAD_QUOTING }, => but is actually wrong, as # does not require to be 'closed' with a 2nd # at the end. Does a test like { "foo #bar", 0, { NULL }, !G_SHELL_ERROR_BAD_QUOTING }, work? That would make the check correct.
(I can't really test the suite, as make check fails even without my patch: https://bugzilla.gnome.org/show_bug.cgi?id=682074)
f (current_quote && current_quote != '#') seems actually not needed. I could not reproduce any test failing on that, so that should be fine to be skipped. Will attach a patch without this change
Created attachment 221586 [details] [review] 3rd variant... I think this should be about it now.
(In reply to comment #5) > (I can't really test the suite, as make check fails even without my patch: > https://bugzilla.gnome.org/show_bug.cgi?id=682074) This obstacle was overcome.. so the test suite passes with and without my patch. The new tests only pass with my patch, which sounds correct to me.
Do we see chances to merge this fix for an old bug in the current cycle? Or would you prefer to have this only for gnome 3.8 target?
Review of attachment 221586 [details] [review]: No, its fine. Sorry I forgot to set the status here
(In reply to comment #10) > Review of attachment 221586 [details] [review]: > /me has no commit access
The following fix has been pushed: 6e4acf4 gshell: Fix parsing of comments in command lines.
Created attachment 222907 [details] [review] gshell: Fix parsing of comments in command lines. Fixes bug 562907
This causes bug 683821, as it breaks the fallthrough case for '\\'
Created attachment 224198 [details] [review] Fix regression in g_shell_parse_argv() The commit in 6e4acf44b3a943906432a2bf55223ac107d8e0c2 broke the fallthrough case for '\\' when it changed the '#' case. This caused issues like this: https://bugzilla.gnome.org/show_bug.cgi?id=683821
*** Bug 683821 has been marked as a duplicate of this bug. ***
Attachment 224198 [details] pushed as c99acf5 - Fix regression in g_shell_parse_argv()
Sorry for that break... and Alex! Thanks for the quick fix!