[pacman-dev] [PATCH 1/3] Database cleanup enhancements
Ensure we give database signatures special treatment like we already did for package signatures. Attempt to parse the database name out of them before taking the proper steps to handle their existence. This fixes FS#28714. We also add an unlink_verbose() helper method that displays any errors that occur when unlinking, optionally opting to skip any ENOENT errors from being fatal. Finally, the one prompt per unknown database has been removed, this has no real sound purpose and we don't do this for packages. Simply kill databases we don't know about; other programs shouldn't have random data in this directory anyway. Signed-off-by: Dan McGee <dan@archlinux.org> --- For maint. src/pacman/sync.c | 71 +++++++++++++++++++++++++++++++++-------------------- 1 file changed, 45 insertions(+), 26 deletions(-) diff --git a/src/pacman/sync.c b/src/pacman/sync.c index fcc887a..a9a9e99 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -25,6 +25,7 @@ #include <string.h> #include <limits.h> #include <unistd.h> +#include <errno.h> #include <dirent.h> #include <sys/stat.h> @@ -37,6 +38,20 @@ #include "package.h" #include "conf.h" +static int unlink_verbose(const char *pathname, int ignore_missing) +{ + int ret = unlink(pathname); + if(ret) { + if(ignore_missing && errno == ENOENT) { + ret = 0; + } else { + pm_printf(ALPM_LOG_ERROR, _("could not remove %s: %s\n"), + pathname, strerror(errno)); + } + } + return ret; +} + /* if keep_used != 0, then the db files which match an used syncdb * will be kept */ static int sync_cleandb(const char *dbpath, int keep_used) @@ -44,6 +59,7 @@ static int sync_cleandb(const char *dbpath, int keep_used) DIR *dir; struct dirent *ent; alpm_list_t *syncdbs; + int ret = 0; dir = opendir(dbpath); if(dir == NULL) { @@ -60,6 +76,7 @@ static int sync_cleandb(const char *dbpath, int keep_used) struct stat buf; int found = 0; const char *dname = ent->d_name; + char *dbname; size_t len; if(strcmp(dname, ".") == 0 || strcmp(dname, "..") == 0) { @@ -79,44 +96,46 @@ static int sync_cleandb(const char *dbpath, int keep_used) /* remove all non-skipped directories and non-database files */ stat(path, &buf); - len = strlen(path); - if(S_ISDIR(buf.st_mode) || strcmp(path + len - 3, ".db") != 0) { + if(S_ISDIR(buf.st_mode)) { if(rmrf(path)) { - pm_printf(ALPM_LOG_ERROR, - _("could not remove %s\n"), path); - closedir(dir); - return 1; + pm_printf(ALPM_LOG_ERROR, _("could not remove %s: %s\n"), + path, strerror(errno)); } continue; } + len = strlen(dname); + if(len > 3 && strcmp(dname + len - 3, ".db") == 0) { + dbname = strndup(dname, len - 3); + } else if(len > 7 && strcmp(dname + len - 7, ".db.sig") == 0) { + dbname = strndup(dname, len - 7); + } else { + ret += unlink_verbose(path, 0); + continue; + } + if(keep_used) { alpm_list_t *i; - len = strlen(dname); - char *dbname = strndup(dname, len - 3); for(i = syncdbs; i && !found; i = alpm_list_next(i)) { alpm_db_t *db = alpm_list_getdata(i); found = !strcmp(dbname, alpm_db_get_name(db)); } - free(dbname); } - /* We have a database that doesn't match any syncdb. - * Ask the user if he wants to remove it. */ - if(!found) { - if(!yesno(_("Do you want to remove %s?"), path)) { - continue; - } - if(rmrf(path)) { - pm_printf(ALPM_LOG_ERROR, - _("could not remove %s\n"), path); - closedir(dir); - return 1; - } + /* We have a database that doesn't match any syncdb. */ + if(!found) { + /* ENOENT check is because the signature and database could come in any + * order in our readdir() call, so either file may already be gone. */ + snprintf(path, PATH_MAX, "%s%s.db", dbpath, dbname); + ret += unlink_verbose(path, 1); + /* unlink a signature file if present too */ + snprintf(path, PATH_MAX, "%s%s.db.sig", dbpath, dbname); + ret += unlink_verbose(path, 1); } + free(dbname); } closedir(dir); - return 0; + return ret; } static int sync_cleandb_all(void) @@ -130,6 +149,7 @@ static int sync_cleandb_all(void) if(!yesno(_("Do you want to remove unused repositories?"))) { return 0; } + printf(_("removing unused sync repositories...\n")); /* The sync dbs were previously put in dbpath/ but are now in dbpath/sync/. * We will clean everything in dbpath/ except local/, sync/ and db.lck, and * only the unused sync dbs in dbpath/sync/ */ @@ -142,7 +162,6 @@ static int sync_cleandb_all(void) ret += sync_cleandb(newdbpath, 1); free(newdbpath); - printf(_("Database directory cleaned up\n")); return ret; } @@ -210,7 +229,7 @@ static int sync_cleancache(int level) /* short circuit for removing all files from cache */ if(level > 1) { - unlink(path); + ret += unlink_verbose(path, 0); continue; } @@ -256,11 +275,11 @@ static int sync_cleancache(int level) if(delete) { size_t pathlen = strlen(path); - unlink(path); + ret += unlink_verbose(path, 0); /* unlink a signature file if present too */ if(PATH_MAX - 5 >= pathlen) { strcpy(path + pathlen, ".sig"); - unlink(path); + ret += unlink_verbose(path, 1); } } } -- 1.7.9.2
This is easily triggered via a `pacman -Sc` operation when it attempts to open a delta file as a package- we end up leaking loads of memory due to us never freeing the archive object. When you have upwards of 1200 delta files in your sync database directory, this results in a memory leak of nearly 1.5 MiB. Also fix another memory leak noticed at the same time- we need to call the internal _alpm_pkg_free() function, as without the origin data being set the public free function will do nothing. Signed-off-by: Dan McGee <dan@archlinux.org> --- For maint. Seen when testing the previous patch. lib/libalpm/be_package.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 4d9d0e8..ad34640 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -382,7 +382,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, /* try to create an archive object to read in the package */ if((archive = archive_read_new()) == NULL) { - alpm_pkg_free(newpkg); + _alpm_pkg_free(newpkg); RET_ERR(handle, ALPM_ERR_LIBARCHIVE, NULL); } @@ -391,8 +391,8 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, if(archive_read_open_filename(archive, pkgfile, ALPM_BUFFER_SIZE) != ARCHIVE_OK) { - alpm_pkg_free(newpkg); - RET_ERR(handle, ALPM_ERR_PKG_OPEN, NULL); + handle->pm_errno = ALPM_ERR_PKG_OPEN; + goto error; } _alpm_log(handle, ALPM_LOG_DEBUG, "starting package load for %s\n", pkgfile); -- 1.7.9.2
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@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
participants (1)
-
Dan McGee