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@xxxxxxxxxxxx>
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:
signature.asc
Description: PGP signature
