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

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

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

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