[PATCH 00/14] libfdt: Fix signed/unsigned comparison warnings

[PATCH 00/14] libfdt: Fix signed/unsigned comparison warnings

[Andre Przywara]

When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
some warnings about the signedness of the operands not matching:
=================
libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
   if ((absoffset < offset)
.....
=================

This does not occur under normal conditions in the dtc repo, but might
show up when libfdt is embedded in another project. There have been reports
from U-Boot and Trusted-Firmware-A.

The underlying issue is mostly due to C's promotion behaviour (ANSI C
section 6.1.3.8) when dealing with operands of different signedness
(but same size): Signed values get implictly casted to unsigned, which
is not typically what we want if they could have been negative.

The Internet(TM) suggests that blindly applying casts is probably doing
more harm than it helps, so this series tries to fix the underlying
issues properly.
In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
some signed types are due to them being returned along wih error values in
other functions (node offsets).
So these fixes here have been based on the following assumptions:
- We cannot change the prototype of exported functions.
- It's better to change types (for local variables) than to cast.
- If we have established that a signed value is not negative, we can safely
  cast it to an unsigned type.

I split up the fixes in small chunks, to make them easier to review.
The first four patches change types, the next six ones use casts after
we made sure the values are not negative.

This is only covering libfdt for now (which is what those other projects
care about). There are more issues with dtc, but they can be addressed
later.

Not sure if some of these checks should be gated by can_assume() calls.

Please have a look, happy to discuss the invididual cases.

Cheers,
Andre

Andre Przywara (13):
  libfdt: fdt_offset_ptr(): Fix comparison warnings
  libfdt: fdt_mem_rsv(): Fix comparison warnings
  libfdt: fdt_grab_space_(): Fix comparison warning
  libfdt: fdt_add_string_(): Fix comparison warning
  libfdt: fdt_move(): Fix comparison warnings
  libfdt: fdt_splice_(): Fix comparison warning
  libfdt: fdt_create_with_flags(): Fix comparison warning
  libfdt: fdt_resize(): Fix comparison warning
  libfdt: libfdt_wip: Fix comparison warning
  libfdt: overlay: Fix comparison warning
  libfdt: fdt_get_string(): Fix sequential write comparison warnings
  libfdt: fdt_node_offset_by_phandle(): Fix comparison warning
  libfdt: fdt_strerror(): Fix comparison warning

Simon Glass (1):
  libfdt: fdt_get_string(): Fix comparison warnings

 libfdt/fdt.c          | 15 +++++++++++----
 libfdt/fdt_overlay.c  |  3 ++-
 libfdt/fdt_ro.c       | 14 +++++++-------
 libfdt/fdt_rw.c       |  2 +-
 libfdt/fdt_strerror.c |  2 +-
 libfdt/fdt_sw.c       | 14 +++++++++-----
 libfdt/fdt_wip.c      |  4 ++--
 7 files changed, 33 insertions(+), 21 deletions(-)

-- 
2.17.5

[PATCH 01/14] libfdt: fdt_offset_ptr(): Fix comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about mismatching signedness in
comparisons in fdt_offset_ptr().

This mostly stems from "offset" being passed in as a signed integer,
even though the function would not really tolerate negative values.

