a bug in read

a bug in read

[Yoshinori K. Okuji]

Hello,

I have noticed that read.c has a bug. In this line:

  while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))

LINE is not initialized yet at the first time, so this refers to a 
uninitialized location.

Regards,
Okuji

Re: a bug in read

[Pavel Roskin]

On Tue, 2008-07-08 at 00:18 +0200, Yoshinori K. Okuji wrote:
> Hello,
> 
> I have noticed that read.c has a bug. In this line:
> 
>   while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
> 
> LINE is not initialized yet at the first time, so this refers to a 
> uninitialized location.

Thank you!  What's worse, i is 0, so we are reading outside the buffer.
I think this patch should do what the code was meant to do:

diff --git a/commands/read.c b/commands/read.c
index 1995918..96519f8 100644
--- a/commands/read.c
+++ b/commands/read.c
@@ -30,15 +30,16 @@ grub_getline (void)
   int i;
   char *line;
   char *tmp;
+  char last = 0;
 
   i = 0;
   line = grub_malloc (1 + i + sizeof('\0'));
   if (! line)
     return NULL;
 
-  while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
+  while ((last != '\n') && (last != '\r'))
     {
-      line[i] = grub_getkey ();
+      last = line[i] = grub_getkey ();
       if (grub_isprint (line[i]))
 	grub_putchar (line[i]);
       i++;


We should test all grub utilities in Valgrind to find such problems.

By the way, read is not a part of grub-emu.  We'll need to improve the
build system to make such oversights less likely.  We also need "exit"
in grub-emu, as "reboot" doesn't sound right.

-- 
Regards,
Pavel Roskin

Re: a bug in read

[Vesa Jääskeläinen]

Pavel Roskin wrote:
> By the way, read is not a part of grub-emu.  We'll need to improve the
> build system to make such oversights less likely.  We also need "exit"
> in grub-emu, as "reboot" doesn't sound right.

I think there should be emu arch that provides architecture specific 
functionality. This way we use exactly same code paths for everything. 
Then we can catch bugs more easily.

Re: a bug in read

[Javier Martín]

Couldn't this have been worked around changing while to do-while and
refactoring the loop instead of creating a new variable, whose
handling takes space? I'm not sure it can be done because I'm on
vacation and reading mail through dialup access, but it might be worth
a try.

2008/7/9, Pavel Roskin <proski@gnu.org>:
> On Tue, 2008-07-08 at 00:18 +0200, Yoshinori K. Okuji wrote:
> > Hello,
> >
> > I have noticed that read.c has a bug. In this line:
> >
> >   while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
> >
> > LINE is not initialized yet at the first time, so this refers to a
> > uninitialized location.
>
> Thank you!  What's worse, i is 0, so we are reading outside the buffer.
> I think this patch should do what the code was meant to do:
>
> diff --git a/commands/read.c b/commands/read.c
> index 1995918..96519f8 100644
> --- a/commands/read.c
> +++ b/commands/read.c
> @@ -30,15 +30,16 @@ grub_getline (void)
>   int i;
>   char *line;
>   char *tmp;
> +  char last = 0;
>
>   i = 0;
>   line = grub_malloc (1 + i + sizeof('\0'));
>   if (! line)
>     return NULL;
>
> -  while ((line[i - 1] != '\n') && (line[i - 1] != '\r'))
> +  while ((last != '\n') && (last != '\r'))
>     {
> -      line[i] = grub_getkey ();
> +      last = line[i] = grub_getkey ();
>       if (grub_isprint (line[i]))
>        grub_putchar (line[i]);
>       i++;
>
>
> We should test all grub utilities in Valgrind to find such problems.
>
> By the way, read is not a part of grub-emu.  We'll need to improve the
> build system to make such oversights less likely.  We also need "exit"
> in grub-emu, as "reboot" doesn't sound right.
>
> --
> Regards,
> Pavel Roskin
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> http://lists.gnu.org/mailman/listinfo/grub-devel
>

Re: a bug in read

[Pavel Roskin]

On Wed, 2008-07-09 at 19:56 +0200, Javier Martín wrote:
> Couldn't this have been worked around changing while to do-while and
> refactoring the loop instead of creating a new variable, whose
> handling takes space? I'm not sure it can be done because I'm on
> vacation and reading mail through dialup access, but it might be worth
> a try.

A new variable was useful anyway to strip the final newline, but
changing the loop logic made the code more readable.  It's a standard
case where the exit condition occurs in the middle of the block, not in
the beginning or in the end.

The patch has been applied.

-- 
Regards,
Pavel Roskin