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

Carl-Daniel Hailfinger c-d.hailfinger.devel.2006 at gmx.net
Tue May 27 00:03:58 CEST 2014


I forgot:
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Regards,
Carl-Daniel


Am 26.05.2014 23:54 schrieb Carl-Daniel Hailfinger:
> Am 14.05.2014 00:05 schrieb 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).
> OK with removing the comment.
>
>
>>>>  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.
> No alternative needed, your explanation is fine.
>
>
>>>> +	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?
> Yes, given that you added the FIXME comment above, a removal here is OK.
>
>
>> 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.
> Except the board enable table which is a nightmare on its own, and even
> I don't remember the exact semantics for which fields are required in
> which combination for which effect unless I explicitly read the code and
> comments.
> That said, given funnies like arch-specific contents of some arrays
> (e.g. processor enables, chipset enables), I'd like to have some checks
> for them as well just to make sure compilation on non-x86 doesn't mess
> them up. This can be done in a followup patch, though.
>
>
>> 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.
> Hm. I wonder if that causes any fun down the road when someone changes a
> checked array and recompiles only the affected file. Should be safe.
> Anyway, having a NULL-containing member as terminator allows us to
> iterate over pointers instead of having to use an explicit counter
> variable somewhere.
>
>
>>>> +	/* TODO: implement similar sanity checks for other arrays where deemed necessary. */
>>>>  	return ret;
>>>>  }
>>>>    
>> Sorry for being a bit bitchy today :)
> Sorry for the long delay of approval.
>
> Regards,
> Carl-Daniel
>


-- 
http://www.hailfinger.org/





More information about the flashrom mailing list