[PATCH] CID1130005: Array compared against 0
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. Signed-off-by: Stefan Reinauer <stefan.reinauer@coreboot.org> Index: print.c =================================================================== --- print.c (revision 1763) +++ print.c (working copy) @@ -1073,7 +1073,7 @@ B("ZOTAC", "ZBOX HD-ID11", OK, NULL, NULL), #endif - {0}, + {NULL}, }; /* Please keep this list alphabetically ordered by vendor/board. */ @@ -1101,6 +1101,6 @@ B("Lenovo", "3000 V100 TF05Cxx", OK, "http://www5.pc.ibm.com/europe/products.nsf/products?openagent&brand=Lenovo30...", NULL), #endif - {0}, + {NULL}, }; #endif Index: flashrom.c =================================================================== --- flashrom.c (revision 1763) +++ flashrom.c (working copy) @@ -1759,24 +1759,6 @@ 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 return ret; }
On Tue, 19 Nov 2013 20:35:57 +0100 Stefan Reinauer <stefan.reinauer@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. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Am 08.05.2014 18:56 schrieb Stefan Tauner:
On Tue, 19 Nov 2013 20:35:57 +0100 Stefan Reinauer <stefan.reinauer@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. We might want to guard against an array which is not terminated with an empty (NULL/0) member, but checking if the pointer is NULL is indeed not needed. Regards, Carl-Daniel -- http://www.hailfinger.org/
On Thu, 08 May 2014 22:51:43 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@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? :) -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
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@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@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. Regards, Carl-Daniel -- http://www.hailfinger.org/
On Thu, 08 May 2014 23:24:22 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@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@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? -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
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@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@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@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). Regards, Carl-Daniel -- http://www.hailfinger.org/
On Fri, 09 May 2014 00:53:39 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@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@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@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. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
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@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@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@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@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@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@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@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/
On Mon, 12 May 2014 10:32:32 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
From: Stefan Tauner <stefan.tauner@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@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@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
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@gmx.net> wrote:
From: Stefan Tauner <stefan.tauner@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@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@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/
I forgot: Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@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@gmx.net> wrote:
From: Stefan Tauner <stefan.tauner@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@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@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
On Tue, 27 May 2014 00:03:58 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote:
I forgot: Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net>
Thanks, committed in r1799. -- Kind regards/Mit freundlichen Grüßen, Stefan Tauner
Check for NULL termination of the array, that each board has the two main PCI ID sets defined, that coreboot vendor and model fields are either both set or unset, and that either an enable function or a max decode size is available. Signed-off-by: Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at> --- board_enable.c | 30 ++++++++++++++++++++++++++++++ flashrom.c | 2 ++ programmer.h | 1 + 3 files changed, 33 insertions(+) diff --git a/board_enable.c b/board_enable.c index 2cf6776..2efc710 100644 --- a/board_enable.c +++ b/board_enable.c @@ -2463,6 +2463,36 @@ const struct board_match board_matches[] = { { 0, 0, 0, 0, 0, 0, 0, 0, NULL, NULL, NULL, P3, NULL, NULL, 0, NT, NULL}, /* end marker */ }; +int selfcheck_board_enables(void) +{ + if (board_matches[ARRAY_SIZE(board_matches) - 1].vendor_name != NULL) { + msg_gerr("Board enables table miscompilation!\n"); + return 1; + } + + int ret = 0; + unsigned int i; + for (i = 0; i < ARRAY_SIZE(board_matches) - 1; i++) { + const struct board_match *b = &board_matches[i]; + if (b->vendor_name == NULL || b->board_name == NULL) { + msg_gerr("ERROR: Board enable #%d does not define a vendor and board name.\n" + "Please report a bug at flashrom@flashrom.org\n", i); + ret = 1; + continue; + } + if ((b->first_vendor == 0 || b->first_device == 0 || + b->second_vendor == 0 || b->second_device == 0) || + ((b->lb_vendor == NULL) ^ (b->lb_part == NULL)) || + (b->max_rom_decode_parallel == 0 && b->enable == NULL)) { + msg_gerr("ERROR: Board enable for %s %s is misdefined.\n" + "Please report a bug at flashrom@flashrom.org\n", + b->vendor_name, b->board_name); + ret = 1; + } + } + return ret; +} + /* Parse the <vendor>:<board> string specified by the user as part of -p internal:mainboard=<vendor>:<board>. * Parameters vendor and model will be overwritten. Returns 0 on success. * Note: strtok modifies the original string, so we work on a copy and allocate memory for the results. diff --git a/flashrom.c b/flashrom.c index 98101b7..fdb5cf7 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1770,6 +1770,8 @@ int selfcheck(void) } } + ret |= selfcheck_board_enables(); + /* TODO: implement similar sanity checks for other arrays where deemed necessary. */ return ret; } diff --git a/programmer.h b/programmer.h index b896ddf..30b0074 100644 --- a/programmer.h +++ b/programmer.h @@ -255,6 +255,7 @@ void internal_delay(unsigned int usecs); #if CONFIG_INTERNAL == 1 /* board_enable.c */ +int selfcheck_board_enables(void); int board_parse_parameter(const char *boardstring, const char **vendor, const char **model); void w836xx_ext_enter(uint16_t port); void w836xx_ext_leave(uint16_t port); -- Kind regards, Stefan Tauner
participants (3)
-
Carl-Daniel Hailfinger -
Stefan Reinauer -
Stefan Tauner