[pacman-dev] [PATCH] Release resources on error paths.

[Andrew Gregory]

On 06/05/16 at 07:51pm, Tobias Stoeckmann wrote:
> Some resources (memory or file descriptors) are not released on all
> error paths.
> 
> Signed-off-by: Tobias Stoeckmann <tobias at stoeckmann.org>
> ---
> Yes it's rather ironic to send this patch after forgetting one on
> my own just now. ;)
> ---
>  lib/libalpm/add.c        |  5 ++++-
>  lib/libalpm/backup.c     |  5 +++--
>  lib/libalpm/be_local.c   | 18 +++++++++++++++---
>  lib/libalpm/be_package.c |  1 +
>  4 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> index f5c9a95..d132e52 100644
> --- a/lib/libalpm/add.c
> +++ b/lib/libalpm/add.c
> @@ -466,7 +466,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg,
>  		}
>  	}
>  
> -	/* prepare directory for database entries so permission are correct after
> +	/* prepare directory for database entries so permissions are correct after
>  	   changelog/install script installation */
>  	if(_alpm_local_db_prepare(db, newpkg)) {
>  		alpm_logaction(handle, ALPM_CALLER_PREFIX,
> @@ -503,6 +503,9 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg,
>  			_alpm_log(handle, ALPM_LOG_ERROR, _("could not change directory to %s (%s)\n"),
>  					handle->root, strerror(errno));
>  			_alpm_archive_read_free(archive);
> +			if(cwdfd >= 0) {
> +				close(cwdfd);
> +			}
>  			close(fd);
>  			ret = -1;
>  			goto cleanup;
> diff --git a/lib/libalpm/backup.c b/lib/libalpm/backup.c
> index f622589..50bad5e 100644
> --- a/lib/libalpm/backup.c
> +++ b/lib/libalpm/backup.c
> @@ -48,9 +48,10 @@ int _alpm_split_backup(const char *string, alpm_backup_t **backup)
>  	ptr++;
>  	/* now str points to the filename and ptr points to the hash */
>  	STRDUP((*backup)->name, str, FREE(str); return -1);
> -	STRDUP((*backup)->hash, ptr, FREE(str); return -1);
> +	STRDUP((*backup)->hash, ptr, FREE((*backup)->name); FREE(str); return -1);
>  	FREE(str);
> -	return 0;}
> +	return 0;
> +}
>  
>  /* Look for a filename in a alpm_pkg_t.backup list. If we find it,
>   * then we return the full backup entry.
> diff --git a/lib/libalpm/be_local.c b/lib/libalpm/be_local.c
> index f817822..0b351f9 100644
> --- a/lib/libalpm/be_local.c
> +++ b/lib/libalpm/be_local.c
> @@ -794,7 +794,7 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq)
>  			_alpm_strip_newline(line, 0);
>  			if(strcmp(line, "%FILES%") == 0) {
>  				size_t files_count = 0, files_size = 0, len;
> -				alpm_file_t *files = NULL;
> +				alpm_file_t *files = NULL, *newfiles;
>  
>  				while(safe_fgets(line, sizeof(line), fp) &&
>  						(len = _alpm_strip_newline(line, 0))) {
> @@ -805,13 +805,18 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq)

Immediately above this is a call to _alpm_greedy_grow which needs
memory released on failure as well.

>  					/* since we know the length of the file string already,
>  					 * we can do malloc + memcpy rather than strdup */
>  					len += 1;
> -					MALLOC(files[files_count].name, len, goto error);
> +					MALLOC(files[files_count].name, len, goto nomem);
>  					memcpy(files[files_count].name, line, len);
>  					files_count++;
>  				}
>  				/* attempt to hand back any memory we don't need */
>  				if(files_count > 0) {
> -					files = realloc(files, sizeof(alpm_file_t) * files_count);
> +					newfiles = realloc(files, sizeof(alpm_file_t) * files_count);

newfiles should be declared here; we limit variables to the smallest
scope possible.

> +					if(newfiles == NULL) {
> +						goto nomem;

I don't think we need to treat this as an error.  The realloc is just
releasing memory; failure should not be significant.

> +					}
> +					files = newfiles;
> +
>  					/* make sure the list is sorted */
>  					qsort(files, files_count, sizeof(alpm_file_t), _alpm_files_cmp);
>  				} else {
> @@ -819,6 +824,13 @@ static int local_db_read(alpm_pkg_t *info, alpm_dbinfrq_t inforeq)
>  				}
>  				info->files.count = files_count;
>  				info->files.files = files;
> +				continue;
> +nomem:
> +				while(files_count > 0) {
> +					FREE(files[--files_count].name);
> +				}
> +				FREE(files);
> +				goto error;
>  			} else if(strcmp(line, "%BACKUP%") == 0) {
>  				while(safe_fgets(line, sizeof(line), fp) && _alpm_strip_newline(line, 0)) {
>  					alpm_backup_t *backup;
> diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
> index c9ed770..a79c0c5 100644
> --- a/lib/libalpm/be_package.c
> +++ b/lib/libalpm/be_package.c
> @@ -762,6 +762,7 @@ int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle, const char *filename, int ful
>  
>  			if(fail) {
>  				_alpm_log(handle, ALPM_LOG_ERROR, _("required key missing from keyring\n"));
> +				free(sigpath);
>  				return -1;
>  			}
>  		}
> -- 
> 2.8.3