ACK: [PATCH] fwts: fix common realloc() memory leak anti-pattern (LP: #1267193)

[IvanHu]

On 01/09/2014 01:44 AM, Colin King wrote:
> From: Colin Ian King <colin.king at canonical.com>
>
> cppcheck found some minor memory leaks with failed reallocs.
>
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
> ---
>   src/acpica/fwts_acpica.c       | 11 ++++++++---
>   src/lib/src/fwts_acpi_tables.c |  7 ++++++-
>   src/lib/src/fwts_acpid.c       | 10 ++++++++--
>   src/lib/src/fwts_args.c        | 11 ++++++++---
>   src/lib/src/fwts_log.c         |  9 +++++++--
>   src/lib/src/fwts_pipeio.c      | 10 ++++++++--
>   6 files changed, 45 insertions(+), 13 deletions(-)
>
> diff --git a/src/acpica/fwts_acpica.c b/src/acpica/fwts_acpica.c
> index 3e011a8..06aa0b7 100644
> --- a/src/acpica/fwts_acpica.c
> +++ b/src/acpica/fwts_acpica.c
> @@ -427,12 +427,17 @@ void fwts_acpica_vprintf(const char *fmt, va_list args)
>   		else
>   			buffer_len = 0;
>   	} else {
> +		char *new_buf;
> +
>   		buffer_len += tmp_len;
> -		buffer = realloc(buffer, buffer_len);
> -		if (buffer)
> +		new_buf = realloc(buffer, buffer_len);
> +		if (new_buf) {
> +			buffer = new_buf;
>   			strcat(buffer, tmp);
> -		else
> +		} else {
> +			free(buffer);
>   			buffer_len = 0;
> +		}
>   	}
>
>   	if (index(buffer, '\n') != NULL) {
> diff --git a/src/lib/src/fwts_acpi_tables.c b/src/lib/src/fwts_acpi_tables.c
> index e899e5e..b226e51 100644
> --- a/src/lib/src/fwts_acpi_tables.c
> +++ b/src/lib/src/fwts_acpi_tables.c
> @@ -415,6 +415,7 @@ static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
>
>   	/* Pull in 16 bytes at a time */
>   	while (fgets(buffer, sizeof(buffer), fp) ) {
> +		uint8_t *new_tmp;
>   		int n;
>   		buffer[56] = '\0';	/* truncate */
>   		if ((n = sscanf(buffer,"  %x: %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx %hhx",
> @@ -426,8 +427,12 @@ static uint8_t *fwts_acpi_load_table_from_acpidump(FILE *fp, char *name, uint64_
>   			break;
>
>   		len += (n - 1);
> -		if ((tmp = realloc(tmp, len)) == NULL)
> +		if ((new_tmp = realloc(tmp, len)) == NULL) {
> +			free(tmp);
>   			return NULL;
> +		} else
> +			tmp = new_tmp;
> +
>   		memcpy(tmp + offset, data, n-1);
>   	}
>
> diff --git a/src/lib/src/fwts_acpid.c b/src/lib/src/fwts_acpid.c
> index 5aac045..3620a45 100644
> --- a/src/lib/src/fwts_acpid.c
> +++ b/src/lib/src/fwts_acpid.c
> @@ -105,9 +105,15 @@ char *fwts_acpi_event_read(const int fd, size_t *length, const int timeout)
>   			return NULL;
>   		}
>   		else {
> -			ptr = realloc(ptr, size + n + 1);
> -			if (ptr == NULL)
> +			char *new_ptr;
> +
> +			new_ptr = realloc(ptr, size + n + 1);
> +			if (new_ptr == NULL) {
> +				free(ptr);
>   				return NULL;
> +			} else
> +				ptr = new_ptr;
> +
>   			memcpy(ptr + size, buffer, n);
>   			size += n;
>   			*(ptr+size) = 0;
> diff --git a/src/lib/src/fwts_args.c b/src/lib/src/fwts_args.c
> index 829182a..631ba5f 100644
> --- a/src/lib/src/fwts_args.c
> +++ b/src/lib/src/fwts_args.c
> @@ -129,15 +129,20 @@ int fwts_args_parse(fwts_framework *fw, const int argc, char * const argv[])
>
>   			if (short_name && (len = strlen(short_name)) > 0) {
>   				if (short_options) {
> -					short_options = realloc(short_options,
> +					char *new_short_options;
> +
> +					new_short_options = realloc(short_options,
>   						short_options_len + len + 1);
> -					if (short_options == NULL) {
> +					if (new_short_options == NULL) {
> +						free(short_options);
>   						free(long_options);
>   						fwts_log_error(fw,
>   							"Out of memory "
>   							"allocating options.");
>   						return FWTS_ERROR;
> -					}
> +					} else
> +						short_options = new_short_options;
> +
>   					strcat(short_options, short_name);
>   					short_options_len += (len + 1);
>   				} else {
> diff --git a/src/lib/src/fwts_log.c b/src/lib/src/fwts_log.c
> index 53cdbc8..8d9923e 100644
> --- a/src/lib/src/fwts_log.c
> +++ b/src/lib/src/fwts_log.c
> @@ -578,11 +578,16 @@ char *fwts_log_get_filenames(const char *filename, const fwts_log_type type)
>   			}
>
>   			if (filenames) {
> +				char *new_filenames;
> +
>   				len += strlen(tmp) + 2;
> -				if ((filenames = realloc(filenames, len)) == NULL) {
> +				if ((new_filenames = realloc(filenames, len)) == NULL) {
> +					free(filenames);
>   					free(tmp);
>   					return NULL;
> -				}
> +				} else
> +					filenames = new_filenames;
> +
>   				strcat(filenames, " ");
>   				strcat(filenames, tmp);
>   			} else {
> diff --git a/src/lib/src/fwts_pipeio.c b/src/lib/src/fwts_pipeio.c
> index 13f8150..1f21ed7 100644
> --- a/src/lib/src/fwts_pipeio.c
> +++ b/src/lib/src/fwts_pipeio.c
> @@ -97,9 +97,15 @@ char *fwts_pipe_read(const int fd, ssize_t *length)
>   			}
>   		}
>   		else {
> -			ptr = realloc(ptr, size + n + 1);
> -			if (ptr == NULL)
> +			char *new_ptr;
> +
> +			new_ptr = realloc(ptr, size + n + 1);
> +			if (new_ptr == NULL) {
> +				free(ptr);
>   				return NULL;
> +			} else
> +				ptr = new_ptr;
> +
>   			memcpy(ptr + size, buffer, n);
>   			size += n;
>   			*(ptr+size) = 0;
>

Acked-by: Ivan Hu <ivan.hu at canonical.com>