From 5affc221f3074be4e7239ba2a0683cae06b525fd Mon Sep 17 00:00:00 2001 From: Alexander Goncharov Date: Tue, 14 Feb 2023 19:31:44 +0400 Subject: 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 --- cli_classic.c | 5 +++-- flashrom.c | 60 +++++++++++++++++++++++++++++++++++++++-------------------- print.c | 10 ++++++++++ print_wiki.c | 2 +- 4 files changed, 54 insertions(+), 23 deletions(-) diff --git a/cli_classic.c b/cli_classic.c index 787de67a..e4db9131 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1011,7 +1011,7 @@ int main(int argc, char *argv[]) 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 @@ int main(int argc, char *argv[]) /* 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 3e6e0fb5..e14516f6 100644 --- a/flashrom.c +++ b/flashrom.c @@ -65,6 +65,17 @@ static struct shutdown_func_data { */ 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 @@ int map_flash(struct flashctx *flash) /* * 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 @@ notfound: 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 98d31094..0caa0da7 100644 --- a/print.c +++ b/print.c @@ -103,6 +103,10 @@ static int print_supported_chips(void) } 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 @@ static int print_supported_chips(void) 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 2c8c1093..a0cade9c 100644 --- a/print_wiki.c +++ b/print_wiki.c @@ -325,7 +325,7 @@ static void print_supported_chips_wiki(int cols) "|| %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), -- cgit v1.2.1