[pacman-dev] [PATCH 3/3] Fix a potential memory leak in filelist creation

[Dan McGee]

If we begin to create a file list when loading a package, but abort
because of an error to one of our goto labels, the memory used to create
the file list will leak. This is because we use a set of local variables
to hold the data, and thus _alpm_pkg_free() cannot clean up for us.

Use the file list struct on the package object as much as possible to
keep state when building the file list, thus allowing _alpm_pkg_free()
to clean up any partially built data.

Signed-off-by: Dan McGee <dan at archlinux.org>
---

Also for maint.

 lib/libalpm/be_package.c |   32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
index ad34640..93b762a 100644
--- a/lib/libalpm/be_package.c
+++ b/lib/libalpm/be_package.c
@@ -360,8 +360,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle,
 	struct archive_entry *entry;
 	alpm_pkg_t *newpkg = NULL;
 	struct stat st;
-	size_t files_count = 0, files_size = 0;
-	alpm_file_t *files = NULL;
+	size_t files_size = 0;
 
 	if(pkgfile == NULL || strlen(pkgfile) == 0) {
 		RET_ERR(handle, ALPM_ERR_WRONG_ARGS, NULL);
@@ -426,28 +425,34 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle,
 			/* for now, ignore all files starting with '.' that haven't
 			 * already been handled (for future possibilities) */
 		} else if(full) {
+			const size_t files_count = newpkg->files.count;
+			alpm_file_t *current_file;
 			/* Keep track of all files for filelist generation */
 			if(files_count >= files_size) {
 				size_t old_size = files_size;
+				alpm_file_t *newfiles;
 				if(files_size == 0) {
 					files_size = 4;
 				} else {
 					files_size *= 2;
 				}
-				files = realloc(files, sizeof(alpm_file_t) * files_size);
-				if(!files) {
+				newfiles = realloc(newpkg->files.files,
+						sizeof(alpm_file_t) * files_size);
+				if(!newfiles) {
 					ALLOC_FAIL(sizeof(alpm_file_t) * files_size);
 					goto error;
 				}
 				/* ensure all new memory is zeroed out, in both the initial
 				 * allocation and later reallocs */
-				memset(files + old_size, 0,
+				memset(newfiles + old_size, 0,
 						sizeof(alpm_file_t) * (files_size - old_size));
+				newpkg->files.files = newfiles;
 			}
-			STRDUP(files[files_count].name, entry_name, goto error);
-			files[files_count].size = archive_entry_size(entry);
-			files[files_count].mode = archive_entry_mode(entry);
-			files_count++;
+			current_file = newpkg->files.files + files_count;
+			STRDUP(current_file->name, entry_name, goto error);
+			current_file->size = archive_entry_size(entry);
+			current_file->mode = archive_entry_mode(entry);
+			newpkg->files.count++;
 		}
 
 		if(archive_read_data_skip(archive)) {
@@ -485,15 +490,16 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle,
 	newpkg->infolevel = INFRQ_BASE | INFRQ_DESC | INFRQ_SCRIPTLET;
 
 	if(full) {
-		if(files) {
+		if(newpkg->files.files) {
 			/* attempt to hand back any memory we don't need */
-			files = realloc(files, sizeof(alpm_file_t) * files_count);
+			newpkg->files.files = realloc(newpkg->files.files,
+					sizeof(alpm_file_t) * newpkg->files.count);
 			/* "checking for conflicts" requires a sorted list, ensure that here */
 			_alpm_log(handle, ALPM_LOG_DEBUG,
 					"sorting package filelist for %s\n", pkgfile);
-			newpkg->files.files = files_msort(files, files_count);
+			newpkg->files.files = files_msort(newpkg->files.files,
+					newpkg->files.count);
 		}
-		newpkg->files.count = files_count;
 		newpkg->infolevel |= INFRQ_FILES;
 	}
 
-- 
1.7.9.2