two bugs in configfile parser

two bugs in configfile parser

[Patrick Georgi]

Hi,

given the following grub.cfg (simplified test case):

menuentry "test" {
if test "a" = "a" ; then
   echo foo
fi
}

I have some issues. To reproduce, load grub with that file, enter the 
editor on the menu item, run it with ctrl-x.
Two things can happen (I have some local patches which lead to one 
happening slightly more often than the other, so it seems to be quite 
sensitive to $whatever):

  1. it crashes on malloc magic problems. It seems to be related to the 
leading spaces on "echo foo". If I remove them, it works. I guess, 
they're skipped at some place, and after that, the string should be 
grub_free()d. grub's mm doesn't support that.

  2. it corrupts the text once it finishes. After removing the leading 
spaces, it runs correctly and returns to the editor. Unfortunately, 
starting with "  echo foo", the text is corrupted. Another run (with all 
those garbage strings) ends in malloc magic error.

That code runs fine if executed directly from the menu.


Regards,
Patrick Georgi

Re: two bugs in configfile parser

[Bean]

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

On Fri, Aug 1, 2008 at 7:44 AM, Patrick Georgi <patrick@georgi-clan.de> wrote:
> Hi,
>
> given the following grub.cfg (simplified test case):
>
> menuentry "test" {
> if test "a" = "a" ; then
>  echo foo
> fi
> }
>
> I have some issues. To reproduce, load grub with that file, enter the editor
> on the menu item, run it with ctrl-x.
> Two things can happen (I have some local patches which lead to one happening
> slightly more often than the other, so it seems to be quite sensitive to
> $whatever):
>
>  1. it crashes on malloc magic problems. It seems to be related to the
> leading spaces on "echo foo". If I remove them, it works. I guess, they're
> skipped at some place, and after that, the string should be grub_free()d.
> grub's mm doesn't support that.
>
>  2. it corrupts the text once it finishes. After removing the leading
> spaces, it runs correctly and returns to the editor. Unfortunately, starting
> with "  echo foo", the text is corrupted. Another run (with all those
> garbage strings) ends in malloc magic error.
>
> That code runs fine if executed directly from the menu.

Hi,

Ok, I've found the bug. In editor_getline (normal/menu_entry.c), it
should return a string allocated with grub_strdup, instead of the
original one, as the result will be release in the lexer once it's
done.


-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: getline.diff --]
[-- Type: text/x-diff; name=getline.diff, Size: 370 bytes --]

diff --git a/normal/menu_entry.c b/normal/menu_entry.c
index 5faeef0..8a106f2 100644
--- a/normal/menu_entry.c
+++ b/normal/menu_entry.c
@@ -996,7 +996,7 @@ run (struct screen *screen)
       linep->len = p - linep->buf;
       for (p = linep->buf; grub_isspace (*p); p++)
 	;
-      *line = p;
+      *line = grub_strdup (p);
       currline++;
       return 0;
     }

Re: two bugs in configfile parser

[Marco Gerards]

Hi,

Bean <bean123ch@gmail.com> writes:

> Ok, I've found the bug. In editor_getline (normal/menu_entry.c), it
> should return a string allocated with grub_strdup, instead of the
> original one, as the result will be release in the lexer once it's
> done.

The patch looks fine.  You can commit it, if you didn't do this
already.

--
Marco

Re: two bugs in configfile parser

[Bean]

On Tue, Aug 5, 2008 at 6:17 PM, Marco Gerards <mgerards@xs4all.nl> wrote:
> Hi,
>
> Bean <bean123ch@gmail.com> writes:
>
>> Ok, I've found the bug. In editor_getline (normal/menu_entry.c), it
>> should return a string allocated with grub_strdup, instead of the
>> original one, as the result will be release in the lexer once it's
>> done.
>
> The patch looks fine.  You can commit it, if you didn't do this
> already.

Oh, I have committed it.

-- 
Bean