OLPC regression, ofdisk stops working

[Robert Millan <rmh@aybabtu.com>]

Hi,

I got completely puzzled at this one.   Turns out r2132 broke ofdisk on
OLPC.  But I don't see anything wrong in this commit.

I isolated the change that causes this breakage, and it's very confusing.
It turns out that allocating devtype in the heap instead of the stack
causes its result to be truncated to 4 bytes (+ null terminator).

I'm not sure what can we do about this.  If we're certain it's an OFW
bug, perhaps we could workaround it by comparing only the first 4 bytes
of the result.  This worked for me:

-      if (! grub_strcmp (alias->type, "block"))
+      if (! grub_strncmp (alias->type, "bloc", 4))

(but using the existing "workaround framework")

but it's scary.  I don't know if this 4-byte limit is consistent, or
will differ arbitrarily.  Maybe we could issue a warning or so, I don't
know.

Is there someone else (Bean?) who can reproduce this problem?  Does it
fail in the same way?

On Wed, Apr 22, 2009 at 09:46:55AM +0000, David S. Miller wrote:
> Revision: 2132
>           http://svn.sv.gnu.org/viewvc/?view=rev&root=grub&revision=2132
> Author:   davem
> Date:     2009-04-22 09:46:54 +0000 (Wed, 22 Apr 2009)
> Log Message:
> -----------
> 	* include/grub/ieee1275/ieee1275.h (IEEE1275_MAX_PROP_LEN,
> 	IEEE1275_MAX_PATH_LEN): Define.
> 	* kern/ieee1275/openfw.c (grub_children_iterate): Dynamically
> 	allocate 'childtype', 'childpath', 'childname', and 'fullname'.
> 	(grub_devalias_iterate): Dynamically allocate 'aliasname' and
> 	'devtype'.  Explicitly NULL terminate devalias expansion.
> 
> Modified Paths:
> --------------
>     trunk/grub2/ChangeLog
>     trunk/grub2/include/grub/ieee1275/ieee1275.h
>     trunk/grub2/kern/ieee1275/openfw.c
> 
> Modified: trunk/grub2/ChangeLog
> ===================================================================
> --- trunk/grub2/ChangeLog	2009-04-22 09:45:43 UTC (rev 2131)
> +++ trunk/grub2/ChangeLog	2009-04-22 09:46:54 UTC (rev 2132)
> @@ -3,6 +3,13 @@
>  	* kern/ieee1275/mmap.c (grub_machine_mmap_iterate): If size_cells
>  	is larger than address_cells, use that value for address_cells too.
>  
> +	* include/grub/ieee1275/ieee1275.h (IEEE1275_MAX_PROP_LEN,
> +	IEEE1275_MAX_PATH_LEN): Define.
> +	* kern/ieee1275/openfw.c (grub_children_iterate): Dynamically
> +	allocate 'childtype', 'childpath', 'childname', and 'fullname'.
> +	(grub_devalias_iterate): Dynamically allocate 'aliasname' and
> +	'devtype'.  Explicitly NULL terminate devalias expansion.
> +
>  2009-04-19  Vladimir Serbinenko <phcoder@gmail.com>
>  
>  	Correct GPT definition
> 
> Modified: trunk/grub2/include/grub/ieee1275/ieee1275.h
> ===================================================================
> --- trunk/grub2/include/grub/ieee1275/ieee1275.h	2009-04-22 09:45:43 UTC (rev 2131)
> +++ trunk/grub2/include/grub/ieee1275/ieee1275.h	2009-04-22 09:46:54 UTC (rev 2132)
> @@ -39,6 +39,9 @@
>    unsigned int size;
>  };
>  
> +#define IEEE1275_MAX_PROP_LEN	8192
> +#define IEEE1275_MAX_PATH_LEN	256
> +
>  #ifndef IEEE1275_CALL_ENTRY_FN
>  #define IEEE1275_CALL_ENTRY_FN(args) (*grub_ieee1275_entry_fn) (args)
>  #endif
> 
> Modified: trunk/grub2/kern/ieee1275/openfw.c
> ===================================================================
> --- trunk/grub2/kern/ieee1275/openfw.c	2009-04-22 09:45:43 UTC (rev 2131)
> +++ trunk/grub2/kern/ieee1275/openfw.c	2009-04-22 09:46:54 UTC (rev 2132)
> @@ -38,6 +38,8 @@
>  {
>    grub_ieee1275_phandle_t dev;
>    grub_ieee1275_phandle_t child;
> +  char *childtype, *childpath;
> +  char *childname, *fullname;
>  
>    if (grub_ieee1275_finddevice (devpath, &dev))
>      return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "Unknown device");
> @@ -45,13 +47,33 @@
>    if (grub_ieee1275_child (dev, &child))
>      return grub_error (GRUB_ERR_BAD_DEVICE, "Device has no children");
>  
> +  childtype = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +  if (!childtype)
> +    return grub_errno;
> +  childpath = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +  if (!childpath)
> +    {
> +      grub_free (childtype);
> +      return grub_errno;
> +    }
> +  childname = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +  if (!childname)
> +    {
> +      grub_free (childpath);
> +      grub_free (childtype);
> +      return grub_errno;
> +    }
> +  fullname = grub_malloc (IEEE1275_MAX_PATH_LEN);
> +  if (!fullname)
> +    {
> +      grub_free (childname);
> +      grub_free (childpath);
> +      grub_free (childtype);
> +      return grub_errno;
> +    }
> +
>    do
>      {
> -      /* XXX: Don't use hardcoded path lengths.  */
> -      char childtype[64];
> -      char childpath[64];
> -      char childname[64];
> -      char fullname[64];
>        struct grub_ieee1275_devalias alias;
>        grub_ssize_t actual;
>  
> @@ -76,6 +98,11 @@
>      }
>    while (grub_ieee1275_peer (child, &child));
>  
> +  grub_free (fullname);
> +  grub_free (childname);
> +  grub_free (childpath);
> +  grub_free (childtype);
> +
>    return 0;
>  }
>  
> @@ -85,13 +112,23 @@
>  grub_devalias_iterate (int (*hook) (struct grub_ieee1275_devalias *alias))
>  {
>    grub_ieee1275_phandle_t aliases;
> -  char aliasname[32];
> +  char *aliasname, *devtype;
>    grub_ssize_t actual;
>    struct grub_ieee1275_devalias alias;
>  
>    if (grub_ieee1275_finddevice ("/aliases", &aliases))
>      return -1;
>  
> +  aliasname = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +  if (!aliasname)
> +    return grub_errno;
> +  devtype = grub_malloc (IEEE1275_MAX_PROP_LEN);
> +  if (!devtype)
> +    {
> +      grub_free (aliasname);
> +      return grub_errno;
> +    }
> +
>    /* Find the first property.  */
>    aliasname[0] = '\0';
>  
> @@ -100,8 +137,6 @@
>        grub_ieee1275_phandle_t dev;
>        grub_ssize_t pathlen;
>        char *devpath;
> -      /* XXX: This should be large enough for any possible case.  */
> -      char devtype[64];
>  
>        grub_dprintf ("devalias", "devalias name = %s\n", aliasname);
>  
> @@ -111,9 +146,17 @@
>        if (!grub_strcmp (aliasname, "name"))
>  	continue;
>  
> +      /* Sun's OpenBoot often doesn't zero terminate the device alias
> +	 strings, so we will add a NULL byte at the end explicitly.  */
> +      pathlen += 1;
> +
>        devpath = grub_malloc (pathlen);
>        if (! devpath)
> -	return grub_errno;
> +	{
> +	  grub_free (devtype);
> +	  grub_free (aliasname);
> +	  return grub_errno;
> +	}
>  
>        if (grub_ieee1275_get_property (aliases, aliasname, devpath, pathlen,
>  				      &actual))
> @@ -121,6 +164,7 @@
>  	  grub_dprintf ("devalias", "get_property (%s) failed\n", aliasname);
>  	  goto nextprop;
>  	}
> +      devpath [actual] = '\0';
>  
>        if (grub_ieee1275_finddevice (devpath, &dev))
>  	{
> @@ -144,6 +188,8 @@
>        grub_free (devpath);
>      }
>  
> +  grub_free (devtype);
> +  grub_free (aliasname);
>    return 0;
>  }
>  
> 
> 
> 
> 

-- 
Robert Millan

  The DRM opt-in fallacy: "Your data belongs to us. We will decide when (and
  how) you may access your data; but nobody's threatening your freedom: we
  still allow you to remove your data and not access it at all."