[flashrom] [PATCH] CID1130005: Array compared against 0
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Mon May 12 10:32:32 CEST 2014
Am 09.05.2014 21:20 schrieb Stefan Tauner:
> On Fri, 09 May 2014 00:53:39 +0200
> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>
>> > Am 08.05.2014 23:33 schrieb Stefan Tauner:
>>> > > On Thu, 08 May 2014 23:24:22 +0200
>>> > > Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>>> > >
>>>> > >> Am 08.05.2014 23:11 schrieb Stefan Tauner:
>>>>> > >>> On Thu, 08 May 2014 22:51:43 +0200
>>>>> > >>> Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:
>>>>> > >>>
>>>>>> > >>>> Am 08.05.2014 18:56 schrieb Stefan Tauner:
>>>>>>> > >>>>> On Tue, 19 Nov 2013 20:35:57 +0100
>>>>>>> > >>>>> Stefan Reinauer <stefan.reinauer at coreboot.org> wrote:
>>>>>>> > >>>>>
>>>>>>>>> > >>>>>>> CID1130005: Array compared against 0
>>>>>>>>> > >>>>>>>
>>>>>>>>> > >>>>>>> The address of an array is never NULL, so the comparison will always evaluate
>>>>>>>>> > >>>>>>> the same way.
>>>>>>>>> > >>>>>>> In selfcheck: Array compared against NULL pointer
>>>>>>>>> > >>>>>>>
>>>>>>>>> > >>>>>>> Since the array is defined unconditionally in C code the check does not really
>>>>>>>>> > >>>>>>> make sense. It might make more sense to check whether there are entries in the
>>>>>>>>> > >>>>>>> array, but that is not required on all platforms so far.
>>>>>>> > >>>>> Thanks for reporting this. I have attached by approach to fix this.
>>>>>>> > >>>>> IMHO it makes no sense to check the array outside its compilation unit.
>>>>>>> > >>>>> That's just stupid. Instead I move the checks of the flashchips array
>>>>>>> > >>>>> from flashrom.c to flashchips.c and remove all others that are futile
>>>>>>> > >>>>> anyway.
>>>>>> > >>>> NACK. flashchips.c should only have data, not code.
>>>>> > >>> Says who, and more importantly why? :)
>>>> > >> I say that.
>>>> > >> The file is called flashchips.c and not
>>>> > >> flashchip_definitions_and_functions.c. For example, we could split
>>>> > >> flashchips.c and add each group of chips to the associated driver file,
>>>> > >> e.g. spi25.c. If we indeed start introducing code to flashchips.c, we
>>>> > >> might as well kill the separation altogether. Besides that, right now
>>>> > >> that file is nicely grepable and can be processed easily with Unix
>>>> > >> command line tools for texts. I used that often in the past, and having
>>>> > >> to use head/tail in addition to grep just to weed out the non-data stuff
>>>> > >> is not exactly the kind of fun I'm looking for.
>>>> > >>
>>>> > >> I hope the reasons above are not totally unreasonable.
>>> > > Not completely unreasonable but I don't buy them. Anyway, what about
>>> > > exporting the size of the array to a global variable?
>> >
>> > Excellent idea. That would actually allow us to check for nonzero size
>> > of all arrays, and it would also allow us to check if the last member is
>> > the zero-filled member. Both conditions together are a good safeguard
>> > against accidental array overrun (e.g. someone being accidentally too
>> > broad with some #if 0 or architecture-specific #ifdef).
> Let's start with the flashchips array for now. New patch attached.
>
> >From 139fa5fe10135f8605c258592d4e4eed133296fb Mon Sep 17 00:00:00 2001
> From: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> Date: Fri, 9 May 2014 21:18:40 +0200
> Subject: [PATCH] Fix selfcheck of various arrays.
>
> Stefan Reinauer has reported ridiculous NULL checks for arrays in our
> self_check function found by Coverity (CID1130005). This patch removes
> the useless checks but keeps and fixes the one responsible for the
> flashchips array by exporting the array size in a new constant.
>
> Signed-off-by: Stefan Tauner <stefan.tauner at alumni.tuwien.ac.at>
> ---
> flash.h | 1 +
> flashchips.c | 4 +++-
> flashrom.c | 57 ++++++++++++++++++++++++---------------------------------
> 3 files changed, 28 insertions(+), 34 deletions(-)
>
> diff --git a/flash.h b/flash.h
> index 59f7cd4..4765213 100644
> --- a/flash.h
> +++ b/flash.h
> @@ -224,6 +224,7 @@ struct flashctx {
> #define TIMING_ZERO -2
>
> extern const struct flashchip flashchips[];
> +extern const unsigned int flashchips_size;
>
> void chip_writeb(const struct flashctx *flash, uint8_t val, chipaddr addr);
> void chip_writew(const struct flashctx *flash, uint16_t val, chipaddr addr);
> diff --git a/flashchips.c b/flashchips.c
> index 8059374..92dc947 100644
> --- a/flashchips.c
> +++ b/flashchips.c
> @@ -13362,5 +13362,7 @@ const struct flashchip flashchips[] = {
> .write = NULL,
> },
>
> - { NULL }
> + {0}
> };
> +
> +const unsigned int flashchips_size = ARRAY_SIZE(flashchips);
> diff --git a/flashrom.c b/flashrom.c
> index c20461a..9d64da3 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1271,10 +1271,7 @@ out_free:
> return ret;
> }
>
> -/* This function shares a lot of its structure with erase_and_write_flash() and
> - * walk_eraseregions().
> - * Even if an error is found, the function will keep going and check the rest.
> - */
> +/* Even if an error is found, the function will keep going and check the rest. */
Not sure if we want to remove parts of the comment... if we ever change
the eraseblock structure or semantics, we may want to be able to quickly
locate all places where we have to change stuff. And that may happen
soon for some weird chips with erase commands which only happen to work
on parts of the chip. The idea some months ago was to mark eraseblocks
where the erase function doesn't work by design by using a negative
eraseblock size.
Hm.
> static int selfcheck_eraseblocks(const struct flashchip *chip)
> {
> int i, j, k;
> @@ -1697,8 +1694,7 @@ void print_banner(void)
>
> int selfcheck(void)
> {
> - const struct flashchip *chip;
> - int i;
> + unsigned int i;
> int ret = 0;
>
> /* Safety check. Instead of aborting after the first error, check
> @@ -1751,37 +1747,32 @@ int selfcheck(void)
> ret = 1;
> }
> }
> - /* It would be favorable if we could also check for correct termination
> - * of the following arrays, but we don't know their sizes in here...
> - * For 'flashchips' we check the first element to be non-null. In the
> - * other cases there exist use cases where the first element can be
> - * null. */
> - if (flashchips == NULL || flashchips[0].vendor == NULL) {
> +
> + /* It would be favorable if we could check for the correct layout (especially termination) of various
> + * constant arrays: flashchips, chipset_enables, board_matches, boards_known, laptops_known.
> + * They are all defined as externs in this compilation unit so we don't know their sizes which vary
> + * depending on compiler flags, e.g. the target architecture, and can sometimes be 0.
> + * For 'flashchips' we export the size explicitly to work around this and to be able to implement the
> + * checks below. */
IMHO the comment above is superfluous.
> + if (flashchips_size <= 1 || flashchips[flashchips_size-1].name != NULL) {
size == 1 is arguably stupid because no chip would be supported, but at
least programmer init will work and we can get some valuable info from
that as well. Not sure we want to forbid it. Then again, what use is
flashrom without any flash chip support? I tend to agree with the code
you propose.
Whitespace: flashchips_size - 1
> msg_gerr("Flashchips table miscompilation!\n");
> ret = 1;
> + } else {
> + for (i = 0; i < flashchips_size - 1; i++) {
> + const struct flashchip *chip = &flashchips[i];
> + if (chip->vendor == NULL || chip->name == NULL || chip->bustype == BUS_NONE) {
> + ret = 1;
> + msg_gerr("ERROR: Some field of flash chip #%d (%s) is misconfigured.\n"
> + "Please report a bug at flashrom at flashrom.org\n", i,
> + chip->name == NULL ? "unnamed" : chip->name);
> + }
> + if (selfcheck_eraseblocks(chip)) {
> + ret = 1;
> + }
> + }
> }
> - for (chip = flashchips; chip && chip->name; chip++)
> - if (selfcheck_eraseblocks(chip))
> - ret = 1;
>
> -#if CONFIG_INTERNAL == 1
> - if (chipset_enables == NULL) {
> - msg_gerr("Chipset enables table does not exist!\n");
> - ret = 1;
> - }
> - if (board_matches == NULL) {
> - msg_gerr("Board enables table does not exist!\n");
> - ret = 1;
> - }
> - if (boards_known == NULL) {
> - msg_gerr("Known boards table does not exist!\n");
> - ret = 1;
> - }
> - if (laptops_known == NULL) {
> - msg_gerr("Known laptops table does not exist!\n");
> - ret = 1;
> - }
> -#endif
Please leave out that removal or replace it with similar size checks as
for flashchips[] except that the other array sizes only fail if <1. The
way you do the checks above is definitely the way to go.
> + /* TODO: implement similar sanity checks for other arrays where deemed necessary. */
> return ret;
> }
>
Regards,
Carl-Daniel
--
http://www.hailfinger.org/
More information about the flashrom
mailing list