[PATCH] libfdt: Correct signed/unsigned comparisons

[PATCH] libfdt: Correct signed/unsigned comparisons

[Simon Glass]

These warnings appear when building U-Boot on x86 and some other targets.
Correct them by adding casts.

Example:

scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
   if ((absoffset < offset)

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

 libfdt/fdt.c          |  4 ++--
 libfdt/fdt_overlay.c  |  2 +-
 libfdt/fdt_ro.c       | 13 +++++++------
 libfdt/fdt_strerror.c |  2 +-
 libfdt/fdt_sw.c       |  9 +++++----
 libfdt/fdt_wip.c      |  2 +-
 6 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index d6ce7c0..8b29dbd 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -116,7 +116,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
 
 	if ((absoffset < offset)
-	    || ((absoffset + len) < absoffset)
+	    || ((absoffset + len) < (unsigned int)absoffset)
 	    || (absoffset + len) > fdt_totalsize(fdt))
 		return NULL;
 
@@ -283,7 +283,7 @@ int fdt_move(const void *fdt, void *buf, int bufsize)
 {
 	FDT_RO_PROBE(fdt);
 
-	if (fdt_totalsize(fdt) > bufsize)
+	if (fdt_totalsize(fdt) > (unsigned int)bufsize)
 		return -FDT_ERR_NOSPACE;
 
 	memmove(buf, fdt, fdt_totalsize(fdt));
diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index b310e49..dba4f52 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -250,7 +250,7 @@ static int overlay_update_local_node_references(void *fdto,
 			return tree_len;
 		}
 
-		for (i = 0; i < (fixup_len / sizeof(uint32_t)); i++) {
+		for (i = 0; i < (int)(fixup_len / sizeof(uint32_t)); i++) {
 			fdt32_t adj_val;
 			uint32_t poffset;
 
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index a5c2797..e4c90d4 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -44,7 +44,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 		goto fail;
 
 	err = -FDT_ERR_BADOFFSET;
-	if (absoffset >= totalsize)
+	if (absoffset >= (unsigned int)totalsize)
 		goto fail;
 	len = totalsize - absoffset;
 
@@ -52,14 +52,14 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 		if (stroffset < 0)
 			goto fail;
 		if (fdt_version(fdt) >= 17) {
-			if (stroffset >= fdt_size_dt_strings(fdt))
+			if ((unsigned int)stroffset >= fdt_size_dt_strings(fdt))
 				goto fail;
 			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
 				len = fdt_size_dt_strings(fdt) - stroffset;
 		}
 	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
 		if ((stroffset >= 0)
-		    || (stroffset < -fdt_size_dt_strings(fdt)))
+		    || ((unsigned int)stroffset < -fdt_size_dt_strings(fdt)))
 			goto fail;
 		if ((-stroffset) < len)
 			len = -stroffset;
@@ -151,9 +151,10 @@ static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
 	int offset = n * sizeof(struct fdt_reserve_entry);
 	int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
 
-	if (absoffset < fdt_off_mem_rsvmap(fdt))
+	if ((unsigned int)absoffset < fdt_off_mem_rsvmap(fdt))
 		return NULL;
-	if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
+	if ((unsigned int)absoffset > fdt_totalsize(fdt) -
+	    sizeof(struct fdt_reserve_entry))
 		return NULL;
 	return fdt_mem_rsv_(fdt, n);
 }
@@ -658,7 +659,7 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle)
 {
 	int offset;
 
-	if ((phandle == 0) || (phandle == -1))
+	if ((phandle == 0) || (phandle == (uint32_t)-1))
 		return -FDT_ERR_BADPHANDLE;
 
 	FDT_RO_PROBE(fdt);
diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
index 768db66..c02c6ef 100644
--- a/libfdt/fdt_strerror.c
+++ b/libfdt/fdt_strerror.c
@@ -48,7 +48,7 @@ const char *fdt_strerror(int errval)
 		return "<valid offset/length>";
 	else if (errval == 0)
 		return "<no error>";
-	else if (errval > -FDT_ERRTABSIZE) {
+	else if (errval > (int)-FDT_ERRTABSIZE) {
 		const char *s = fdt_errtable[-errval].str;
 
 		if (s)
diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 76bea22..e37d785 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -95,7 +95,8 @@ static void *fdt_grab_space_(void *fdt, size_t len)
 	spaceleft = fdt_totalsize(fdt) - fdt_off_dt_struct(fdt)
 		- fdt_size_dt_strings(fdt);
 
-	if ((offset + len < offset) || (offset + len > spaceleft))
+	if ((offset + len < (size_t)offset) ||
+	    (offset + len > (size_t)spaceleft))
 		return NULL;
 
 	fdt_set_size_dt_struct(fdt, offset + len);
@@ -108,7 +109,7 @@ int fdt_create_with_flags(void *buf, int bufsize, uint32_t flags)
 					 sizeof(struct fdt_reserve_entry));
 	void *fdt = buf;
 
-	if (bufsize < hdrsize)
+	if ((unsigned int)bufsize < hdrsize)
 		return -FDT_ERR_NOSPACE;
 
 	if (flags & ~FDT_CREATE_FLAGS_ALL)
@@ -154,7 +155,7 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
 	if ((headsize + tailsize) > fdt_totalsize(fdt))
 		return -FDT_ERR_INTERNAL;
 
-	if ((headsize + tailsize) > bufsize)
+	if ((headsize + tailsize) > (size_t)bufsize)
 		return -FDT_ERR_NOSPACE;
 
 	oldtail = (char *)fdt + fdt_totalsize(fdt) - tailsize;
@@ -248,7 +249,7 @@ static int fdt_add_string_(void *fdt, const char *s)
 
 	offset = -strtabsize - len;
 	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
-	if (fdt_totalsize(fdt) + offset < struct_top)
+	if (fdt_totalsize(fdt) + offset < (unsigned int)struct_top)
 		return 0; /* no more room :( */
 
 	memcpy(strtab + offset, s, len);
diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
index f64139e..82db674 100644
--- a/libfdt/fdt_wip.c
+++ b/libfdt/fdt_wip.c
@@ -23,7 +23,7 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
 	if (!propval)
 		return proplen;
 
-	if (proplen < (len + idx))
+	if ((unsigned int)proplen < (len + idx))
 		return -FDT_ERR_NOSPACE;
 
 	memcpy((char *)propval + idx, val, len);
-- 
2.25.0.rc1.283.g88dfdc4193-goog

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[David Gibson]

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

On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> These warnings appear when building U-Boot on x86 and some other targets.
> Correct them by adding casts.
> 
> Example:
> 
> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
>    if ((absoffset < offset)
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Hmm.  So squashing warnings is certainly a good thing in general.

Unfortunately, I'm really uncomfortable with most of these changes.
In a number of cases they are outright wrong.  In most of the others,
the code was already correct.  I dislike adding casts to suppress
spurious warnings on correct code because that can end up hiding real
problems which might be introduced by future changes.

Case by case details below.

> ---
> 
>  libfdt/fdt.c          |  4 ++--
>  libfdt/fdt_overlay.c  |  2 +-
>  libfdt/fdt_ro.c       | 13 +++++++------
>  libfdt/fdt_strerror.c |  2 +-
>  libfdt/fdt_sw.c       |  9 +++++----
>  libfdt/fdt_wip.c      |  2 +-
>  6 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index d6ce7c0..8b29dbd 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -116,7 +116,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
>  
>  	if ((absoffset < offset)
> -	    || ((absoffset + len) < absoffset)
> +	    || ((absoffset + len) < (unsigned int)absoffset)

I'm baffled by this.  Both absoffset and len are defined as unsigned,
so how is a signed comparison appearing here?

>  	    || (absoffset + len) > fdt_totalsize(fdt))
>  		return NULL;
>  
> @@ -283,7 +283,7 @@ int fdt_move(const void *fdt, void *buf, int bufsize)
>  {
>  	FDT_RO_PROBE(fdt);
>  
> -	if (fdt_totalsize(fdt) > bufsize)
> +	if (fdt_totalsize(fdt) > (unsigned int)bufsize)

I don't like this change.  The comparison is correct as it stands,
even though the types are different: fdt_totalsize() > INT_MAX (which
should always result in a false comparison) is an error in the dtb,
and that's checked by fdt_ro_probe_().

Worse, this means that passing in -1 to what is a signed int parameter
will now make the test *pass*, which really doesn't seem right.

>  		return -FDT_ERR_NOSPACE;
>  
>  	memmove(buf, fdt, fdt_totalsize(fdt));
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index b310e49..dba4f52 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -250,7 +250,7 @@ static int overlay_update_local_node_references(void *fdto,
>  			return tree_len;
>  		}
>  
> -		for (i = 0; i < (fixup_len / sizeof(uint32_t)); i++) {
> +		for (i = 0; i < (int)(fixup_len / sizeof(uint32_t)); i++) {

Hrm.  Not sure how to handle that.  The explicit cast is ugly (and
casts in general make things more fragile), but there is a real type
difference.  It's pretty easy to see that this usage is safe, since an
int divided by sizeof(uint32_t) is clearly within the range of an int.

>  			fdt32_t adj_val;
>  			uint32_t poffset;
>  
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index a5c2797..e4c90d4 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -44,7 +44,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  		goto fail;
>  
>  	err = -FDT_ERR_BADOFFSET;
> -	if (absoffset >= totalsize)
> +	if (absoffset >= (unsigned int)totalsize)
>  		goto fail;

Again, this test is correct, despite the difference in types.  This
time the change is safe, because we've just verified that totalsize >=
0, but assuming here still makes the code a bit more fragile against
rearrangements.

>  	len = totalsize - absoffset;
>  
> @@ -52,14 +52,14 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  		if (stroffset < 0)
>  			goto fail;
>  		if (fdt_version(fdt) >= 17) {
> -			if (stroffset >= fdt_size_dt_strings(fdt))
> +			if ((unsigned int)stroffset >= fdt_size_dt_strings(fdt))

Again, the test is correct despite the type difference.  We've checked
for the stroffset < 0 case a few lines above.

>  				goto fail;
>  			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
>  				len = fdt_size_dt_strings(fdt) - stroffset;
>  		}
>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
>  		if ((stroffset >= 0)
> -		    || (stroffset < -fdt_size_dt_strings(fdt)))
> +		    || ((unsigned int)stroffset < -fdt_size_dt_strings(fdt)))

This is 100% wrong.  In the SW case stroffset *will* be negative
(we've just checked against that above), and forcing it to an unsigned
will give us the wrong result.

>  			goto fail;
>  		if ((-stroffset) < len)
>  			len = -stroffset;
> @@ -151,9 +151,10 @@ static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
>  	int offset = n * sizeof(struct fdt_reserve_entry);
>  	int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
>  
> -	if (absoffset < fdt_off_mem_rsvmap(fdt))
> +	if ((unsigned int)absoffset < fdt_off_mem_rsvmap(fdt))
>  		return NULL;

I think there's a real bug here, but rather than casting it would be
preferable to change the type of absoffset and offset to unsigned.

> -	if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
> +	if ((unsigned int)absoffset > fdt_totalsize(fdt) -
> +	    sizeof(struct fdt_reserve_entry))

Same here.

>  		return NULL;
>  	return fdt_mem_rsv_(fdt, n);
>  }
> @@ -658,7 +659,7 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle)
>  {
>  	int offset;
>  
> -	if ((phandle == 0) || (phandle == -1))
> +	if ((phandle == 0) || (phandle == (uint32_t)-1))

This change is ok.

>  		return -FDT_ERR_BADPHANDLE;
>  
>  	FDT_RO_PROBE(fdt);
> diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
> index 768db66..c02c6ef 100644
> --- a/libfdt/fdt_strerror.c
> +++ b/libfdt/fdt_strerror.c
> @@ -48,7 +48,7 @@ const char *fdt_strerror(int errval)
>  		return "<valid offset/length>";
>  	else if (errval == 0)
>  		return "<no error>";
> -	else if (errval > -FDT_ERRTABSIZE) {
> +	else if (errval > (int)-FDT_ERRTABSIZE) {

This one confuses me as well.  If the unary - on the RHS was being
interpreted unsigned, I can't see how this could have ever worked.
But I have gotten meaningful results from fdt_strerror(), so that
doesn't seem right.

But if FDT_ERRTABSIZE is being coerced to signed before applying the
unary -, then I don't see why there's any cause to warn.

>  		const char *s = fdt_errtable[-errval].str;
>  
>  		if (s)
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 76bea22..e37d785 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -95,7 +95,8 @@ static void *fdt_grab_space_(void *fdt, size_t len)
>  	spaceleft = fdt_totalsize(fdt) - fdt_off_dt_struct(fdt)
>  		- fdt_size_dt_strings(fdt);
>  
> -	if ((offset + len < offset) || (offset + len > spaceleft))
> +	if ((offset + len < (size_t)offset) ||
> +	    (offset + len > (size_t)spaceleft))
>  		return NULL;

Looks like changing the type of offset would be preferable.

>  
>  	fdt_set_size_dt_struct(fdt, offset + len);
> @@ -108,7 +109,7 @@ int fdt_create_with_flags(void *buf, int bufsize, uint32_t flags)
>  					 sizeof(struct fdt_reserve_entry));
>  	void *fdt = buf;
>  
> -	if (bufsize < hdrsize)
> +	if ((unsigned int)bufsize < hdrsize)
>  		return -FDT_ERR_NOSPACE;

As with some of the earlier things, the existing comparison is
correct, and the changed one is actively dangerous if the caller
passes a negative bufsize.

>  
>  	if (flags & ~FDT_CREATE_FLAGS_ALL)
> @@ -154,7 +155,7 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
>  	if ((headsize + tailsize) > fdt_totalsize(fdt))
>  		return -FDT_ERR_INTERNAL;
>  
> -	if ((headsize + tailsize) > bufsize)
> +	if ((headsize + tailsize) > (size_t)bufsize)

Ditto.

>  		return -FDT_ERR_NOSPACE;
>  
>  	oldtail = (char *)fdt + fdt_totalsize(fdt) - tailsize;
> @@ -248,7 +249,7 @@ static int fdt_add_string_(void *fdt, const char *s)
>  
>  	offset = -strtabsize - len;
>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
> -	if (fdt_totalsize(fdt) + offset < struct_top)
> +	if (fdt_totalsize(fdt) + offset < (unsigned int)struct_top)
>  		return 0; /* no more room :( */

struct_top is generated as the sum of two unsigneds, so changing its
type would be preferable.

>  	memcpy(strtab + offset, s, len);
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index f64139e..82db674 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -23,7 +23,7 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
>  	if (!propval)
>  		return proplen;
>  
> -	if (proplen < (len + idx))
> +	if ((unsigned int)proplen < (len + idx))

Oof. this is a bit nasty.  We're not checking for overflow of len +
idx at all, which is a bigger issue than the signedness.

>  		return -FDT_ERR_NOSPACE;
>  
>  	memcpy((char *)propval + idx, val, len);

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[Simon Glass]

Hi David,

On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> > These warnings appear when building U-Boot on x86 and some other targets.
> > Correct them by adding casts.
> >
> > Example:
> >
> > scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> > scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >    if ((absoffset < offset)
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>
> Hmm.  So squashing warnings is certainly a good thing in general.
>
> Unfortunately, I'm really uncomfortable with most of these changes.
> In a number of cases they are outright wrong.  In most of the others,
> the code was already correct.  I dislike adding casts to suppress
> spurious warnings on correct code because that can end up hiding real
> problems which might be introduced by future changes.
>
> Case by case details below.

Thanks for the review. I agree this is all really horrible,
particularly in light of the fact that it doesn't fix bugs and perhaps
introduces some.

This was just a quick patch to silence the warnings. If we do make
fixes here we should really make sure that there are test cases to
trigger each check. I suspect we have some but not all.

What do you think we should do? The warnings are going to be very
annoying for people. I could perhaps split the patch up and work
through things one by one.

Regards,
Simon

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[David Gibson]

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

On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> Hi David,
> 
> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> > > These warnings appear when building U-Boot on x86 and some other targets.
> > > Correct them by adding casts.
> > >
> > > Example:
> > >
> > > scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> > > scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> > >    if ((absoffset < offset)
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >
> > Hmm.  So squashing warnings is certainly a good thing in general.
> >
> > Unfortunately, I'm really uncomfortable with most of these changes.
> > In a number of cases they are outright wrong.  In most of the others,
> > the code was already correct.  I dislike adding casts to suppress
> > spurious warnings on correct code because that can end up hiding real
> > problems which might be introduced by future changes.
> >
> > Case by case details below.
> 
> Thanks for the review. I agree this is all really horrible,
> particularly in light of the fact that it doesn't fix bugs and perhaps
> introduces some.
> 
> This was just a quick patch to silence the warnings. If we do make
> fixes here we should really make sure that there are test cases to
> trigger each check. I suspect we have some but not all.

Yeah, adding some safety test cases for egregiously bad input like
negative buffer sizes is probably a good idea.

> What do you think we should do? The warnings are going to be very
> annoying for people. I could perhaps split the patch up and work
> through things one by one.

Yeah, we want to find some way to remove the warnings, and I think
splitting up and working piece by piece will be necessary.

I think the very first step, is to find definitive info on what
exactly the defined behaviour of C is with a signed vs. unsigned
comparison.

The help text of -Wsign-compare seems to imply that assuming:

	signed int a;
	unsigned int b;

then 
	if (a < b) ...

is equivalent to
	if ((unsigned int)a < b) ...

But I thought that this was not the case.  Rather, I thought it was
supposed to always evaluate to true if b > INT_MAX.  We need to know
which is the case as a starting point.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[André Przywara]

On 17/01/2020 09:23, David Gibson wrote:

Hi,

> On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
>> Hi David,
>>
>> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>>>
>>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
>>>> These warnings appear when building U-Boot on x86 and some other targets.
>>>> Correct them by adding casts.
>>>>
>>>> Example:
>>>>
>>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
>>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
>>>>    if ((absoffset < offset)
>>>>
>>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>>
>>> Hmm.  So squashing warnings is certainly a good thing in general.
>>>
>>> Unfortunately, I'm really uncomfortable with most of these changes.
>>> In a number of cases they are outright wrong.  In most of the others,
>>> the code was already correct.  I dislike adding casts to suppress
>>> spurious warnings on correct code because that can end up hiding real
>>> problems which might be introduced by future changes.
>>>
>>> Case by case details below.
>>
>> Thanks for the review. I agree this is all really horrible,
>> particularly in light of the fact that it doesn't fix bugs and perhaps
>> introduces some.
>>
>> This was just a quick patch to silence the warnings. If we do make
>> fixes here we should really make sure that there are test cases to
>> trigger each check. I suspect we have some but not all.
> 
> Yeah, adding some safety test cases for egregiously bad input like
> negative buffer sizes is probably a good idea.
> 
>> What do you think we should do? The warnings are going to be very
>> annoying for people. I could perhaps split the patch up and work
>> through things one by one.
> 
> Yeah, we want to find some way to remove the warnings, and I think
> splitting up and working piece by piece will be necessary.

Has anyone done anything on that front?
If not, I would take a deep breath and try to tackle this one by one. I
was grudgingly ignoring this in U-Boot so far, but it popped up in
Trusted Firmware now as well, so I have a business reason (TM).

Cheers,
Andre

> I think the very first step, is to find definitive info on what
> exactly the defined behaviour of C is with a signed vs. unsigned
> comparison.
> 
> The help text of -Wsign-compare seems to imply that assuming:
> 
> 	signed int a;
> 	unsigned int b;
> 
> then 
> 	if (a < b) ...
> 
> is equivalent to
> 	if ((unsigned int)a < b) ...
> 
> But I thought that this was not the case.  Rather, I thought it was
> supposed to always evaluate to true if b > INT_MAX.  We need to know
> which is the case as a starting point.
> 

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[Simon Glass]

Hi André,

On Thu, 17 Sep 2020 at 04:27, André Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On 17/01/2020 09:23, David Gibson wrote:
>
> Hi,
>
> > On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> >> Hi David,
> >>
> >> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >>>
> >>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> >>>> These warnings appear when building U-Boot on x86 and some other targets.
> >>>> Correct them by adding casts.
> >>>>
> >>>> Example:
> >>>>
> >>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> >>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >>>>    if ((absoffset < offset)
> >>>>
> >>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>>
> >>> Hmm.  So squashing warnings is certainly a good thing in general.
> >>>
> >>> Unfortunately, I'm really uncomfortable with most of these changes.
> >>> In a number of cases they are outright wrong.  In most of the others,
> >>> the code was already correct.  I dislike adding casts to suppress
> >>> spurious warnings on correct code because that can end up hiding real
> >>> problems which might be introduced by future changes.
> >>>
> >>> Case by case details below.
> >>
> >> Thanks for the review. I agree this is all really horrible,
> >> particularly in light of the fact that it doesn't fix bugs and perhaps
> >> introduces some.
> >>
> >> This was just a quick patch to silence the warnings. If we do make
> >> fixes here we should really make sure that there are test cases to
> >> trigger each check. I suspect we have some but not all.
> >
> > Yeah, adding some safety test cases for egregiously bad input like
> > negative buffer sizes is probably a good idea.
> >
> >> What do you think we should do? The warnings are going to be very
> >> annoying for people. I could perhaps split the patch up and work
> >> through things one by one.
> >
> > Yeah, we want to find some way to remove the warnings, and I think
> > splitting up and working piece by piece will be necessary.
>
> Has anyone done anything on that front?
> If not, I would take a deep breath and try to tackle this one by one. I
> was grudgingly ignoring this in U-Boot so far, but it popped up in
> Trusted Firmware now as well, so I have a business reason (TM).

No, I never got back to it. It would be great if you could take it on!

Regards,
Simon

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[David Gibson]

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

On Thu, Sep 17, 2020 at 11:26:44AM +0100, André Przywara wrote:
> On 17/01/2020 09:23, David Gibson wrote:
> 
> Hi,
> 
> > On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> >> Hi David,
> >>
> >> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> >>>
> >>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> >>>> These warnings appear when building U-Boot on x86 and some other targets.
> >>>> Correct them by adding casts.
> >>>>
> >>>> Example:
> >>>>
> >>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> >>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >>>>    if ((absoffset < offset)
> >>>>
> >>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>>
> >>> Hmm.  So squashing warnings is certainly a good thing in general.
> >>>
> >>> Unfortunately, I'm really uncomfortable with most of these changes.
> >>> In a number of cases they are outright wrong.  In most of the others,
> >>> the code was already correct.  I dislike adding casts to suppress
> >>> spurious warnings on correct code because that can end up hiding real
> >>> problems which might be introduced by future changes.
> >>>
> >>> Case by case details below.
> >>
> >> Thanks for the review. I agree this is all really horrible,
> >> particularly in light of the fact that it doesn't fix bugs and perhaps
> >> introduces some.
> >>
> >> This was just a quick patch to silence the warnings. If we do make
> >> fixes here we should really make sure that there are test cases to
> >> trigger each check. I suspect we have some but not all.
> > 
> > Yeah, adding some safety test cases for egregiously bad input like
> > negative buffer sizes is probably a good idea.
> > 
> >> What do you think we should do? The warnings are going to be very
> >> annoying for people. I could perhaps split the patch up and work
> >> through things one by one.
> > 
> > Yeah, we want to find some way to remove the warnings, and I think
> > splitting up and working piece by piece will be necessary.
> 
> Has anyone done anything on that front?
> If not, I would take a deep breath and try to tackle this one by one. I
> was grudgingly ignoring this in U-Boot so far, but it popped up in
> Trusted Firmware now as well, so I have a business reason (TM).

> > I think the very first step, is to find definitive info on what
> > exactly the defined behaviour of C is with a signed vs. unsigned
> > comparison.
> > 
> > The help text of -Wsign-compare seems to imply that assuming:
> > 
> > 	signed int a;
> > 	unsigned int b;
> > 
> > then 
> > 	if (a < b) ...
> > 
> > is equivalent to
> > 	if ((unsigned int)a < b) ...
> > 
> > But I thought that this was not the case.  Rather, I thought it was
> > supposed to always evaluate to true if b > INT_MAX.  We need to know
> > which is the case as a starting point.

I since had a brief look at this, and it appears I was wrong about
this behaviour, which makes this whole project rather more urgent.
I'd still appreciate someone looking at the standards to double check,
though.

I'm unlikely to have time to look at this myself, though, so I'd be
very happy if you took this on, and I'll do my absolute best to review
and merge promptly.

Fwiw, the more "bite-size" the patches are, the easier it is for me to
review.  So I'd suggest fixing just a small set of related cases at a
time, with a clear justification for why the new semantics are correct
in the commit message.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[André Przywara]

On 21/09/2020 07:00, David Gibson wrote:

Hi David,

> On Thu, Sep 17, 2020 at 11:26:44AM +0100, André Przywara wrote:
>> On 17/01/2020 09:23, David Gibson wrote:
>>
>> Hi,
>>
>>> On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
>>>> Hi David,
>>>>
>>>> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>>>>>
>>>>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
>>>>>> These warnings appear when building U-Boot on x86 and some other targets.
>>>>>> Correct them by adding casts.
>>>>>>
>>>>>> Example:
>>>>>>
>>>>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
>>>>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
>>>>>>    if ((absoffset < offset)
>>>>>>
>>>>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>>>>>
>>>>> Hmm.  So squashing warnings is certainly a good thing in general.
>>>>>
>>>>> Unfortunately, I'm really uncomfortable with most of these changes.
>>>>> In a number of cases they are outright wrong.  In most of the others,
>>>>> the code was already correct.  I dislike adding casts to suppress
>>>>> spurious warnings on correct code because that can end up hiding real
>>>>> problems which might be introduced by future changes.
>>>>>
>>>>> Case by case details below.
>>>>
>>>> Thanks for the review. I agree this is all really horrible,
>>>> particularly in light of the fact that it doesn't fix bugs and perhaps
>>>> introduces some.
>>>>
>>>> This was just a quick patch to silence the warnings. If we do make
>>>> fixes here we should really make sure that there are test cases to
>>>> trigger each check. I suspect we have some but not all.
>>>
>>> Yeah, adding some safety test cases for egregiously bad input like
>>> negative buffer sizes is probably a good idea.
>>>
>>>> What do you think we should do? The warnings are going to be very
>>>> annoying for people. I could perhaps split the patch up and work
>>>> through things one by one.
>>>
>>> Yeah, we want to find some way to remove the warnings, and I think
>>> splitting up and working piece by piece will be necessary.
>>
>> Has anyone done anything on that front?
>> If not, I would take a deep breath and try to tackle this one by one. I
>> was grudgingly ignoring this in U-Boot so far, but it popped up in
>> Trusted Firmware now as well, so I have a business reason (TM).
> 
>>> I think the very first step, is to find definitive info on what
>>> exactly the defined behaviour of C is with a signed vs. unsigned
>>> comparison.
>>>
>>> The help text of -Wsign-compare seems to imply that assuming:
>>>
>>> 	signed int a;
>>> 	unsigned int b;
>>>
>>> then 
>>> 	if (a < b) ...
>>>
>>> is equivalent to
>>> 	if ((unsigned int)a < b) ...
>>>
>>> But I thought that this was not the case.  Rather, I thought it was
>>> supposed to always evaluate to true if b > INT_MAX.  We need to know
>>> which is the case as a starting point.
> 
> I since had a brief look at this, and it appears I was wrong about
> this behaviour, which makes this whole project rather more urgent.
> I'd still appreciate someone looking at the standards to double check,
> though.
> 
> I'm unlikely to have time to look at this myself, though, so I'd be
> very happy if you took this on, and I'll do my absolute best to review
> and merge promptly.
> 
> Fwiw, the more "bite-size" the patches are, the easier it is for me to
> review.  So I'd suggest fixing just a small set of related cases at a
> time, with a clear justification for why the new semantics are correct
> in the commit message.

Yes, breaking this down (as suggested earlier) was the plan. Either by
group of functions or by type of fix used, not decided yet. And I will
try to add as much rationale to the commit messages as possible.

- I guess I can't change the external interface, to amend the signedness
in function parameters?

- Can we assume (or stipulate?) that a DTB is always smaller than 2GB?
The DT spec doesn't explicitly mention a limit, but the header uses
uint32_t for size fields. libfdt seems to assume the the actual
structure block is smaller than 2GB already (by using ints for node
offsets). So that leaves the corner case of totalsize being potentially
larger than 2GB only. Do we care about this?

Cheers,
Andre

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[Simon Glass]

Hi André,

On Mon, 21 Sep 2020 at 03:55, André Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>
> On 21/09/2020 07:00, David Gibson wrote:
>
> Hi David,
>
> > On Thu, Sep 17, 2020 at 11:26:44AM +0100, André Przywara wrote:
> >> On 17/01/2020 09:23, David Gibson wrote:
> >>
> >> Hi,
> >>
> >>> On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> >>>> Hi David,
> >>>>
> >>>> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6ryscVyMRj84@public.gmane.org.au> wrote:
> >>>>>
> >>>>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> >>>>>> These warnings appear when building U-Boot on x86 and some other targets.
> >>>>>> Correct them by adding casts.
> >>>>>>
> >>>>>> Example:
> >>>>>>
> >>>>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> >>>>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >>>>>>    if ((absoffset < offset)
> >>>>>>
> >>>>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>>>>
> >>>>> Hmm.  So squashing warnings is certainly a good thing in general.
> >>>>>
> >>>>> Unfortunately, I'm really uncomfortable with most of these changes.
> >>>>> In a number of cases they are outright wrong.  In most of the others,
> >>>>> the code was already correct.  I dislike adding casts to suppress
> >>>>> spurious warnings on correct code because that can end up hiding real
> >>>>> problems which might be introduced by future changes.
> >>>>>
> >>>>> Case by case details below.
> >>>>
> >>>> Thanks for the review. I agree this is all really horrible,
> >>>> particularly in light of the fact that it doesn't fix bugs and perhaps
> >>>> introduces some.
> >>>>
> >>>> This was just a quick patch to silence the warnings. If we do make
> >>>> fixes here we should really make sure that there are test cases to
> >>>> trigger each check. I suspect we have some but not all.
> >>>
> >>> Yeah, adding some safety test cases for egregiously bad input like
> >>> negative buffer sizes is probably a good idea.
> >>>
> >>>> What do you think we should do? The warnings are going to be very
> >>>> annoying for people. I could perhaps split the patch up and work
> >>>> through things one by one.
> >>>
> >>> Yeah, we want to find some way to remove the warnings, and I think
> >>> splitting up and working piece by piece will be necessary.
> >>
> >> Has anyone done anything on that front?
> >> If not, I would take a deep breath and try to tackle this one by one. I
> >> was grudgingly ignoring this in U-Boot so far, but it popped up in
> >> Trusted Firmware now as well, so I have a business reason (TM).
> >
> >>> I think the very first step, is to find definitive info on what
> >>> exactly the defined behaviour of C is with a signed vs. unsigned
> >>> comparison.
> >>>
> >>> The help text of -Wsign-compare seems to imply that assuming:
> >>>
> >>>     signed int a;
> >>>     unsigned int b;
> >>>
> >>> then
> >>>     if (a < b) ...
> >>>
> >>> is equivalent to
> >>>     if ((unsigned int)a < b) ...
> >>>
> >>> But I thought that this was not the case.  Rather, I thought it was
> >>> supposed to always evaluate to true if b > INT_MAX.  We need to know
> >>> which is the case as a starting point.
> >
> > I since had a brief look at this, and it appears I was wrong about
> > this behaviour, which makes this whole project rather more urgent.
> > I'd still appreciate someone looking at the standards to double check,
> > though.
> >
> > I'm unlikely to have time to look at this myself, though, so I'd be
> > very happy if you took this on, and I'll do my absolute best to review
> > and merge promptly.
> >
> > Fwiw, the more "bite-size" the patches are, the easier it is for me to
> > review.  So I'd suggest fixing just a small set of related cases at a
> > time, with a clear justification for why the new semantics are correct
> > in the commit message.
>
> Yes, breaking this down (as suggested earlier) was the plan. Either by
> group of functions or by type of fix used, not decided yet. And I will
> try to add as much rationale to the commit messages as possible.
>
> - I guess I can't change the external interface, to amend the signedness
> in function parameters?
>
> - Can we assume (or stipulate?) that a DTB is always smaller than 2GB?
> The DT spec doesn't explicitly mention a limit, but the header uses
> uint32_t for size fields. libfdt seems to assume the the actual
> structure block is smaller than 2GB already (by using ints for node
> offsets). So that leaves the corner case of totalsize being potentially
> larger than 2GB only. Do we care about this?

I feel that 2GB should be enough for everyone(TM). U-Boot supports
images being outside the FIT as well.

Regards,
Simon

Re: [PATCH] libfdt: Correct signed/unsigned comparisons

[David Gibson]

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

On Mon, Sep 21, 2020 at 10:54:32AM +0100, André Przywara wrote:
> On 21/09/2020 07:00, David Gibson wrote:
> 
> Hi David,
> 
> > On Thu, Sep 17, 2020 at 11:26:44AM +0100, André Przywara wrote:
> >> On 17/01/2020 09:23, David Gibson wrote:
> >>
> >> Hi,
> >>
> >>> On Thu, Jan 16, 2020 at 08:58:12PM +1300, Simon Glass wrote:
> >>>> Hi David,
> >>>>
> >>>> On Thu, 16 Jan 2020 at 19:50, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6ryscVyMRj84@public.gmane.org.au> wrote:
> >>>>>
> >>>>> On Sun, Jan 12, 2020 at 11:52:08AM -0700, Simon Glass wrote:
> >>>>>> These warnings appear when building U-Boot on x86 and some other targets.
> >>>>>> Correct them by adding casts.
> >>>>>>
> >>>>>> Example:
> >>>>>>
> >>>>>> scripts/dtc/libfdt/fdt.c: In function ‘fdt_offset_ptr’:
> >>>>>> scripts/dtc/libfdt/fdt.c:137:18: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
> >>>>>>    if ((absoffset < offset)
> >>>>>>
> >>>>>> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> >>>>>
> >>>>> Hmm.  So squashing warnings is certainly a good thing in general.
> >>>>>
> >>>>> Unfortunately, I'm really uncomfortable with most of these changes.
> >>>>> In a number of cases they are outright wrong.  In most of the others,
> >>>>> the code was already correct.  I dislike adding casts to suppress
> >>>>> spurious warnings on correct code because that can end up hiding real
> >>>>> problems which might be introduced by future changes.
> >>>>>
> >>>>> Case by case details below.
> >>>>
> >>>> Thanks for the review. I agree this is all really horrible,
> >>>> particularly in light of the fact that it doesn't fix bugs and perhaps
> >>>> introduces some.
> >>>>
> >>>> This was just a quick patch to silence the warnings. If we do make
> >>>> fixes here we should really make sure that there are test cases to
> >>>> trigger each check. I suspect we have some but not all.
> >>>
> >>> Yeah, adding some safety test cases for egregiously bad input like
> >>> negative buffer sizes is probably a good idea.
> >>>
> >>>> What do you think we should do? The warnings are going to be very
> >>>> annoying for people. I could perhaps split the patch up and work
> >>>> through things one by one.
> >>>
> >>> Yeah, we want to find some way to remove the warnings, and I think
> >>> splitting up and working piece by piece will be necessary.
> >>
> >> Has anyone done anything on that front?
> >> If not, I would take a deep breath and try to tackle this one by one. I
> >> was grudgingly ignoring this in U-Boot so far, but it popped up in
> >> Trusted Firmware now as well, so I have a business reason (TM).
> > 
> >>> I think the very first step, is to find definitive info on what
> >>> exactly the defined behaviour of C is with a signed vs. unsigned
> >>> comparison.
> >>>
> >>> The help text of -Wsign-compare seems to imply that assuming:
> >>>
> >>> 	signed int a;
> >>> 	unsigned int b;
> >>>
> >>> then 
> >>> 	if (a < b) ...
> >>>
> >>> is equivalent to
> >>> 	if ((unsigned int)a < b) ...
> >>>
> >>> But I thought that this was not the case.  Rather, I thought it was
> >>> supposed to always evaluate to true if b > INT_MAX.  We need to know
> >>> which is the case as a starting point.
> > 
> > I since had a brief look at this, and it appears I was wrong about
> > this behaviour, which makes this whole project rather more urgent.
> > I'd still appreciate someone looking at the standards to double check,
> > though.
> > 
> > I'm unlikely to have time to look at this myself, though, so I'd be
> > very happy if you took this on, and I'll do my absolute best to review
> > and merge promptly.
> > 
> > Fwiw, the more "bite-size" the patches are, the easier it is for me to
> > review.  So I'd suggest fixing just a small set of related cases at a
> > time, with a clear justification for why the new semantics are correct
> > in the commit message.
> 
> Yes, breaking this down (as suggested earlier) was the plan. Either by
> group of functions or by type of fix used, not decided yet. And I will
> try to add as much rationale to the commit messages as possible.

👍

> - I guess I can't change the external interface, to amend the signedness
> in function parameters?

On the external interface? No, I don't think that's worth the damage.

On internal functions, absolutely feel free to update the interface if
that's the cleanest approach.

> - Can we assume (or stipulate?) that a DTB is always smaller than 2GB?

Yes.  In fact, fdt_check_header() already verifies that.

> The DT spec doesn't explicitly mention a limit, but the header uses
> uint32_t for size fields. libfdt seems to assume the the actual
> structure block is smaller than 2GB already (by using ints for node
> offsets). So that leaves the corner case of totalsize being potentially
> larger than 2GB only. Do we care about this?

No, as above we already check that that totalsize < 2^31.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]