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

[Vladimir 'phcoder' Serbinenko]

On Fri, Jul 31, 2009 at 6:17 AM, Pavel Roskin<address@hidden> 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