Short of changing the prototype, check that offset is not negative, and
use an unsigned type internally.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 37b7b93..04e1e06 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -134,16 +134,20 @@ int fdt_check_header(const void *fdt)
 
 const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 {
-	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
+	unsigned int uoffset = offset;
+	unsigned int absoffset = offset + fdt_off_dt_struct(fdt);
+
+	if (offset < 0)
+		return NULL;
 
 	if (!can_assume(VALID_INPUT))
-		if ((absoffset < offset)
+		if ((absoffset < uoffset)
 		    || ((absoffset + len) < absoffset)
 		    || (absoffset + len) > fdt_totalsize(fdt))
 			return NULL;
 
 	if (can_assume(LATEST) || fdt_version(fdt) >= 0x11)
-		if (((offset + len) < offset)
+		if (((uoffset + len) < uoffset)
 		    || ((offset + len) > fdt_size_dt_struct(fdt)))
 			return NULL;
 
-- 
2.17.5

Re: [PATCH 01/14] libfdt: fdt_offset_ptr(): Fix comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:50PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about mismatching signedness in
> comparisons in fdt_offset_ptr().
> 
> This mostly stems from "offset" being passed in as a signed integer,
> even though the function would not really tolerate negative values.
> 
> Short of changing the prototype, check that offset is not negative, and
> use an unsigned type internally.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 37b7b93..04e1e06 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -134,16 +134,20 @@ int fdt_check_header(const void *fdt)
>  
>  const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  {
> -	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> +	unsigned int uoffset = offset;
> +	unsigned int absoffset = offset + fdt_off_dt_struct(fdt);
> +
> +	if (offset < 0)
> +		return NULL;
>  
>  	if (!can_assume(VALID_INPUT))
> -		if ((absoffset < offset)
> +		if ((absoffset < uoffset)
>  		    || ((absoffset + len) < absoffset)
>  		    || (absoffset + len) > fdt_totalsize(fdt))
>  			return NULL;
>  
>  	if (can_assume(LATEST) || fdt_version(fdt) >= 0x11)
> -		if (((offset + len) < offset)
> +		if (((uoffset + len) < uoffset)
>  		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>  			return NULL;
>  

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

[PATCH 02/14] libfdt: fdt_mem_rsv(): Fix comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in comparisons in fdt_mem_rsv().

Since all involved values must be positive, change the used types to be
unsigned.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_ro.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index e03570a..3c8d143 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -157,8 +157,8 @@ int fdt_generate_phandle(const void *fdt, uint32_t *phandle)
 
 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;
+	unsigned int offset = n * sizeof(struct fdt_reserve_entry);
+	unsigned int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
 
 	if (!can_assume(VALID_INPUT)) {
 		if (absoffset < fdt_off_mem_rsvmap(fdt))
-- 
2.17.5

Re: [PATCH 02/14] libfdt: fdt_mem_rsv(): Fix comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:51PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in comparisons in fdt_mem_rsv().
> 
> Since all involved values must be positive, change the used types to be
> unsigned.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_ro.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index e03570a..3c8d143 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -157,8 +157,8 @@ int fdt_generate_phandle(const void *fdt, uint32_t *phandle)
>  
>  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;
> +	unsigned int offset = n * sizeof(struct fdt_reserve_entry);
> +	unsigned int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
>  
>  	if (!can_assume(VALID_INPUT)) {
>  		if (absoffset < fdt_off_mem_rsvmap(fdt))

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

[PATCH 03/14] libfdt: fdt_grab_space_(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_grab_space_().

All the involved values cannot be negative, so let's switch the types of
the local variables to unsigned to make the compiler happy.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 94ce4bb..d10a720 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -93,8 +93,8 @@ static inline uint32_t sw_flags(void *fdt)
 
 static void *fdt_grab_space_(void *fdt, size_t len)
 {
-	int offset = fdt_size_dt_struct(fdt);
-	int spaceleft;
+	unsigned int offset = fdt_size_dt_struct(fdt);
+	unsigned int spaceleft;
 
 	spaceleft = fdt_totalsize(fdt) - fdt_off_dt_struct(fdt)
 		- fdt_size_dt_strings(fdt);
-- 
2.17.5

Re: [PATCH 03/14] libfdt: fdt_grab_space_(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:52PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_grab_space_().
> 
> All the involved values cannot be negative, so let's switch the types of
> the local variables to unsigned to make the compiler happy.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_sw.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 94ce4bb..d10a720 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -93,8 +93,8 @@ static inline uint32_t sw_flags(void *fdt)
>  
>  static void *fdt_grab_space_(void *fdt, size_t len)
>  {
> -	int offset = fdt_size_dt_struct(fdt);
> -	int spaceleft;
> +	unsigned int offset = fdt_size_dt_struct(fdt);
> +	unsigned int spaceleft;
>  
>  	spaceleft = fdt_totalsize(fdt) - fdt_off_dt_struct(fdt)
>  		- fdt_size_dt_strings(fdt);

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

[PATCH 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_add_string_().

As struct_top can only be positive, just use an unsigned type for it,
and avoid the signedness difference.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index d10a720..d65e9c8 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
 	char *strtab = (char *)fdt + fdt_totalsize(fdt);
 	int strtabsize = fdt_size_dt_strings(fdt);
 	int len = strlen(s) + 1;
-	int struct_top, offset;
+	unsigned int struct_top;
+	int offset;
 
 	offset = -strtabsize - len;
 	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
-- 
2.17.5

Re: [PATCH 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_add_string_().
> 
> As struct_top can only be positive, just use an unsigned type for it,
> and avoid the signedness difference.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

I'm not sure this is right.  Well.. I'm also not sure it was right
before.  Adding some more context to explain why..

> ---
>  libfdt/fdt_sw.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index d10a720..d65e9c8 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
>  	int strtabsize = fdt_size_dt_strings(fdt);
>  	int len = strlen(s) + 1;
> -	int struct_top, offset;
> +	unsigned int struct_top;
> +	int offset;
>  
>  	offset = -strtabsize - len;
>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>	if (fdt_totalsize(fdt) + offset < struct_top)
>		return 0; /* no more room :( */

So strtabsize and len will always be positive (or, if they're not,
that's another problem), so offset is always negative.  Which means we
need the signed addition between totalsize and offset for this to be
correct.

So I suspect we want to make 'len' and 'offset' unsigned as well,
reverse the sign on offset and make it a subtraction in the if instead
of an addition-of-negative.

We might then need to explicitly check for offset < totalsize as well,
to cover the overflow case.

-- 
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 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[André Przywara]

On 24/09/2020 02:01, David Gibson wrote:
> On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
>> With -Wsign-compare, compilers warn about a mismatching signedness
>> in a comparison in fdt_add_string_().
>>
>> As struct_top can only be positive, just use an unsigned type for it,
>> and avoid the signedness difference.
>>
>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> 
> I'm not sure this is right.  Well.. I'm also not sure it was right
> before.  Adding some more context to explain why..
> 
>> ---
>>  libfdt/fdt_sw.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
>> index d10a720..d65e9c8 100644
>> --- a/libfdt/fdt_sw.c
>> +++ b/libfdt/fdt_sw.c
>> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
>>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
>>  	int strtabsize = fdt_size_dt_strings(fdt);
>>  	int len = strlen(s) + 1;
>> -	int struct_top, offset;
>> +	unsigned int struct_top;
>> +	int offset;
>>  
>>  	offset = -strtabsize - len;
>>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>> 	if (fdt_totalsize(fdt) + offset < struct_top)
>> 		return 0; /* no more room :( */
> 
> So strtabsize and len will always be positive (or, if they're not,
> that's another problem), so offset is always negative.  Which means we
> need the signed addition between totalsize and offset for this to be
> correct.
> 
> So I suspect we want to make 'len' and 'offset' unsigned as well,
> reverse the sign on offset and make it a subtraction in the if instead
> of an addition-of-negative.

Ah, yes, much better! To be honest I just did my due diligence on *this*
function before, because all those minus signs confused me quite a lot.
But your approach makes it much clearer what is going on here.

So if I get this correctly, this function really returns the *negative*
offset of the new string, which will end up in the property, for now?
And at the end we translate all negative values into the actual offsets?
And there is initially a gap between the dt_struct part and the string
table, to allow both growing towards each other, without needing to
rewrite everything, every time?
Is this documented somewhere? Shall this approach be described in a
comment at the top of this file?

Thanks!
Andre.


> 
> We might then need to explicitly check for offset < totalsize as well,
> to cover the overflow case.
> 

Re: [PATCH 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[David Gibson]

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

On Thu, Oct 01, 2020 at 04:33:50PM +0100, André Przywara wrote:
> On 24/09/2020 02:01, David Gibson wrote:
> > On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
> >> With -Wsign-compare, compilers warn about a mismatching signedness
> >> in a comparison in fdt_add_string_().
> >>
> >> As struct_top can only be positive, just use an unsigned type for it,
> >> and avoid the signedness difference.
> >>
> >> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> > 
> > I'm not sure this is right.  Well.. I'm also not sure it was right
> > before.  Adding some more context to explain why..
> > 
> >> ---
> >>  libfdt/fdt_sw.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> >> index d10a720..d65e9c8 100644
> >> --- a/libfdt/fdt_sw.c
> >> +++ b/libfdt/fdt_sw.c
> >> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
> >>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
> >>  	int strtabsize = fdt_size_dt_strings(fdt);
> >>  	int len = strlen(s) + 1;
> >> -	int struct_top, offset;
> >> +	unsigned int struct_top;
> >> +	int offset;
> >>  
> >>  	offset = -strtabsize - len;
> >>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
> >> 	if (fdt_totalsize(fdt) + offset < struct_top)
> >> 		return 0; /* no more room :( */
> > 
> > So strtabsize and len will always be positive (or, if they're not,
> > that's another problem), so offset is always negative.  Which means we
> > need the signed addition between totalsize and offset for this to be
> > correct.
> > 
> > So I suspect we want to make 'len' and 'offset' unsigned as well,
> > reverse the sign on offset and make it a subtraction in the if instead
> > of an addition-of-negative.
> 
> Ah, yes, much better! To be honest I just did my due diligence on *this*
> function before, because all those minus signs confused me quite a lot.
> But your approach makes it much clearer what is going on here.
> 
> So if I get this correctly, this function really returns the *negative*
> offset of the new string, which will end up in the property, for now?
> And at the end we translate all negative values into the actual offsets?
> And there is initially a gap between the dt_struct part and the string
> table, to allow both growing towards each other, without needing to
> rewrite everything, every time?

Yes, that's correct.  During sequential write mode, to avoid excessive
memmove()s, the structure block grows up from the bottom of the
buffer, while the strings block grows down from the top.  Because the
"start" (lowest offset) of the strings block is moving, we can't use
offsets from that (without having to adjust all the offsets in the
structure block all the time).  So, we use negative offsets from the
"end" (highest offset).  I'm scare quoting "start" and "end" because
the header records the highest offset of the string block as its
"start", which is why the offsets are negative - that makes some of
the code paths more common with the normal dtb case.

> Is this documented somewhere? Shall this approach be described in a
> comment at the top of this file?

No, I don't think it is.  So, yes there probably should be a big
explanatory comment.  Patches welcome?

Note that this isn't supposed to be an externally visible format,
you're always supposed to use fdt_finish() to convert it into a
regular dtb before handing it off to any other piece of code.  We use
a different magic number in the header while we're in this state to
avoid anything mistaking this for a regular dtb.

-- 
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 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[André Przywara]

On 02/10/2020 01:01, David Gibson wrote:

Hi David,

> On Thu, Oct 01, 2020 at 04:33:50PM +0100, André Przywara wrote:
>> On 24/09/2020 02:01, David Gibson wrote:
>>> On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
>>>> With -Wsign-compare, compilers warn about a mismatching signedness
>>>> in a comparison in fdt_add_string_().
>>>>
>>>> As struct_top can only be positive, just use an unsigned type for it,
>>>> and avoid the signedness difference.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
>>>
>>> I'm not sure this is right.  Well.. I'm also not sure it was right
>>> before.  Adding some more context to explain why..
>>>
>>>> ---
>>>>  libfdt/fdt_sw.c | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
>>>> index d10a720..d65e9c8 100644
>>>> --- a/libfdt/fdt_sw.c
>>>> +++ b/libfdt/fdt_sw.c
>>>> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
>>>>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
>>>>  	int strtabsize = fdt_size_dt_strings(fdt);
>>>>  	int len = strlen(s) + 1;
>>>> -	int struct_top, offset;
>>>> +	unsigned int struct_top;
>>>> +	int offset;
>>>>  
>>>>  	offset = -strtabsize - len;
>>>>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>>>> 	if (fdt_totalsize(fdt) + offset < struct_top)
>>>> 		return 0; /* no more room :( */
>>>
>>> So strtabsize and len will always be positive (or, if they're not,
>>> that's another problem), so offset is always negative.  Which means we
>>> need the signed addition between totalsize and offset for this to be
>>> correct.
>>>
>>> So I suspect we want to make 'len' and 'offset' unsigned as well,
>>> reverse the sign on offset and make it a subtraction in the if instead
>>> of an addition-of-negative.
>>
>> Ah, yes, much better! To be honest I just did my due diligence on *this*
>> function before, because all those minus signs confused me quite a lot.
>> But your approach makes it much clearer what is going on here.
>>
>> So if I get this correctly, this function really returns the *negative*
>> offset of the new string, which will end up in the property, for now?
>> And at the end we translate all negative values into the actual offsets?
>> And there is initially a gap between the dt_struct part and the string
>> table, to allow both growing towards each other, without needing to
>> rewrite everything, every time?
> 
> Yes, that's correct.  During sequential write mode, to avoid excessive
> memmove()s, the structure block grows up from the bottom of the
> buffer, while the strings block grows down from the top.  Because the
> "start" (lowest offset) of the strings block is moving, we can't use
> offsets from that (without having to adjust all the offsets in the
> structure block all the time).  So, we use negative offsets from the
> "end" (highest offset).  I'm scare quoting "start" and "end" because
> the header records the highest offset of the string block as its
> "start", which is why the offsets are negative - that makes some of
> the code paths more common with the normal dtb case.
> 
>> Is this documented somewhere? Shall this approach be described in a
>> comment at the top of this file?
> 
> No, I don't think it is.  So, yes there probably should be a big
> explanatory comment.  Patches welcome?
Yes, that was the idea, just want to double check that I got this right.

> Note that this isn't supposed to be an externally visible format,
> you're always supposed to use fdt_finish() to convert it into a
> regular dtb before handing it off to any other piece of code.  We use
> a different magic number in the header while we're in this state to
> avoid anything mistaking this for a regular dtb.

Yeah, I understand, but the different magics and the negative offsets
were quite confusing at first, until I discovered the whole story.
Since the library is typically embedded into other projects, I think
it's a good idea to document this, in case people follow some rabbit
with ctags ...

Cheers,
Andre

Re: [PATCH 04/14] libfdt: fdt_add_string_(): Fix comparison warning

[David Gibson]

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

On Fri, Oct 02, 2020 at 10:31:06AM +0100, André Przywara wrote:
> On 02/10/2020 01:01, David Gibson wrote:
> 
> Hi David,
> 
> > On Thu, Oct 01, 2020 at 04:33:50PM +0100, André Przywara wrote:
> >> On 24/09/2020 02:01, David Gibson wrote:
> >>> On Mon, Sep 21, 2020 at 05:52:53PM +0100, Andre Przywara wrote:
> >>>> With -Wsign-compare, compilers warn about a mismatching signedness
> >>>> in a comparison in fdt_add_string_().
> >>>>
> >>>> As struct_top can only be positive, just use an unsigned type for it,
> >>>> and avoid the signedness difference.
> >>>>
> >>>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> >>>
> >>> I'm not sure this is right.  Well.. I'm also not sure it was right
> >>> before.  Adding some more context to explain why..
> >>>
> >>>> ---
> >>>>  libfdt/fdt_sw.c | 3 ++-
> >>>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> >>>> index d10a720..d65e9c8 100644
> >>>> --- a/libfdt/fdt_sw.c
> >>>> +++ b/libfdt/fdt_sw.c
> >>>> @@ -249,7 +249,8 @@ static int fdt_add_string_(void *fdt, const char *s)
> >>>>  	char *strtab = (char *)fdt + fdt_totalsize(fdt);
> >>>>  	int strtabsize = fdt_size_dt_strings(fdt);
> >>>>  	int len = strlen(s) + 1;
> >>>> -	int struct_top, offset;
> >>>> +	unsigned int struct_top;
> >>>> +	int offset;
> >>>>  
> >>>>  	offset = -strtabsize - len;
> >>>>  	struct_top = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
> >>>> 	if (fdt_totalsize(fdt) + offset < struct_top)
> >>>> 		return 0; /* no more room :( */
> >>>
> >>> So strtabsize and len will always be positive (or, if they're not,
> >>> that's another problem), so offset is always negative.  Which means we
> >>> need the signed addition between totalsize and offset for this to be
> >>> correct.
> >>>
> >>> So I suspect we want to make 'len' and 'offset' unsigned as well,
> >>> reverse the sign on offset and make it a subtraction in the if instead
> >>> of an addition-of-negative.
> >>
> >> Ah, yes, much better! To be honest I just did my due diligence on *this*
> >> function before, because all those minus signs confused me quite a lot.
> >> But your approach makes it much clearer what is going on here.
> >>
> >> So if I get this correctly, this function really returns the *negative*
> >> offset of the new string, which will end up in the property, for now?
> >> And at the end we translate all negative values into the actual offsets?
> >> And there is initially a gap between the dt_struct part and the string
> >> table, to allow both growing towards each other, without needing to
> >> rewrite everything, every time?
> > 
> > Yes, that's correct.  During sequential write mode, to avoid excessive
> > memmove()s, the structure block grows up from the bottom of the
> > buffer, while the strings block grows down from the top.  Because the
> > "start" (lowest offset) of the strings block is moving, we can't use
> > offsets from that (without having to adjust all the offsets in the
> > structure block all the time).  So, we use negative offsets from the
> > "end" (highest offset).  I'm scare quoting "start" and "end" because
> > the header records the highest offset of the string block as its
> > "start", which is why the offsets are negative - that makes some of
> > the code paths more common with the normal dtb case.
> > 
> >> Is this documented somewhere? Shall this approach be described in a
> >> comment at the top of this file?
> > 
> > No, I don't think it is.  So, yes there probably should be a big
> > explanatory comment.  Patches welcome?
> Yes, that was the idea, just want to double check that I got this right.
> 
> > Note that this isn't supposed to be an externally visible format,
> > you're always supposed to use fdt_finish() to convert it into a
> > regular dtb before handing it off to any other piece of code.  We use
> > a different magic number in the header while we're in this state to
> > avoid anything mistaking this for a regular dtb.
> 
> Yeah, I understand, but the different magics and the negative offsets
> were quite confusing at first, until I discovered the whole story.
> Since the library is typically embedded into other projects, I think
> it's a good idea to document this, in case people follow some rabbit
> with ctags ...

Yes, that makes sense.

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

[PATCH 05/14] libfdt: fdt_move(): Fix comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in comparisons in fdt_move().

This stems from "bufsize" being passed in as a signed integer, even
though we would expect a buffer size to be positive.

Short of changing the prototype, check that bufsize is not negative, and
cast it to an unsigned type in the comparison.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 04e1e06..43aaada 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -314,9 +314,12 @@ const char *fdt_find_string_(const char *strtab, int tabsize, const char *s)
 
 int fdt_move(const void *fdt, void *buf, int bufsize)
 {
+	if (bufsize < 0)
+		return -FDT_ERR_NOSPACE;
+
 	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));
-- 
2.17.5

Re: [PATCH 05/14] libfdt: fdt_move(): Fix comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:54PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in comparisons in fdt_move().
> 
> This stems from "bufsize" being passed in as a signed integer, even
> though we would expect a buffer size to be positive.
> 
> Short of changing the prototype, check that bufsize is not negative, and
> cast it to an unsigned type in the comparison.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Logic looks good, but the libfdt-in-tiny-bootloaders people would
probably appreciate a !can_assume(VALID_INPUT) test to elide the check
on bufsize.

> ---
>  libfdt/fdt.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 04e1e06..43aaada 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -314,9 +314,12 @@ const char *fdt_find_string_(const char *strtab, int tabsize, const char *s)
>  
>  int fdt_move(const void *fdt, void *buf, int bufsize)
>  {
> +	if (bufsize < 0)
> +		return -FDT_ERR_NOSPACE;
> +
>  	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));

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

[PATCH 06/14] libfdt: fdt_get_string(): Fix comparison warnings

[Andre Przywara]

From: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in fdt_get_string().

In the first two cases, we have just established that the signed values
are not negative, so it's safe to cast the values to an unsigned type.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_ro.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 3c8d143..ddb5a2d 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -53,7 +53,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 
 	err = -FDT_ERR_BADOFFSET;
 	absoffset = stroffset + fdt_off_dt_strings(fdt);
-	if (absoffset >= totalsize)
+	if (absoffset >= (unsigned)totalsize)
 		goto fail;
 	len = totalsize - absoffset;
 
@@ -61,7 +61,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 		if (stroffset < 0)
 			goto fail;
 		if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
-			if (stroffset >= fdt_size_dt_strings(fdt))
+			if ((unsigned)stroffset >= fdt_size_dt_strings(fdt))
 				goto fail;
 			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
 				len = fdt_size_dt_strings(fdt) - stroffset;
-- 
2.17.5

Re: [PATCH 06/14] libfdt: fdt_get_string(): Fix comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:55PM +0100, Andre Przywara wrote:
> From: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> 
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in fdt_get_string().
> 
> In the first two cases, we have just established that the signed values
> are not negative, so it's safe to cast the values to an unsigned type.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_ro.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 3c8d143..ddb5a2d 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -53,7 +53,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  
>  	err = -FDT_ERR_BADOFFSET;
>  	absoffset = stroffset + fdt_off_dt_strings(fdt);
> -	if (absoffset >= totalsize)
> +	if (absoffset >= (unsigned)totalsize)
>  		goto fail;
>  	len = totalsize - absoffset;
>  
> @@ -61,7 +61,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  		if (stroffset < 0)
>  			goto fail;
>  		if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
> -			if (stroffset >= fdt_size_dt_strings(fdt))
> +			if ((unsigned)stroffset >= fdt_size_dt_strings(fdt))
>  				goto fail;
>  			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
>  				len = fdt_size_dt_strings(fdt) - stroffset;

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

[PATCH 07/14] libfdt: fdt_splice_(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_splice_().

Since we just established that oldlen is not negative, we can safely
cast it to an unsigned type.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_rw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 93e4a2b..68887b9 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -59,7 +59,7 @@ static int fdt_splice_(void *fdt, void *splicepoint, int oldlen, int newlen)
 
 	if ((oldlen < 0) || (soff + oldlen < soff) || (soff + oldlen > dsize))
 		return -FDT_ERR_BADOFFSET;
-	if ((p < (char *)fdt) || (dsize + newlen < oldlen))
+	if ((p < (char *)fdt) || (dsize + newlen < (unsigned)oldlen))
 		return -FDT_ERR_BADOFFSET;
 	if (dsize - oldlen + newlen > fdt_totalsize(fdt))
 		return -FDT_ERR_NOSPACE;
-- 
2.17.5

Re: [PATCH 07/14] libfdt: fdt_splice_(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:56PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_splice_().
> 
> Since we just established that oldlen is not negative, we can safely
> cast it to an unsigned type.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_rw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 93e4a2b..68887b9 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -59,7 +59,7 @@ static int fdt_splice_(void *fdt, void *splicepoint, int oldlen, int newlen)
>  
>  	if ((oldlen < 0) || (soff + oldlen < soff) || (soff + oldlen > dsize))
>  		return -FDT_ERR_BADOFFSET;
> -	if ((p < (char *)fdt) || (dsize + newlen < oldlen))
> +	if ((p < (char *)fdt) || (dsize + newlen < (unsigned)oldlen))
>  		return -FDT_ERR_BADOFFSET;
>  	if (dsize - oldlen + newlen > fdt_totalsize(fdt))
>  		return -FDT_ERR_NOSPACE;

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

[PATCH 08/14] libfdt: fdt_create_with_flags(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_create_with_flags().

We have just established that bufsize is not negative, so a cast to an
unsigned type is safe.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index d65e9c8..8a76adf 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -112,7 +112,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 (bufsize < 0 || (unsigned)bufsize < hdrsize)
 		return -FDT_ERR_NOSPACE;
 
 	if (flags & ~FDT_CREATE_FLAGS_ALL)
-- 
2.17.5

Re: [PATCH 08/14] libfdt: fdt_create_with_flags(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:57PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_create_with_flags().
> 
> We have just established that bufsize is not negative, so a cast to an
> unsigned type is safe.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

This looks correct, but I think we might be able to do it a bit more
neatly by changing the type of hdrsize to signed instead.  It's a
small value, so we know that's a valid conversion, and that would let
your two tests be merged into one here.

> ---
>  libfdt/fdt_sw.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index d65e9c8..8a76adf 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -112,7 +112,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 (bufsize < 0 || (unsigned)bufsize < hdrsize)
>  		return -FDT_ERR_NOSPACE;
>  
>  	if (flags & ~FDT_CREATE_FLAGS_ALL)

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

[PATCH 09/14] libfdt: fdt_resize(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_resize().

A negative buffer size will surely do us no good, so let's rule this
case out first.
In the actual comparison we then know that a cast to an unsigned type is
safe.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_sw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 8a76adf..4c62922 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -152,6 +152,9 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
 
 	FDT_SW_PROBE(fdt);
 
+	if (bufsize < 0)
+		return -FDT_ERR_NOSPACE;
+
 	headsize = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
 	tailsize = fdt_size_dt_strings(fdt);
 
@@ -159,7 +162,7 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
 	    headsize + tailsize > fdt_totalsize(fdt))
 		return -FDT_ERR_INTERNAL;
 
-	if ((headsize + tailsize) > bufsize)
+	if ((headsize + tailsize) > (unsigned)bufsize)
 		return -FDT_ERR_NOSPACE;
 
 	oldtail = (char *)fdt + fdt_totalsize(fdt) - tailsize;
-- 
2.17.5

Re: [PATCH 09/14] libfdt: fdt_resize(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:58PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_resize().
> 
> A negative buffer size will surely do us no good, so let's rule this
> case out first.
> In the actual comparison we then know that a cast to an unsigned type is
> safe.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_sw.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 8a76adf..4c62922 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -152,6 +152,9 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
>  
>  	FDT_SW_PROBE(fdt);
>  
> +	if (bufsize < 0)
> +		return -FDT_ERR_NOSPACE;
> +
>  	headsize = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>  	tailsize = fdt_size_dt_strings(fdt);
>  
> @@ -159,7 +162,7 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
>  	    headsize + tailsize > fdt_totalsize(fdt))
>  		return -FDT_ERR_INTERNAL;
>  
> -	if ((headsize + tailsize) > bufsize)
> +	if ((headsize + tailsize) > (unsigned)bufsize)
>  		return -FDT_ERR_NOSPACE;
>  
>  	oldtail = (char *)fdt + fdt_totalsize(fdt) - tailsize;

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

[PATCH 10/14] libfdt: libfdt_wip: Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_setprop_inplace_namelen_partial().

Since fdt_getprop_namelen() can return negative error values, check for
those first and bail out early.
Later on we can then safely cast the value to an unsigned type.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_wip.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
index f64139e..d1b4f1b 100644
--- a/libfdt/fdt_wip.c
+++ b/libfdt/fdt_wip.c
@@ -20,10 +20,10 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
 
 	propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
 					&proplen);
-	if (!propval)
+	if (!propval || proplen < 0)
 		return proplen;
 
-	if (proplen < (len + idx))
+	if ((unsigned)proplen < (len + idx))
 		return -FDT_ERR_NOSPACE;
 
 	memcpy((char *)propval + idx, val, len);
-- 
2.17.5

Re: [PATCH 10/14] libfdt: libfdt_wip: Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:59PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_setprop_inplace_namelen_partial().
> 
> Since fdt_getprop_namelen() can return negative error values, check for
> those first and bail out early.
> Later on we can then safely cast the value to an unsigned type.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  libfdt/fdt_wip.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c
> index f64139e..d1b4f1b 100644
> --- a/libfdt/fdt_wip.c
> +++ b/libfdt/fdt_wip.c
> @@ -20,10 +20,10 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset,
>  
>  	propval = fdt_getprop_namelen_w(fdt, nodeoffset, name, namelen,
>  					&proplen);
> -	if (!propval)
> +	if (!propval || proplen < 0)
>  		return proplen;

We don't actually need that test - if proplen < 0 with propval
non-NULL, that's a bug in fdt_getprop_namelen_w().

> -	if (proplen < (len + idx))
> +	if ((unsigned)proplen < (len + idx))
>  		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 --]

[PATCH 11/14] libfdt: overlay: Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
a comparison in overlay_update_local_node_references().

This happens because the division of a signed int by an unsigned int
promotes the dividend to unsigned first (ANSI C standard 6.1.3.8).

As in this case we basically just divide by 4, we can do the division
separately earlier, which preserves the original type.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_overlay.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index b310e49..d217e79 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -241,6 +241,7 @@ static int overlay_update_local_node_references(void *fdto,
 
 		if (fixup_len % sizeof(uint32_t))
 			return -FDT_ERR_BADOVERLAY;
+		fixup_len /= sizeof(uint32_t);
 
 		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
 		if (!tree_val) {
@@ -250,7 +251,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 < fixup_len; i++) {
 			fdt32_t adj_val;
 			uint32_t poffset;
 
-- 
2.17.5

Re: [PATCH 11/14] libfdt: overlay: Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:53:00PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> a comparison in overlay_update_local_node_references().
> 
> This happens because the division of a signed int by an unsigned int
> promotes the dividend to unsigned first (ANSI C standard 6.1.3.8).
> 
> As in this case we basically just divide by 4, we can do the division
> separately earlier, which preserves the original type.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_overlay.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index b310e49..d217e79 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -241,6 +241,7 @@ static int overlay_update_local_node_references(void *fdto,
>  
>  		if (fixup_len % sizeof(uint32_t))
>  			return -FDT_ERR_BADOVERLAY;
> +		fixup_len /= sizeof(uint32_t);
>  
>  		tree_val = fdt_getprop(fdto, tree_node, name, &tree_len);
>  		if (!tree_val) {
> @@ -250,7 +251,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 < fixup_len; i++) {
>  			fdt32_t adj_val;
>  			uint32_t poffset;
>  

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

[PATCH 12/14] libfdt: fdt_get_string(): Fix sequential write comparison warnings

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness in
comparisons in fdt_get_string().

The two occassions dealing with the sequential write case are a bit
tricky, since we deliberately abuse the negative values here.

As we have just established that stroffset is negative, we can use
casts to appease the compiler.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_ro.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index ddb5a2d..059d302 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -68,9 +68,9 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 		}
 	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
 		if ((stroffset >= 0)
-		    || (stroffset < -fdt_size_dt_strings(fdt)))
+		    || (stroffset < -(int)fdt_size_dt_strings(fdt)))
 			goto fail;
-		if ((-stroffset) < len)
+		if ((unsigned)(-stroffset) < len)
 			len = -stroffset;
 	} else {
 		err = -FDT_ERR_INTERNAL;
-- 
2.17.5

Re: [PATCH 12/14] libfdt: fdt_get_string(): Fix sequential write comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:53:01PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness in
> comparisons in fdt_get_string().
> 
> The two occassions dealing with the sequential write case are a bit
> tricky, since we deliberately abuse the negative values here.
> 
> As we have just established that stroffset is negative, we can use
> casts to appease the compiler.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> ---
>  libfdt/fdt_ro.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index ddb5a2d..059d302 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -68,9 +68,9 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  		}
>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
>  		if ((stroffset >= 0)
> -		    || (stroffset < -fdt_size_dt_strings(fdt)))
> +		    || (stroffset < -(int)fdt_size_dt_strings(fdt)))

So, I'm pretty sure at this point we've tested that
fdt_size_dt_strings < INT_MAX.  But to make it more obvious that it's
safe, I think it would be preferable to case (-stroffset) to unsigned
rather than casting size_dt_strings to signed.

>  			goto fail;
> -		if ((-stroffset) < len)
> +		if ((unsigned)(-stroffset) < len)
>  			len = -stroffset;
>  	} else {
>  		err = -FDT_ERR_INTERNAL;

-- 
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 12/14] libfdt: fdt_get_string(): Fix sequential write comparison warnings

[André Przywara]

On 25/09/2020 05:09, David Gibson wrote:
> On Mon, Sep 21, 2020 at 05:53:01PM +0100, Andre Przywara wrote:
>> With -Wsign-compare, compilers warn about a mismatching signedness in
>> comparisons in fdt_get_string().
>>
>> The two occassions dealing with the sequential write case are a bit
>> tricky, since we deliberately abuse the negative values here.
>>
>> As we have just established that stroffset is negative, we can use
>> casts to appease the compiler.
>>
>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  libfdt/fdt_ro.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
>> index ddb5a2d..059d302 100644
>> --- a/libfdt/fdt_ro.c
>> +++ b/libfdt/fdt_ro.c
>> @@ -68,9 +68,9 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>>  		}
>>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
>>  		if ((stroffset >= 0)
>> -		    || (stroffset < -fdt_size_dt_strings(fdt)))
>> +		    || (stroffset < -(int)fdt_size_dt_strings(fdt)))
> 
> So, I'm pretty sure at this point we've tested that
> fdt_size_dt_strings < INT_MAX.  But to make it more obvious that it's
> safe, I think it would be preferable to case (-stroffset) to unsigned
> rather than casting size_dt_strings to signed.

Ah, indeed, and this makes the whole thing suddenly much more readable.
I took the freedom of adding: "unsigned int sw_stroffset = -stroffset;",
to avoid those ugly casts altogether.

Cheers,
Andre

> 
>>  			goto fail;
>> -		if ((-stroffset) < len)
>> +		if ((unsigned)(-stroffset) < len)
>>  			len = -stroffset;
>>  	} else {
>>  		err = -FDT_ERR_INTERNAL;
> 

[PATCH 13/14] libfdt: fdt_node_offset_by_phandle(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_node_offset_by_phandle().

Uses a better suited bitwise NOT operator to denote the special value of
-1, which automatically results in an unsigned type.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_ro.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 059d302..44ad7ec 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -680,7 +680,7 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle)
 {
 	int offset;
 
-	if ((phandle == 0) || (phandle == -1))
+	if ((phandle == 0) || (phandle == ~0U))
 		return -FDT_ERR_BADPHANDLE;
 
 	FDT_RO_PROBE(fdt);
-- 
2.17.5

Re: [PATCH 13/14] libfdt: fdt_node_offset_by_phandle(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:53:02PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_node_offset_by_phandle().
> 
> Uses a better suited bitwise NOT operator to denote the special value of
> -1, which automatically results in an unsigned type.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Applied, thanks.

> ---
>  libfdt/fdt_ro.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 059d302..44ad7ec 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -680,7 +680,7 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle)
>  {
>  	int offset;
>  
> -	if ((phandle == 0) || (phandle == -1))
> +	if ((phandle == 0) || (phandle == ~0U))
>  		return -FDT_ERR_BADPHANDLE;
>  
>  	FDT_RO_PROBE(fdt);

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

[PATCH 14/14] libfdt: fdt_strerror(): Fix comparison warning

[Andre Przywara]

With -Wsign-compare, compilers warn about a mismatching signedness
in a comparison in fdt_strerror().

At this point we are sure that errval is negative, and since
FDT_ERRTABSIZE is surely positive, we can do some casting to make the
compiler happy.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 libfdt/fdt_strerror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
index 768db66..efce51d 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)
-- 
2.17.5

Re: [PATCH 14/14] libfdt: fdt_strerror(): Fix comparison warning

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:53:03PM +0100, Andre Przywara wrote:
> With -Wsign-compare, compilers warn about a mismatching signedness
> in a comparison in fdt_strerror().
> 
> At this point we are sure that errval is negative, and since
> FDT_ERRTABSIZE is surely positive, we can do some casting to make the
> compiler happy.
> 
> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>

Since error values are typically conveyed as negative numbers (which
is kind of the whole reason we have all these signed values in the
first place), I think it would be slightly nicer to actually fold the
case to signed into the definition of FDT_ERRTABSIZE.

> ---
>  libfdt/fdt_strerror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
> index 768db66..efce51d 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)

-- 
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 14/14] libfdt: fdt_strerror(): Fix comparison warning

[André Przywara]

On 25/09/2020 05:12, David Gibson wrote:
> On Mon, Sep 21, 2020 at 05:53:03PM +0100, Andre Przywara wrote:
>> With -Wsign-compare, compilers warn about a mismatching signedness
>> in a comparison in fdt_strerror().
>>
>> At this point we are sure that errval is negative, and since
>> FDT_ERRTABSIZE is surely positive, we can do some casting to make the
>> compiler happy.
>>
>> Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> 
> Since error values are typically conveyed as negative numbers (which
> is kind of the whole reason we have all these signed values in the
> first place), I think it would be slightly nicer to actually fold the
> case to signed into the definition of FDT_ERRTABSIZE.

Right, makes more sense. I changed the comparison to:
	if (-errval < FDT_ERRTABSIZE)
which is more in line with what we actually do below.

Cheers,
Andre

>> ---
>>  libfdt/fdt_strerror.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libfdt/fdt_strerror.c b/libfdt/fdt_strerror.c
>> index 768db66..efce51d 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)
> 

Re: [PATCH 00/14] libfdt: Fix signed/unsigned comparison warnings

[David Gibson]

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

On Mon, Sep 21, 2020 at 05:52:49PM +0100, Andre Przywara wrote:
> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
> some warnings about the signedness of the operands not matching:
> =================
> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>    if ((absoffset < offset)
> .....
> =================
> 
> This does not occur under normal conditions in the dtc repo, but might
> show up when libfdt is embedded in another project. There have been reports
> from U-Boot and Trusted-Firmware-A.
> 
> The underlying issue is mostly due to C's promotion behaviour (ANSI C
> section 6.1.3.8) when dealing with operands of different signedness
> (but same size): Signed values get implictly casted to unsigned, which
> is not typically what we want if they could have been negative.
> 
> The Internet(TM) suggests that blindly applying casts is probably doing
> more harm than it helps, so this series tries to fix the underlying
> issues properly.
> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
> some signed types are due to them being returned along wih error values in
> other functions (node offsets).
> So these fixes here have been based on the following assumptions:
> - We cannot change the prototype of exported functions.
> - It's better to change types (for local variables) than to cast.
> - If we have established that a signed value is not negative, we can safely
>   cast it to an unsigned type.
> 
> I split up the fixes in small chunks, to make them easier to review.
> The first four patches change types, the next six ones use casts after
> we made sure the values are not negative.
> 
> This is only covering libfdt for now (which is what those other projects
> care about). There are more issues with dtc, but they can be addressed
> later.
> 
> Not sure if some of these checks should be gated by can_assume() calls.
> 
> Please have a look, happy to discuss the invididual cases.

Thanks so much for these.  I've applied about half of them, the rest I
have mostly minor comments for.

-- 
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 00/14] libfdt: Fix signed/unsigned comparison warnings

[André Przywara]

On 25/09/2020 05:12, David Gibson wrote:

Hi David,

> On Mon, Sep 21, 2020 at 05:52:49PM +0100, Andre Przywara wrote:
>> When libfdt is compiled with -Wsign-compare or -Wextra, GCC emits quite
>> some warnings about the signedness of the operands not matching:
>> =================
>> libfdt/fdt.c:140:18: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare]
>>    if ((absoffset < offset)
>> .....
>> =================
>>
>> This does not occur under normal conditions in the dtc repo, but might
>> show up when libfdt is embedded in another project. There have been reports
>> from U-Boot and Trusted-Firmware-A.
>>
>> The underlying issue is mostly due to C's promotion behaviour (ANSI C
>> section 6.1.3.8) when dealing with operands of different signedness
>> (but same size): Signed values get implictly casted to unsigned, which
>> is not typically what we want if they could have been negative.
>>
>> The Internet(TM) suggests that blindly applying casts is probably doing
>> more harm than it helps, so this series tries to fix the underlying
>> issues properly.
>> In libfdt, some types are somewhat suboptimal ("int bufsize" comes to mind);
>> some signed types are due to them being returned along wih error values in
>> other functions (node offsets).
>> So these fixes here have been based on the following assumptions:
>> - We cannot change the prototype of exported functions.
>> - It's better to change types (for local variables) than to cast.
>> - If we have established that a signed value is not negative, we can safely
>>   cast it to an unsigned type.
>>
>> I split up the fixes in small chunks, to make them easier to review.
>> The first four patches change types, the next six ones use casts after
>> we made sure the values are not negative.
>>
>> This is only covering libfdt for now (which is what those other projects
>> care about). There are more issues with dtc, but they can be addressed
>> later.
>>
>> Not sure if some of these checks should be gated by can_assume() calls.
>>
>> Please have a look, happy to discuss the invididual cases.
> 
> Thanks so much for these.  I've applied about half of them, the rest I
> have mostly minor comments for.

Many thanks for reviewing (and applying)! I will send the remaining
patches ASAP, fixed accordingly.

Cheers,
Andre