[BUGFIX] Incorrect count of argument with rescue parser

[BUGFIX] Incorrect count of argument with rescue parser

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1: Type: text/plain, Size: 278 bytes --]

This patch fixes the parsing of two strings like following ones:
"echo 1 " was parsed into "echo", "1", ""
"echo $root" was parsed into "echo" (variable just disappeared)

-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git

[-- Attachment #2: argcfix.diff --]
[-- Type: text/plain, Size: 1245 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 5183b55..55372c4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2009-07-30  Vladimir Serbinenko  <phcoder@gmail.com>
 
+	* kern/parser.c (grub_parser_split_cmdline): Fix incorrect counting
+	of arguments.
+
+2009-07-30  Vladimir Serbinenko  <phcoder@gmail.com>
+
 	* util/i386/pc/grub-setup.c (setup): Check that no partition is in
 	embeding zone, not only the first one.
 
diff --git a/kern/parser.c b/kern/parser.c
index db59af0..4f50136 100644
--- a/kern/parser.c
+++ b/kern/parser.c
@@ -142,7 +142,7 @@ grub_parser_split_cmdline (const char *cmdline, grub_reader_getline_t getline,
 	*(bp++) = *val;
     }
 
-  *argc = 1;
+  *argc = 0;
   do
     {
       if (! *rd)
@@ -188,12 +188,16 @@ grub_parser_split_cmdline (const char *cmdline, grub_reader_getline_t getline,
 	  state = newstate;
 	}
     } while (state != GRUB_PARSER_STATE_TEXT && !check_varstate (state));
-  *(bp++) = '\0';
 
   /* A special case for when the last character was part of a
      variable.  */
   add_var (GRUB_PARSER_STATE_TEXT);
 
+  if (bp != buffer && *(bp - 1))
+    {
+      *(bp++) = '\0';
+      (*argc)++;
+    }
 
   /* Reserve memory for the return values.  */
   args = grub_malloc (bp - buffer);

Re: [BUGFIX] Incorrect count of argument with rescue parser

[Pavel Roskin]

On Fri, 2009-07-31 at 00:46 +0200, Vladimir 'phcoder' Serbinenko wrote:
> This patch fixes the parsing of two strings like following ones:
> "echo 1 " was parsed into "echo", "1", ""
> "echo $root" was parsed into "echo" (variable just disappeared)

It would be helpful if you explain how to see the difference without
tracing grub_parser_split_cmdline() in the debugger.

Also, it would be great if you explain the change.  A comment for the
newly added code would help understand it.  Otherwise it looks like the
previous comment still applies ("A special case for when the last
character was part of a variable").

Since you looked at the problem, perhaps you know why argc is
decremented before the exit.  I think it needs a comment.

Also, grub_malloc() appears to allocate two extra pointers for argv (if
we consider that argc is decremented).  argv is not supposed to be null
terminated.  I'd rather allocate just enough memory so that we could
catch abusers by running grub-emu in valgrind.

Anyway, the patch doesn't pass even minimal testing.  Pressing Tab in
grub-emu crashes it at normal/completion.c:424

(gdb) where
#0  0x00000000004179d1 in grub_normal_do_completion (buf=0x7fff5d5a29d0 "", 
    restore=0x7fff5d5a304c, hook=0x41568e <print_completion>) at normal/completion.c:424
#1  0x00000000004159d1 in grub_cmdline_get (prompt=0x7fff5d5a30e0 "sh:grub> ", 
    cmdline=0x662fa0 "", readline=1) at normal/cmdline.c:329
#2  0x0000000000418813 in grub_normal_read_line (line=0x7fff5d5a3160, cont=0)
    at normal/main.c:504
#3  0x00000000004141b3 in grub_reader_loop (getline=0) at kern/reader.c:43
#4  0x00000000004117f4 in grub_main () at kern/main.c:176
#5  0x00000000004397ab in main (argc=3, argv=0x7fff5d5a32c8) at util/grub-emu.c:236
(gdb) l
419         {
420           /* Complete a command.  */
421           if (grub_command_iterate (iterate_command))
422             goto fail;
423         }
424       else if (*current_word == '-')
425         {
426           if (complete_arguments (buf))
427             goto fail;
428         }
(gdb) p current_word
$1 = 0x21 <Address 0x21 out of bounds>
(gdb)

-- 
Regards,
Pavel Roskin

Re: [BUGFIX] Incorrect count of argument with rescue parser

[Vladimir 'phcoder' Serbinenko]

On Fri, Jul 31, 2009 at 6:17 AM, Pavel Roskin<proski@gnu.org> wrote:
> On Fri, 2009-07-31 at 00:46 +0200, Vladimir 'phcoder' Serbinenko wrote:
>> This patch fixes the parsing of two strings like following ones:
>> "echo 1 " was parsed into "echo", "1", ""
>> "echo $root" was parsed into "echo" (variable just disappeared)
>
> It would be helpful if you explain how to see the difference without
> tracing grub_parser_split_cmdline() in the debugger.
cat /hello
error: file name required
echo $root
Empty line
>
> Also, it would be great if you explain the change.  A comment for the
> newly added code would help understand it.  Otherwise it looks like the
> previous comment still applies ("A special case for when the last
> character was part of a variable").
>
Sorry I forgot to move the comment together with code
> Since you looked at the problem, perhaps you know why argc is
> decremented before the exit.  I think it needs a comment.
It's because till that point argc was count of tokens and afterwards
it's a count of argument. Example
"echo hello" gives 2 tokes "echo" and "hello" but command "echo"
recieves only one argument: "hello". But probably this should be moved
to ./kern/rescue_parser.c, ./script/lua/grub_lib.c and
./normal/completion.c (3 callers of this function)
>
> Also, grub_malloc() appears to allocate two extra pointers for argv (if
> we consider that argc is decremented).  argv is not supposed to be null
> terminated.  I'd rather allocate just enough memory so that we could
> catch abusers by running grub-emu in valgrind.
>
> Anyway, the patch doesn't pass even minimal testing.  Pressing Tab in
> grub-emu crashes it at normal/completion.c:424
>
Ok. I looked into it. I triggered another bug. When someone parses any
empty string he gets 0 tokens which means -1 arguments. Here is a fix:
diff --git a/kern/rescue_parser.c b/kern/rescue_parser.c
index 79f32b8..a93ca36 100644
--- a/kern/rescue_parser.c
+++ b/kern/rescue_parser.c
@@ -35,6 +35,9 @@ grub_rescue_parse_line (char *line,
grub_reader_getline_t getline)
   if (grub_parser_split_cmdline (line, getline, &n, &args) || n < 0)
     return grub_errno;

+  if (n == -1)
+    return GRUB_ERR_NONE;
+
   /* In case of an assignment set the environment accordingly
      instead of calling a function.  */
   if (n == 0 && grub_strchr (line, '='))
diff --git a/normal/completion.c b/normal/completion.c
index 405976a..5c2e0d1 100644
--- a/normal/completion.c
+++ b/normal/completion.c
@@ -399,14 +399,18 @@ grub_normal_do_completion (char *buf, int *restore,

   if (grub_parser_split_cmdline (buf, 0, &argc, &argv))
     return 0;
-
-  current_word = argv[argc];
+
+  if (argc == -1)
+    current_word = "";
+  else
+    current_word = argv[argc];
+

   /* Determine the state the command line is in, depending on the
      state, it can be determined how to complete.  */
   cmdline_state = get_state (buf);

-  if (argc == 0)
+  if (argc == 0 || argc == -1)
     {
       /* Complete a command.  */
       if (grub_command_iterate (iterate_command))
@@ -476,13 +480,15 @@ grub_normal_do_completion (char *buf, int *restore,
           goto fail;
 	}

-      grub_free (argv[0]);
+      if (argc != -1)
+	grub_free (argv[0]);
       grub_free (match);
       return ret;
     }

  fail:
-  grub_free (argv[0]);
+  if (argc != -1)
+    grub_free (argv[0]);
   grub_free (match);
   grub_errno = GRUB_ERR_NONE;


-- 
Regards
Vladimir 'phcoder' Serbinenko

Personal git repository: http://repo.or.cz/w/grub2/phcoder.git