Date: Mon, 13 Jan 2020 09:17:50 -0600 From: Justin Hibbits <chmeeedalf@gmail.com> To: Mark Millard <marklmi@yahoo.com> Cc: FreeBSD PowerPC ML <freebsd-ppc@freebsd.org> Subject: Re: head -r356640 and mixed signed/unsigned comparisons Message-ID: <20200113091750.4a02b428@titan.knownspace> In-Reply-To: <18622739-BB2E-432B-B877-2FBE3A9D4ECB@yahoo.com> References: <18622739-BB2E-432B-B877-2FBE3A9D4ECB.ref@yahoo.com> <18622739-BB2E-432B-B877-2FBE3A9D4ECB@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 11 Jan 2020 17:28:22 -0800 Mark Millard <marklmi@yahoo.com> wrote: > The code: > > 394 for (j = 0; j < addr_cells - 1; j++) { > > looks wrong to me. Evidence follows. > > /usr/src/sys/dev/ofw/openfirm.h:typedef uint32_t pcell_t; > /usr/include/sys/_stdint.h:typedef __uint32_t > uint32_t; /usr/include/machine/_types.h:typedef unsigned int > __uint32_t; > > So pcell_t and int are of the same rank but different > unsigned vs. signed status. > > From sys/powerpc/mpc85xx/lbc.c : > > 363 static int > 364 fdt_lbc_reg_decode(phandle_t node, struct lbc_softc *sc, > 365 struct lbc_devinfo *di) > 366 { > . . . > 369 pcell_t addr_cells, size_cells; > . . . > 371 int i, j, rv, bank; > . . . > 394 for (j = 0; j < addr_cells - 1; j++) { > 395 start <<= 32; > 396 start |= reg[j]; > 397 } > > So, for line 394, after the usual arithmetic > conversions for the "-" and then for the "<" > the line would look like (consolidating > some relevant material to be more > textually-local for ease of comparison): > > for (int j = 0; (unsigned int)j < addr_cells-1u; j++) > > For addr_cells==0u: addr_cells-1u == UINT_MAX . > > So, for addr_cells==0u substituted (at run-time): > > for (int j = 0; (unsigned int)j < UINT_MAX; j++) > > So, unless it is guaranteed that 0u<addr_cells, > there would appear to be a looping problem here. > > It appears to me that the condition: > > addr_cells==0u && 0u<size_cells > > would fail the tuples <= 0 test and so continue > on to try to use line 394. > > But I do not have context to know the > condition would be a worrisome issue. The only way addr_cells and size_cells can be zero is if the device tree explicitly has #address-cells = <0> and #size-cells = <0>. It's not impossible, but it's not reasonable, and violates the requirements of the node. It doesn't hurt to add checks, but it's not a priority either. - Justin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200113091750.4a02b428>