[flashrom] [PATCH] CID1130005: Array compared against 0

[Stefan Tauner]

On Mon, 12 May 2014 10:32:32 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

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

I'd definitely grep for the modified struct member(s) instead of some
vague comments that might or might not exist :)
The old comment reads to me as the author suggests that the similarity
(which is not very surprising actually) should be exploited somehow
(which I think would be a very bad idea). (erase_and_write_block_helper
is already a nightmare that will probably never pay off - at least I
can't see how that could happen).

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

Well, apparently quite a lot of people did not understand the problem
that has been tried to be solved with the code following the original
comment, neither was the code itself understood (else it would never
have been committed), nor was the insanity of a completely bogus "fix"
for this committed to chromiumos (after a review!) clear to the authors:
https://chromium-review.googlesource.com/#/c/188240/
To be fair, I did not notice the original problem myself either.
All of the above indicates to me that there definitely should be an
explanation, maybe not as verbose... feel free to suggest an
alternative.

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

yes... *shrug*

> Whitespace: flashchips_size - 1

fixed.

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

Why should I keep useless and embarrassing code in when the comment
below replaces it well? And BTW i don't deem the other arrays worth
checking. The flashchips array is special because it is complicated and
the complexity (i.e. eraser layouts) can easily be checked for sanity
automatically. The others are simply arrays with easily to check
semantics and syntaxes. If I'd touch related code at all then I would
like to remove the NULL delimiter and use the (then exported) constant
size variables to iterate over them everywhere.

> > +	/* TODO: implement similar sanity checks for other arrays where deemed necessary. */
> >  	return ret;
> >  }
> >    

Sorry for being a bit bitchy today :)
-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner