[flashrom] [commit] r1799 - trunk

[repository service]

Author: stefanct
Date: Tue May 27 00:05:31 2014
New Revision: 1799
URL: http://flashrom.org/trac/flashrom/changeset/1799

Log:
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>
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Modified:
   trunk/flash.h
   trunk/flashchips.c
   trunk/flashrom.c

Modified: trunk/flash.h
==============================================================================
--- trunk/flash.h	Mon May 26 02:36:24 2014	(r1798)
+++ trunk/flash.h	Tue May 27 00:05:31 2014	(r1799)
@@ -225,6 +225,7 @@
 #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);

Modified: trunk/flashchips.c
==============================================================================
--- trunk/flashchips.c	Mon May 26 02:36:24 2014	(r1798)
+++ trunk/flashchips.c	Tue May 27 00:05:31 2014	(r1799)
@@ -13424,5 +13424,7 @@
 		.write		= NULL,
 	},
 
-	{ NULL 	}
+	{0}
 };
+
+const unsigned int flashchips_size = ARRAY_SIZE(flashchips);

Modified: trunk/flashrom.c
==============================================================================
--- trunk/flashrom.c	Mon May 26 02:36:24 2014	(r1798)
+++ trunk/flashrom.c	Tue May 27 00:05:31 2014	(r1799)
@@ -1269,10 +1269,7 @@
 	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. */
 static int selfcheck_eraseblocks(const struct flashchip *chip)
 {
 	int i, j, k;
@@ -1694,8 +1691,7 @@
 
 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
@@ -1748,37 +1744,32 @@
 			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. */
+	if (flashchips_size <= 1 || flashchips[flashchips_size-1].name != NULL) {
 		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
+	/* TODO: implement similar sanity checks for other arrays where deemed necessary. */
 	return ret;
 }