Patchwork fwts: fix common realloc() memory leak anti-pattern (LP: #1267193)

login
register
mail settings
Submitter Colin King
Date Jan. 8, 2014, 5:44 p.m.
Message ID <1389203065-14959-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/308386/
State Accepted
Headers show

Comments

Colin King - Jan. 8, 2014, 5:44 p.m.
From: Colin Ian King <colin.king@canonical.com>

cppcheck found some minor memory leaks with failed reallocs.

Signed-off-by: Colin Ian King <colin.king@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(-)
Ivan Hu - Jan. 9, 2014, 7:09 a.m.
On 01/09/2014 01:44 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> cppcheck found some minor memory leaks with failed reallocs.
>
> Signed-off-by: Colin Ian King <colin.king@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@canonical.com>
Alex Hung - Jan. 14, 2014, 2:06 a.m.
On 01/09/2014 01:44 AM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> cppcheck found some minor memory leaks with failed reallocs.
> 
> Signed-off-by: Colin Ian King <colin.king@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: Alex Hung <alex.hung@canonical.com>

Patch

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;