libfdt: Fix bugs in fdt_get_path()

libfdt: Fix bugs in fdt_get_path()

[David Gibson]

The current implementation of fdt_get_path() has a couple of bugs,
fixed by this patch.

First, contrary to its documentation, on success it returns the length
of the node's path, rather than 0.  The testcase is correspondingly
wrong, and the patch fixes this as well.

Second, in some circumstances, it will return -FDT_ERR_BADOFFSET
instead of -FDT_ERR_NOSPACE when given insufficient buffer space.
Specifically this happens when there is insufficient space even to
hold the path's second last component.  This behaviour is corrected,
and the testcase updated to check it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: dtc/tests/get_path.c
===================================================================
--- dtc.orig/tests/get_path.c	2008-08-29 14:11:09.000000000 +1000
+++ dtc/tests/get_path.c	2008-08-29 14:11:11.000000000 +1000
@@ -43,6 +43,8 @@ static void check_path_buf(void *fdt, co
 	memset(buf, POISON, sizeof(buf)); /* poison the buffer */
 
 	len = fdt_get_path(fdt, offset, buf, buflen);
+	verbose_printf("get_path() %s -> %d -> %s\n", path, offset, buf);
+
 	if (buflen <= pathlen) {
 		if (len != -FDT_ERR_NOSPACE)
 			FAIL("fdt_get_path([%d bytes]) returns %d with "
@@ -51,9 +53,9 @@ static void check_path_buf(void *fdt, co
 		if (len < 0)
 			FAIL("fdt_get_path([%d bytes]): %s", buflen,
 			     fdt_strerror(len));
-		if (len != pathlen)
-			FAIL("fdt_get_path([%d bytes]) reports length %d "
-			     "instead of %d", buflen, len, pathlen);
+		if (len != 0)
+			FAIL("fdt_get_path([%d bytes]) returns %d "
+			     "instead of 0", buflen, len);
 		if (strcmp(buf, path) != 0)
 			FAIL("fdt_get_path([%d bytes]) returns \"%s\" "
 			     "instead of \"%s\"", buflen, buf, path);
@@ -70,6 +72,8 @@ static void check_path(void *fdt, const 
 	check_path_buf(fdt, path, pathlen, 1024);
 	check_path_buf(fdt, path, pathlen, pathlen+1);
 	check_path_buf(fdt, path, pathlen, pathlen);
+	check_path_buf(fdt, path, pathlen, 0);
+	check_path_buf(fdt, path, pathlen, 2);
 }
 
 int main(int argc, char *argv[])
Index: dtc/libfdt/fdt_ro.c
===================================================================
--- dtc.orig/libfdt/fdt_ro.c	2008-08-29 14:11:11.000000000 +1000
+++ dtc/libfdt/fdt_ro.c	2008-08-29 14:11:11.000000000 +1000
@@ -328,9 +328,6 @@ int fdt_get_path(const void *fdt, int no
 	for (offset = 0, depth = 0;
 	     (offset >= 0) && (offset <= nodeoffset);
 	     offset = fdt_next_node(fdt, offset, &depth)) {
-		if (pdepth < depth)
-			continue; /* overflowed buffer */
-
 		while (pdepth > depth) {
 			do {
 				p--;
@@ -338,14 +335,16 @@ int fdt_get_path(const void *fdt, int no
 			pdepth--;
 		}
 
-		name = fdt_get_name(fdt, offset, &namelen);
-		if (!name)
-			return namelen;
-		if ((p + namelen + 1) <= buflen) {
-			memcpy(buf + p, name, namelen);
-			p += namelen;
-			buf[p++] = '/';
-			pdepth++;
+		if (pdepth >= depth) {
+			name = fdt_get_name(fdt, offset, &namelen);
+			if (!name)
+				return namelen;
+			if ((p + namelen + 1) <= buflen) {
+				memcpy(buf + p, name, namelen);
+				p += namelen;
+				buf[p++] = '/';
+				pdepth++;
+			}
 		}
 
 		if (offset == nodeoffset) {
@@ -355,7 +354,7 @@ int fdt_get_path(const void *fdt, int no
 			if (p > 1) /* special case so that root path is "/", not "" */
 				p--;
 			buf[p] = '\0';
-			return p;
+			return 0;
 		}
 	}
 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[U-Boot] libfdt: Fix bugs in fdt_get_path()

[Jerry Van Baren]

FYI, I propose to pick this up for the *NEXT* release of u-boot ("Yellow 
Submarine" ;-).  My reasoning is that (1) the current window closes 
today, which makes me uncomfortable WRT putting in changes today and (2) 
this appears to be a latent bug (at worst, a misleading error message); 
I have not seen evidence that it is causing problems at the moment.

Best regards,
gvb

David Gibson wrote:
> The current implementation of fdt_get_path() has a couple of bugs,
> fixed by this patch.
> 
> First, contrary to its documentation, on success it returns the length
> of the node's path, rather than 0.  The testcase is correspondingly
> wrong, and the patch fixes this as well.
> 
> Second, in some circumstances, it will return -FDT_ERR_BADOFFSET
> instead of -FDT_ERR_NOSPACE when given insufficient buffer space.
> Specifically this happens when there is insufficient space even to
> hold the path's second last component.  This behaviour is corrected,
> and the testcase updated to check it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Index: dtc/tests/get_path.c
> ===================================================================
> --- dtc.orig/tests/get_path.c	2008-08-29 14:11:09.000000000 +1000
> +++ dtc/tests/get_path.c	2008-08-29 14:11:11.000000000 +1000
> @@ -43,6 +43,8 @@ static void check_path_buf(void *fdt, co
>  	memset(buf, POISON, sizeof(buf)); /* poison the buffer */
>  
>  	len = fdt_get_path(fdt, offset, buf, buflen);
> +	verbose_printf("get_path() %s -> %d -> %s\n", path, offset, buf);
> +
>  	if (buflen <= pathlen) {
>  		if (len != -FDT_ERR_NOSPACE)
>  			FAIL("fdt_get_path([%d bytes]) returns %d with "
> @@ -51,9 +53,9 @@ static void check_path_buf(void *fdt, co
>  		if (len < 0)
>  			FAIL("fdt_get_path([%d bytes]): %s", buflen,
>  			     fdt_strerror(len));
> -		if (len != pathlen)
> -			FAIL("fdt_get_path([%d bytes]) reports length %d "
> -			     "instead of %d", buflen, len, pathlen);
> +		if (len != 0)
> +			FAIL("fdt_get_path([%d bytes]) returns %d "
> +			     "instead of 0", buflen, len);
>  		if (strcmp(buf, path) != 0)
>  			FAIL("fdt_get_path([%d bytes]) returns \"%s\" "
>  			     "instead of \"%s\"", buflen, buf, path);
> @@ -70,6 +72,8 @@ static void check_path(void *fdt, const 
>  	check_path_buf(fdt, path, pathlen, 1024);
>  	check_path_buf(fdt, path, pathlen, pathlen+1);
>  	check_path_buf(fdt, path, pathlen, pathlen);
> +	check_path_buf(fdt, path, pathlen, 0);
> +	check_path_buf(fdt, path, pathlen, 2);
>  }
>  
>  int main(int argc, char *argv[])
> Index: dtc/libfdt/fdt_ro.c
> ===================================================================
> --- dtc.orig/libfdt/fdt_ro.c	2008-08-29 14:11:11.000000000 +1000
> +++ dtc/libfdt/fdt_ro.c	2008-08-29 14:11:11.000000000 +1000
> @@ -328,9 +328,6 @@ int fdt_get_path(const void *fdt, int no
>  	for (offset = 0, depth = 0;
>  	     (offset >= 0) && (offset <= nodeoffset);
>  	     offset = fdt_next_node(fdt, offset, &depth)) {
> -		if (pdepth < depth)
> -			continue; /* overflowed buffer */
> -
>  		while (pdepth > depth) {
>  			do {
>  				p--;
> @@ -338,14 +335,16 @@ int fdt_get_path(const void *fdt, int no
>  			pdepth--;
>  		}
>  
> -		name = fdt_get_name(fdt, offset, &namelen);
> -		if (!name)
> -			return namelen;
> -		if ((p + namelen + 1) <= buflen) {
> -			memcpy(buf + p, name, namelen);
> -			p += namelen;
> -			buf[p++] = '/';
> -			pdepth++;
> +		if (pdepth >= depth) {
> +			name = fdt_get_name(fdt, offset, &namelen);
> +			if (!name)
> +				return namelen;
> +			if ((p + namelen + 1) <= buflen) {
> +				memcpy(buf + p, name, namelen);
> +				p += namelen;
> +				buf[p++] = '/';
> +				pdepth++;
> +			}
>  		}
>  
>  		if (offset == nodeoffset) {
> @@ -355,7 +354,7 @@ int fdt_get_path(const void *fdt, int no
>  			if (p > 1) /* special case so that root path is "/", not "" */
>  				p--;
>  			buf[p] = '\0';
> -			return p;
> +			return 0;
>  		}
>  	}
>  
> 

[U-Boot] libfdt: Fix bugs in fdt_get_path()

[David Gibson]

On Sat, Aug 30, 2008 at 12:13:11PM -0400, Jerry Van Baren wrote:
> FYI, I propose to pick this up for the *NEXT* release of u-boot ("Yellow  
> Submarine" ;-).  My reasoning is that (1) the current window closes  
> today, which makes me uncomfortable WRT putting in changes today and (2)  
> this appears to be a latent bug (at worst, a misleading error message);  
> I have not seen evidence that it is causing problems at the moment.

Yeah, that seems reasonable.  It's only an incorrect-error-code bug,
although I'd class it a relatively important one within that class,
since the distinction between NOSPACE and other errors typically does
matter.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: libfdt: Fix bugs in fdt_get_path()

[Jon Loeliger]

> The current implementation of fdt_get_path() has a couple of bugs,
> fixed by this patch.
> 
> First, contrary to its documentation, on success it returns the length
> of the node's path, rather than 0.  The testcase is correspondingly
> wrong, and the patch fixes this as well.
> 
> Second, in some circumstances, it will return -FDT_ERR_BADOFFSET
> instead of -FDT_ERR_NOSPACE when given insufficient buffer space.
> Specifically this happens when there is insufficient space even to
> hold the path's second last component.  This behaviour is corrected,
> and the testcase updated to check it.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied.

Thanks,
jdl

Re: libfdt: Fix bugs in fdt_get_path()

[Jon Loeliger]

> The current implementation of fdt_get_path() has a couple of bugs,
> fixed by this patch.
> 
> First, contrary to its documentation, on success it returns the length
> of the node's path, rather than 0.  The testcase is correspondingly
> wrong, and the patch fixes this as well.
> 
> Second, in some circumstances, it will return -FDT_ERR_BADOFFSET
> instead of -FDT_ERR_NOSPACE when given insufficient buffer space.
> Specifically this happens when there is insufficient space even to
> hold the path's second last component.  This behaviour is corrected,
> and the testcase updated to check it.
> 
> Signed-off-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

Applied.

Thanks,
jdl