[M] Change in flashrom[master]: flashrom: rewrite flashbuses_to_text()

[Anastasia Klimchuk (Code Review)]

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/73039 )

Change subject: flashrom: rewrite flashbuses_to_text()
......................................................................

flashrom: rewrite flashbuses_to_text()

The previous implementation had no error handling, as a result the
flashrom could crash if the computer ran out of memory. The new
version returns NULL in such cases.

Also, rewrite lots of `if` conditions to one cycle, store a name of
buses and `enum chipbustype` in an array by using a custom struct.

The caller always expected a non-null value, so change its behavior to
handle a possible null value or use the `?` symbol. As far as `free()`
can handle null pointers, do nothing with such callers.

TEST=ninja test

Change-Id: I59b9044c99b4ba6c00d8c97f1e91af09d70dce2c
Signed-off-by: Alexander Goncharov 
Ticket: https://ticket.coreboot.org/issues/408
Reviewed-on: https://review.coreboot.org/c/flashrom/+/73039
Tested-by: build bot (Jenkins) 
Reviewed-by: Anastasia Klimchuk 
---
M cli_classic.c
M flashrom.c
M print.c
M print_wiki.c
4 files changed, 81 insertions(+), 23 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Anastasia Klimchuk: Looks good to me, approved

diff --git a/cli_classic.c b/cli_classic.c
index 787de67..e4db913 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -1011,7 +1011,7 @@
    	goto out_shutdown;
    }
    tempstr = flashbuses_to_text(get_buses_supported());
-	msg_pdbg("The following protocols are supported: %s.\n", tempstr);
+	msg_pdbg("The following protocols are supported: %s.\n", tempstr ? tempstr : "?");
    free(tempstr);

for (j = 0; j < registered_master_count; j++) {
@@ -1082,7 +1082,8 @@
    	/* repeat for convenience when looking at foreign logs */
    	tempstr = flashbuses_to_text(flashes[0].chip->bustype);
    	msg_gdbg("Found %s flash chip "%s" (%d kB, %s).\n",
-			 flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size, tempstr);
+			 flashes[0].chip->vendor, flashes[0].chip->name, flashes[0].chip->total_size,
+			 tempstr ? tempstr : "?");
    	free(tempstr);
    }

diff --git a/flashrom.c b/flashrom.c
index 3e6e0fb..e14516f 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -65,6 +65,17 @@
  */
 static bool may_register_shutdown = false;

+static struct bus_type_info {
+	enum chipbustype type;
+	const char *name;
+} bustypes[] = {
+	{ BUS_PARALLEL, "Parallel, " },
+	{ BUS_LPC, "LPC, " },
+	{ BUS_FWH, "FWH, " },
+	{ BUS_SPI, "SPI, " },
+	{ BUS_PROG, "Programmer-specific, " },
+};
+
 /* Register a function to be executed on programmer shutdown.
  * The advantage over atexit() is that you can supply a void pointer which will
  * be used as parameter to the registered function upon programmer shutdown.
@@ -912,35 +923,44 @@

/*
  * Return a string corresponding to the bustype parameter.
- * Memory is obtained with malloc() and must be freed with free() by the caller.
+ * Memory to store the string is allocated. The caller is responsible to free memory.
+ * If there is not enough memory remaining, then NULL is returned.
  */
 char *flashbuses_to_text(enum chipbustype bustype)
 {
-	char *ret = calloc(1, 1);
+	char *ret, *ptr;
+
    /*
     * FIXME: Once all chipsets and flash chips have been updated, NONSPI
     * will cease to exist and should be eliminated here as well.
     */
-	if (bustype == BUS_NONSPI) {
-		ret = strcat_realloc(ret, "Non-SPI, ");
-	} else {
-		if (bustype & BUS_PARALLEL)
-			ret = strcat_realloc(ret, "Parallel, ");
-		if (bustype & BUS_LPC)
-			ret = strcat_realloc(ret, "LPC, ");
-		if (bustype & BUS_FWH)
-			ret = strcat_realloc(ret, "FWH, ");
-		if (bustype & BUS_SPI)
-			ret = strcat_realloc(ret, "SPI, ");
-		if (bustype & BUS_PROG)
-			ret = strcat_realloc(ret, "Programmer-specific, ");
-		if (bustype == BUS_NONE)
-			ret = strcat_realloc(ret, "None, ");
+	if (bustype == BUS_NONSPI)
+		return strdup("Non-SPI");
+	if (bustype == BUS_NONE)
+		return strdup("None");
+
+	ret = calloc(1, 1);
+	if (!ret)
+		return NULL;
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(bustypes); i++)
+	{
+		if (bustype & bustypes[i].type) {
+			ptr = strcat_realloc(ret, bustypes[i].name);
+			if (!ptr) {
+				free(ret);
+				return NULL;
+			}
+			ret = ptr;
+		}
    }
+
    /* Kill last comma. */
    ret[strlen(ret) - 2] = '\0';
-	ret = realloc(ret, strlen(ret) + 1);
-	return ret;
+	ptr = realloc(ret, strlen(ret) + 1);
+	if (!ptr)
+		free(ret);
+	return ptr;
 }

static int init_default_layout(struct flashctx *flash)
@@ -1165,7 +1185,7 @@

tmp = flashbuses_to_text(flash->chip->bustype);
    msg_cinfo("%s %s flash chip "%s" (%d kB, %s) ", force ? "Assuming" : "Found",
-		  flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp);
+		  flash->chip->vendor, flash->chip->name, flash->chip->total_size, tmp ? tmp : "?");
    free(tmp);
    if (master_uses_physmap(mst))
    	msg_cinfo("mapped at physical address 0x%0*" PRIxPTR ".\n",
diff --git a/print.c b/print.c
index 98d3109..0caa0da 100644
--- a/print.c
+++ b/print.c
@@ -103,6 +103,10 @@
    	} while (1);

s = flashbuses_to_text(chip->bustype);
+		if (s == NULL) {
+			msg_gerr("Out of memory!\n");
+			return 1;
+		}
    	maxtypelen = max(maxtypelen, strlen(s));
    	free(s);
    }
@@ -265,6 +269,12 @@
    		msg_ginfo(" ");

s = flashbuses_to_text(chip->bustype);
+		if (s == NULL) {
+			msg_gerr("Out of memory!\n");
+			free(ven);
+			free(dev);
+			return 1;
+		}
    	msg_ginfo("%s", s);
    	for (i = strlen(s); i < maxtypelen; i++)
    		msg_ginfo(" ");
diff --git a/print_wiki.c b/print_wiki.c
index 2c8c109..a0cade9 100644
--- a/print_wiki.c
+++ b/print_wiki.c
@@ -325,7 +325,7 @@
    	       "|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}"
    	       "|| %s || %s\n",
    	       (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
-		       f->total_size, s,
+		       f->total_size, s ? s : "?",
    	       test_state_to_template(f->tested.probe),
    	       test_state_to_template(f->tested.read),
    	       test_state_to_template(f->tested.erase),