[flashrom] [PATCH 2/4] fix memleaks due to incorrect usage of flashbuses_to_text

[Carl-Daniel Hailfinger]

Am 18.06.2011 22:53 schrieb Stefan Tauner:
> Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
>   

The memleak fixes are needed, but I'm not so happy about the long
variable name for a temp. Could you just use tmp instead of tmp_bus_str
or are there conflicts caused by other uses of tmp?


> diff --git a/chipset_enable.c b/chipset_enable.c
> index 0c77f07..70c9bae 100644
> --- a/chipset_enable.c
> +++ b/chipset_enable.c
> @@ -1206,6 +1206,7 @@ int chipset_flash_enable(void)
>  	struct pci_dev *dev = NULL;
>  	int ret = -2;		/* Nothing! */
>  	int i;
> +	char *tmp_bus_str;
>  
>  	/* Now let's try to find the chipset we have... */
>  	for (i = 0; chipset_enables[i].vendor_name != NULL; i++) {
> @@ -1244,8 +1245,10 @@ int chipset_flash_enable(void)
>  			msg_pinfo("PROBLEMS, continuing anyway\n");
>  	}
>  
> +	tmp_bus_str = flashbuses_to_text(buses_supported);
>  	msg_pinfo("This chipset supports the following protocols: %s.\n",
> -	       flashbuses_to_text(buses_supported));
> +	       tmp_bus_str);
> +	free(tmp_bus_str);
>  
>  	return ret;
>  }
> diff --git a/flashrom.c b/flashrom.c
> index cfee1a1..3e400d1 100644
> --- a/flashrom.c
> +++ b/flashrom.c
> @@ -1119,7 +1119,7 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
>  	char location[64];
>  	uint32_t size;
>  	enum chipbustype buses_common;
> -	char *tmp;
> +	char *tmp_bus_str;
>   

Is that change really needed? We don't use i_counter either.


>  
>  	for (flash = flashchips + startchip; flash && flash->name; flash++) {
>  		if (chip_to_probe && strcmp(flash->name, chip_to_probe) != 0)
> @@ -1128,13 +1128,13 @@ int probe_flash(int startchip, struct flashchip *fill_flash, int force)
>  		if (!buses_common) {
>  			msg_gspew("Probing for %s %s, %d kB: skipped. ",
>  			         flash->vendor, flash->name, flash->total_size);
> -			tmp = flashbuses_to_text(buses_supported);
> -			msg_gspew("Host bus type %s ", tmp);
> -			free(tmp);
> -			tmp = flashbuses_to_text(flash->bustype);
> +			tmp_bus_str = flashbuses_to_text(buses_supported);
> +			msg_gspew("Host bus type %s ", tmp_bus_str);
> +			free(tmp_bus_str);
> +			tmp_bus_str = flashbuses_to_text(flash->bustype);
>  			msg_gspew("and chip bus type %s are incompatible.",
> -				  tmp);
> -			free(tmp);
> +				  tmp_bus_str);
> +			free(tmp_bus_str);
>  			msg_gspew("\n");
>  			continue;
>  		}
> @@ -1186,10 +1186,12 @@ notfound:
>  #endif
>  		snprintf(location, sizeof(location), "on %s", programmer_table[programmer].name);
>  
> +	tmp_bus_str = flashbuses_to_text(flash->bustype);
>  	msg_cinfo("%s chip \"%s %s\" (%d kB, %s) %s.\n",
>  	       force ? "Assuming" : "Found",
>  	       flash->vendor, flash->name, flash->total_size,
> -	       flashbuses_to_text(flash->bustype), location);
> +	       tmp_bus_str, location);
> +	free(tmp_bus_str);
>  
>  	/* Flash registers will not be mapped if the chip was forced. Lock info
>  	 * may be stored in registers, so avoid lock info printing.
> diff --git a/print.c b/print.c
> index af3edc1..80316cb 100644
> --- a/print.c
> +++ b/print.c
> @@ -28,7 +28,7 @@
>  
>  /*
>   * Return a string corresponding to the bustype parameter.
> - * Memory is obtained with malloc() and can be freed with free().
> + * Memory is obtained with malloc() and must be freed with free() by the caller.
>   */
>  char *flashbuses_to_text(enum chipbustype bustype)
>  {
> @@ -80,6 +80,7 @@ static void print_supported_chips(void)
>  	int maxvendorlen = strlen("Vendor") + 1;
>  	int maxchiplen = strlen("Device") + 1;
>  	const struct flashchip *f;
> +	char *tmp_bus_str;
>  
>  	for (f = flashchips; f->name != NULL; f++) {
>  		/* Ignore "unknown XXXX SPI chip" entries. */
> @@ -152,7 +153,11 @@ static void print_supported_chips(void)
>  		msg_ginfo("%d", f->total_size);
>  		for (i = 0; i < 10 - digits(f->total_size); i++)
>  			msg_ginfo(" ");
> -		msg_ginfo("%s\n", flashbuses_to_text(f->bustype));
> +
> +		tmp_bus_str = flashbuses_to_text(f->bustype);
> +		msg_ginfo("%s", tmp_bus_str);
> +		free(tmp_bus_str);
> +		msg_ginfo("\n");
>  	}
>  }
>  
> diff --git a/print_wiki.c b/print_wiki.c
> index e9ccc8b..684c4e0 100644
> --- a/print_wiki.c
> +++ b/print_wiki.c
> @@ -203,6 +203,7 @@ static void print_supported_chips_wiki(int cols)
>  	int i = 0, c = 1, chipcount = 0;
>  	const struct flashchip *f, *old = NULL;
>  	uint32_t t;
> +	char *tmp_bus_str;
>  
>  	for (f = flashchips; f->name != NULL; f++)
>  		chipcount++;
> @@ -221,10 +222,11 @@ static void print_supported_chips_wiki(int cols)
>  			c = !c;
>  
>  		t = f->tested;
> +		tmp_bus_str = flashbuses_to_text(f->bustype);
>  		printf("|- bgcolor=\"#%s\"\n| %s || %s || %d "
>  		       "|| %s || {{%s}} || {{%s}} || {{%s}} || {{%s}}\n",
>  		       (c == 1) ? "eeeeee" : "dddddd", f->vendor, f->name,
> -		       f->total_size, flashbuses_to_text(f->bustype),
> +		       f->total_size, tmp_bus_str,
>  		       (t & TEST_OK_PROBE) ? "OK" :
>  		       (t & TEST_BAD_PROBE) ? "No" : "?3",
>  		       (t & TEST_OK_READ) ? "OK" :
> @@ -233,6 +235,7 @@ static void print_supported_chips_wiki(int cols)
>  		       (t & TEST_BAD_ERASE) ? "No" : "?3",
>  		       (t & TEST_OK_WRITE) ? "OK" :
>  		       (t & TEST_BAD_WRITE) ? "No" : "?3");
> +		free(tmp_bus_str);
>  
>  		/* Split table into 'cols' columns. */
>  		if (i >= (chipcount / cols + 1)) {
>   

Looks good otherwise. If you change the variable names, this is
Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Regards,
Carl-Daniel

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