strndup broken

strndup broken

[Hollis Blanchard]

A while back I made the unpleasent discovery that grub_strndup was
broken. This patch fixes it, but the author has been having trouble
sending it out.

-Hollis

2005-01-08  Ian Abel  <iga20@cam.ac.uk>

	* kern/misc.c (grub_strndup): Don't call grub_strlen.
	Null-terminate the new string if possible.

Index: kern/misc.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/misc.c,v
retrieving revision 1.15
diff -u -p -r1.15 misc.c
--- kern/misc.c	14 Sep 2004 08:56:51 -0000	1.15
+++ kern/misc.c	8 Jan 2005 23:47:55 -0000
@@ -341,16 +341,21 @@ grub_strndup (const char *s, grub_size_t
 {
   grub_size_t len = 0;
   char *p = (char *) s;
-  
+
   while (*(p++) && len < n)
     len++;
 
-  len = grub_strlen (s) + 1;
+  if (*p)
+    len++;
+
   p = (char *) grub_malloc (len);
   if (! p)
     return 0;
 
-  return grub_memcpy (p, s, len);
+  if (n < len)
+    p[n] = '\0';
+
+  return grub_memcpy (p, s, n);
 }
 
 void *

Re: strndup broken

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

> A while back I made the unpleasent discovery that grub_strndup was
> broken. This patch fixes it, but the author has been having trouble
> sending it out.

Because of copyright problems (Ian did not assign his copyrights yet)
and especially because the old function was just plain wrong I have
rewritten it.

Can you check if it works for you?  If I don't hear anything I will
commit it next week or so.

Thanks,
Marco

2005-01-21  Marco Gerards  <metgerards@student.han.nl>

	* kern/misc.c (grub_strndup): Function rewritten.


Index: kern/misc.c
===================================================================
RCS file: /cvsroot/grub/grub2/kern/misc.c,v
retrieving revision 1.16
diff -u -p -u -p -r1.16 misc.c
--- kern/misc.c	1 Nov 2004 16:14:16 -0000	1.16
+++ kern/misc.c	21 Jan 2005 20:15:00 -0000
@@ -354,18 +354,19 @@ grub_strdup (const char *s)
 char *
 grub_strndup (const char *s, grub_size_t n)
 {
-  grub_size_t len = 0;
-  char *p = (char *) s;
+  grub_size_t len;
+  char *p;
   
-  while (*(p++) && len < n)
-    len++;
-
-  len = grub_strlen (s) + 1;
-  p = (char *) grub_malloc (len);
+  len = grub_strlen (s);
+  if (len > n)
+    len = n;
+  p = (char *) grub_malloc (len + 1);
   if (! p)
     return 0;
-
-  return grub_memcpy (p, s, len);
+  
+  grub_memcpy (p, s, len);
+  p[len] = '\0';
+  return p;
 }
 
 void *

Re: strndup broken

[Hollis Blanchard]

On Jan 21, 2005, at 2:19 PM, Marco Gerards wrote:

> Hollis Blanchard <hollis@penguinppc.org> writes:
>
>> A while back I made the unpleasent discovery that grub_strndup was
>> broken. This patch fixes it, but the author has been having trouble
>> sending it out.
>
> Because of copyright problems (Ian did not assign his copyrights yet)
> and especially because the old function was just plain wrong I have
> rewritten it.
>
> Can you check if it works for you?  If I don't hear anything I will
> commit it next week or so.

This exposed a stupid bug I introduced months ago:

--- disk/powerpc/ieee1275/ofdisk.c      27 Dec 2004 13:46:20 -0000      
1.6
+++ disk/powerpc/ieee1275/ofdisk.c      22 Jan 2005 05:37:16 -0000
@@ -50,12 +50,12 @@ grub_ofdisk_open (const char *name, grub
  {
    grub_ieee1275_phandle_t dev;
    grub_ieee1275_ihandle_t dev_ihandle = 0;
-  char *devpath = 0;
+  char *devpath;
    /* XXX: This should be large enough for any possible case.  */
    char prop[64];
    int actual;

-  devpath = grub_strndup (name, grub_strlen (devpath) + 2);
+  devpath = grub_strndup (name, grub_strlen (name) + 2);
    if (! devpath)
      return grub_errno;

`devpath' is clearly NULL here.

I think it's in all our best interests to get a working strndup in 
soon. :)

-Hollis

Re: strndup broken

[Marco Gerards]

Hollis Blanchard <hollis@penguinppc.org> writes:

>> Can you check if it works for you?  If I don't hear anything I will
>> commit it next week or so.
>
> This exposed a stupid bug I introduced months ago:
>
> --- disk/powerpc/ieee1275/ofdisk.c      27 Dec 2004 13:46:20 -0000
> 1.6
> +++ disk/powerpc/ieee1275/ofdisk.c      22 Jan 2005 05:37:16 -0000
> @@ -50,12 +50,12 @@ grub_ofdisk_open (const char *name, grub
>   {
>     grub_ieee1275_phandle_t dev;
>     grub_ieee1275_ihandle_t dev_ihandle = 0;
> -  char *devpath = 0;
> +  char *devpath;
>     /* XXX: This should be large enough for any possible case.  */
>     char prop[64];
>     int actual;
>
> -  devpath = grub_strndup (name, grub_strlen (devpath) + 2);
> +  devpath = grub_strndup (name, grub_strlen (name) + 2);
>     if (! devpath)
>       return grub_errno;
>
> `devpath' is clearly NULL here.

Sorry, I don't know what you mean?  If you mean this patch fixes a bug
you introduced, please write a changelog so this can be committed
ASAP. :)

> I think it's in all our best interests to get a working strndup in
> soon. :)

Yeah!  I guess you mean that you have tested grub_strndup and that it
works?  In that case please tell me so I can apply it now. :)

Thanks,
Marco