From 7a6b01d46c1da385dd34466e757beafa33e02afa Mon Sep 17 00:00:00 2001 From: Dan McGee Date: Wed, 26 Oct 2011 15:44:55 -0500 Subject: Don't realloc a 0-length files array when loading packages There is some pecular behavior going on here when a package is loaded that has no files, as is very common in our test suite. When we enter the realloc/sort code, a package without files will call the following: files = realloc(NULL, 0); One would assume this is a no-op, returning a NULL pointer, but that is not the case and valgrind later reports we are leaking memory. Fix the whole thing by skipping the reallocation and sort steps if the pointer is NULL, as we have nothing to do. Note that the package still gets marked as 'files loaded', becuase although there were none, we tried and were successful. Signed-off-by: Dan McGee --- lib/libalpm/be_package.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) (limited to 'lib') diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c20c703f..c8e08e22 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -482,17 +482,19 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, newpkg->origin_data.file = strdup(pkgfile); newpkg->ops = get_file_pkg_ops(); newpkg->handle = handle; + newpkg->infolevel = INFRQ_BASE | INFRQ_DESC | INFRQ_SCRIPTLET; if(full) { - /* attempt to hand back any memory we don't need */ - files = realloc(files, sizeof(alpm_file_t) * 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); + if(files) { + /* attempt to hand back any memory we don't need */ + files = realloc(files, sizeof(alpm_file_t) * 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.count = files_count; - newpkg->infolevel = INFRQ_BASE | INFRQ_DESC | INFRQ_FILES | INFRQ_SCRIPTLET; - } else { - newpkg->infolevel = INFRQ_BASE | INFRQ_DESC | INFRQ_SCRIPTLET; + newpkg->infolevel |= INFRQ_FILES; } return newpkg; -- cgit v1.2.3-70-g09d2 From 8a9ce12a27970beb644952a83d27f54d1d7f751a Mon Sep 17 00:00:00 2001 From: Dan McGee Date: Wed, 26 Oct 2011 17:32:46 -0500 Subject: Fix issues with replacing unowned symlinks There aretwo seperate issues in the same block of file conflict checking code here: 1) If realpath errored, such as when a symlink was broken, we would call 'continue' rather than simply exit this particular method of resolution. This was likely just a copy-paste mistake as the previous resolving steps all use loops where continue makes sense. Refactor the check so we only proceed if realpath is successful, and continue with the rest of the checks either way. 2) The real problem this code was trying to solve was canonicalizing path component (e.g., directory) symlinks. The final component, if not a directory, should not be handled at all in this loop. Add a !S_ISLNK() condition to the loop so we only call this for real files. There are few other small cleanups to the debug messages that I made while debugging this problem- we don't need to keep printing the file name, and ensure every block that sets resolved_conflict to true prints a debug message so we know how it was resolved. This fixes the expected failures from symlink010.py and symlink011.py, while still ensuring the fix for fileconflict007.py works. Signed-off-by: Dan McGee --- lib/libalpm/conflict.c | 33 +++++++++++++++++++-------------- test/pacman/tests/symlink010.py | 2 -- test/pacman/tests/symlink011.py | 2 -- 3 files changed, 19 insertions(+), 18 deletions(-) (limited to 'lib') diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 14c23f45..32f6f303 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -469,16 +469,18 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, continue; } + _alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path); + if(S_ISDIR(file->mode)) { struct stat sbuf; if(S_ISDIR(lsbuf.st_mode)) { - _alpm_log(handle, ALPM_LOG_DEBUG, "%s is a directory, not a conflict\n", path); + _alpm_log(handle, ALPM_LOG_DEBUG, "file is a directory, not a conflict\n"); continue; } stat(path, &sbuf); if(S_ISLNK(lsbuf.st_mode) && S_ISDIR(sbuf.st_mode)) { _alpm_log(handle, ALPM_LOG_DEBUG, - "%s is a symlink to a dir, hopefully not a conflict\n", path); + "file is a symlink to a dir, hopefully not a conflict\n"); continue; } /* if we made it to here, we want all subsequent path comparisons to @@ -487,7 +489,6 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, path[strlen(path) - 1] = '\0'; } - _alpm_log(handle, ALPM_LOG_DEBUG, "checking possible conflict: %s\n", path); relative_path = path + strlen(handle->root); /* Check remove list (will we remove the conflicting local file?) */ @@ -496,7 +497,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, if(rempkg && _alpm_filelist_contains(alpm_pkg_get_files(rempkg), relative_path)) { _alpm_log(handle, ALPM_LOG_DEBUG, - "local file will be removed, not a conflict: %s\n", path); + "local file will be removed, not a conflict\n"); resolved_conflict = 1; } } @@ -517,7 +518,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, handle->trans->skip_remove = alpm_list_add(handle->trans->skip_remove, strdup(filestr)); _alpm_log(handle, ALPM_LOG_DEBUG, - "file changed packages, adding to remove skiplist: %s\n", path); + "file changed packages, adding to remove skiplist\n"); resolved_conflict = 1; } } @@ -535,16 +536,20 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, free(dir); } - if(!resolved_conflict && dbpkg) { + /* check if a component of the filepath was a link. canonicalize the path + * and look for it in the old package. note that the actual file under + * consideration cannot itself be a link, as it might be unowned- path + * components can be safely checked as all directories are "unowned". */ + if(!resolved_conflict && dbpkg && !S_ISLNK(lsbuf.st_mode)) { char *rpath = calloc(PATH_MAX, sizeof(char)); const char *relative_rpath; - if(!realpath(path, rpath)) { - free(rpath); - continue; - } - relative_rpath = rpath + strlen(handle->root); - if(_alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { - resolved_conflict = 1; + if(realpath(path, rpath)) { + relative_rpath = rpath + strlen(handle->root); + if(_alpm_filelist_contains(alpm_pkg_get_files(dbpkg), relative_rpath)) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "package contained the resolved realpath\n"); + resolved_conflict = 1; + } } free(rpath); } @@ -560,7 +565,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, } if(!found) { _alpm_log(handle, ALPM_LOG_DEBUG, - "file was unowned but in new backup list: %s\n", path); + "file was unowned but in new backup list\n"); resolved_conflict = 1; } } diff --git a/test/pacman/tests/symlink010.py b/test/pacman/tests/symlink010.py index 8c80dbc9..a1e562e1 100644 --- a/test/pacman/tests/symlink010.py +++ b/test/pacman/tests/symlink010.py @@ -22,5 +22,3 @@ self.addrule("FILE_EXIST=usr/bin/otherprog") self.addrule("FILE_TYPE=usr/bin/myprog|file") self.addrule("FILE_TYPE=usr/bin/myprogsuffix|link") self.addrule("FILE_TYPE=usr/bin/otherprog|file") - -self.expectfailure = True diff --git a/test/pacman/tests/symlink011.py b/test/pacman/tests/symlink011.py index 85101725..93cd9ddf 100644 --- a/test/pacman/tests/symlink011.py +++ b/test/pacman/tests/symlink011.py @@ -22,5 +22,3 @@ self.addrule("FILE_EXIST=usr/bin/otherprog") self.addrule("FILE_TYPE=usr/bin/myprog|file") self.addrule("FILE_TYPE=usr/bin/myprogsuffix|link") self.addrule("FILE_TYPE=usr/bin/otherprog|file") - -self.expectfailure = True -- cgit v1.2.3-70-g09d2 From 3343185473c79d556b29bda30c9c482c7fef5289 Mon Sep 17 00:00:00 2001 From: Dan McGee Date: Thu, 27 Oct 2011 14:59:24 -0500 Subject: Introduce ALPM_BUFFER_SIZE constant This takes the place of three previously used constants: ARCHIVE_DEFAULT_BYTES_PER_BLOCK, BUFFER_SIZE, and CPBUFSIZE. In libarchive 3.0, the first constant will be no more, so we can ensure we are forward-compatible by removing our usage of it now. The rest are unified for consistency. By default, we will use the value of BUFSIZ provided by , which is 8192 on Linux. If that is undefined, a default value is provided. Signed-off-by: Dan McGee --- lib/libalpm/add.c | 11 +++++++---- lib/libalpm/be_package.c | 4 ++-- lib/libalpm/be_sync.c | 2 +- lib/libalpm/util.c | 20 ++++++++------------ lib/libalpm/util.h | 7 +++++++ 5 files changed, 25 insertions(+), 19 deletions(-) (limited to 'lib') diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 653d2bd9..6c2f0cb6 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -456,6 +456,7 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, alpm_pkg_t *oldpkg = NULL; alpm_db_t *db = handle->db_local; alpm_trans_t *trans = handle->trans; + const char *pkgfile; ASSERT(trans != NULL, return -1); @@ -479,13 +480,15 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, EVENT(handle, ALPM_EVENT_ADD_START, newpkg, NULL); } + pkgfile = newpkg->origin_data.file; + _alpm_log(handle, ALPM_LOG_DEBUG, "%s package %s-%s\n", is_upgrade ? "upgrading" : "adding", newpkg->name, newpkg->version); /* pre_install/pre_upgrade scriptlet */ if(alpm_pkg_has_scriptlet(newpkg) && !(trans->flags & ALPM_TRANS_FLAG_NOSCRIPTLET)) { const char *scriptlet_name = is_upgrade ? "pre_upgrade" : "pre_install"; - _alpm_runscriptlet(handle, newpkg->origin_data.file, + _alpm_runscriptlet(handle, pkgfile, scriptlet_name, newpkg->version, NULL, 1); } @@ -531,9 +534,9 @@ static int commit_single_pkg(alpm_handle_t *handle, alpm_pkg_t *newpkg, archive_read_support_compression_all(archive); archive_read_support_format_all(archive); - _alpm_log(handle, ALPM_LOG_DEBUG, "archive: %s\n", newpkg->origin_data.file); - if(archive_read_open_filename(archive, newpkg->origin_data.file, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + _alpm_log(handle, ALPM_LOG_DEBUG, "archive: %s\n", pkgfile); + if(archive_read_open_filename(archive, pkgfile, + ALPM_BUFFER_SIZE) != ARCHIVE_OK) { handle->pm_errno = ALPM_ERR_PKG_OPEN; ret = -1; goto cleanup; diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c8e08e22..4d9d0e82 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -59,7 +59,7 @@ static void *_package_changelog_open(alpm_pkg_t *pkg) archive_read_support_format_all(archive); if(archive_read_open_filename(archive, pkgfile, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + ALPM_BUFFER_SIZE) != ARCHIVE_OK) { RET_ERR(pkg->handle, ALPM_ERR_PKG_OPEN, NULL); } @@ -390,7 +390,7 @@ alpm_pkg_t *_alpm_pkg_load_internal(alpm_handle_t *handle, archive_read_support_format_all(archive); if(archive_read_open_filename(archive, pkgfile, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + ALPM_BUFFER_SIZE) != ARCHIVE_OK) { alpm_pkg_free(newpkg); RET_ERR(handle, ALPM_ERR_PKG_OPEN, NULL); } diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index e9e816c0..3c990246 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -440,7 +440,7 @@ static int sync_db_populate(alpm_db_t *db) _alpm_log(db->handle, ALPM_LOG_DEBUG, "opening database archive %s\n", dbpath); if(archive_read_open_filename(archive, dbpath, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + ALPM_BUFFER_SIZE) != ARCHIVE_OK) { _alpm_log(db->handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), dbpath, archive_error_string(archive)); archive_read_finish(archive); diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index beefa936..12286c6a 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -129,8 +129,6 @@ int _alpm_makepath_mode(const char *path, mode_t mode) return ret; } -#define CPBUFSIZE 8 * 1024 - int _alpm_copyfile(const char *src, const char *dest) { FILE *in, *out; @@ -148,10 +146,10 @@ int _alpm_copyfile(const char *src, const char *dest) return 1; } - CALLOC(buf, (size_t)CPBUFSIZE, (size_t)1, ret = 1; goto cleanup;); + MALLOC(buf, (size_t)ALPM_BUFFER_SIZE, ret = 1; goto cleanup); /* do the actual file copy */ - while((len = fread(buf, 1, CPBUFSIZE, in))) { + while((len = fread(buf, 1, ALPM_BUFFER_SIZE, in))) { size_t nwritten = 0; nwritten = fwrite(buf, 1, len, out); if((nwritten != len) || ferror(out)) { @@ -174,7 +172,7 @@ int _alpm_copyfile(const char *src, const char *dest) cleanup: fclose(in); fclose(out); - FREE(buf); + free(buf); return ret; } @@ -288,7 +286,7 @@ int _alpm_unpack(alpm_handle_t *handle, const char *archive, const char *prefix, archive_read_support_format_all(_archive); if(archive_read_open_filename(_archive, archive, - ARCHIVE_DEFAULT_BYTES_PER_BLOCK) != ARCHIVE_OK) { + ALPM_BUFFER_SIZE) != ARCHIVE_OK) { _alpm_log(handle, ALPM_LOG_ERROR, _("could not open file %s: %s\n"), archive, archive_error_string(_archive)); RET_ERR(handle, ALPM_ERR_PKG_OPEN, 1); @@ -741,8 +739,6 @@ int _alpm_lstat(const char *path, struct stat *buf) } #ifdef HAVE_LIBSSL -#define BUFFER_SIZE 8192 - static int md5_file(const char *path, unsigned char output[16]) { FILE *f; @@ -750,7 +746,7 @@ static int md5_file(const char *path, unsigned char output[16]) MD5_CTX ctx; unsigned char *buf; - CALLOC(buf, BUFFER_SIZE, sizeof(unsigned char), return 1); + CALLOC(buf, ALPM_BUFFER_SIZE, sizeof(unsigned char), return 1); if((f = fopen(path, "rb")) == NULL) { free(buf); @@ -759,7 +755,7 @@ static int md5_file(const char *path, unsigned char output[16]) MD5_Init(&ctx); - while((n = fread(buf, 1, BUFFER_SIZE, f)) > 0) { + while((n = fread(buf, 1, ALPM_BUFFER_SIZE, f)) > 0) { MD5_Update(&ctx, buf, n); } @@ -785,7 +781,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224) SHA256_CTX ctx; unsigned char *buf; - CALLOC(buf, BUFFER_SIZE, sizeof(unsigned char), return 1); + CALLOC(buf, ALPM_BUFFER_SIZE, sizeof(unsigned char), return 1); if((f = fopen(path, "rb")) == NULL) { free(buf); @@ -798,7 +794,7 @@ static int sha2_file(const char *path, unsigned char output[32], int is224) SHA256_Init(&ctx); } - while((n = fread(buf, 1, BUFFER_SIZE, f)) > 0) { + while((n = fread(buf, 1, ALPM_BUFFER_SIZE, f)) > 0) { if(is224) { SHA224_Update(&ctx, buf, n); } else { diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 2a2d3a93..26fa9044 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -75,6 +75,13 @@ #define CHECK_HANDLE(handle, action) do { if(!(handle)) { action; } (handle)->pm_errno = 0; } while(0) +/** Standard buffer size used throughout the library. */ +#ifdef BUFSIZ +#define ALPM_BUFFER_SIZE BUFSIZ +#else +#define ALPM_BUFFER_SIZE 8192 +#endif + /** * Used as a buffer/state holder for _alpm_archive_fgets(). */ -- cgit v1.2.3-70-g09d2 From f4875fab9bbe3011446032bebcbe232448a3e8d7 Mon Sep 17 00:00:00 2001 From: Dave Reisner Date: Mon, 17 Oct 2011 09:47:06 -0400 Subject: dload: chmod tempfiles to respect umask Dan: fix mask calculation, add it to the success/fail block instead. Signed-off-by: Dave Reisner Signed-off-by: Dan McGee --- lib/libalpm/dload.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'lib') diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 9d919b0a..546329b3 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -178,6 +178,14 @@ static int utimes_long(const char *path, long seconds) return 0; } +/* prefix to avoid possible future clash with getumask(3) */ +static mode_t _getumask(void) +{ + mode_t mask = umask(0); + umask(mask); + return mask; +} + static size_t parse_headers(void *ptr, size_t size, size_t nmemb, void *user) { size_t realsize = size * nmemb; @@ -295,9 +303,12 @@ static FILE *create_tempfile(struct dload_payload *payload, const char *localpat MALLOC(randpath, len, RET_ERR(payload->handle, ALPM_ERR_MEMORY, NULL)); snprintf(randpath, len, "%salpmtmp.XXXXXX", localpath); if((fd = mkstemp(randpath)) == -1 || + fchmod(fd, ~(_getumask()) & 0666) || !(fp = fdopen(fd, payload->tempfile_openmode))) { unlink(randpath); - close(fd); + if(fd >= 0) { + close(fd); + } _alpm_log(payload->handle, ALPM_LOG_ERROR, _("failed to create temporary file for download\n")); return NULL; -- cgit v1.2.3-70-g09d2 From 4c259d51f7e0d5fac0158f70b0ad3b0043930943 Mon Sep 17 00:00:00 2001 From: Dave Reisner Date: Sat, 22 Oct 2011 13:16:39 -0400 Subject: dload: remove redundant conditional Replacing the strdup when after the first NULL check assures that we get continue with payload->remote_name defined. Signed-off-by: Dave Reisner --- lib/libalpm/dload.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 546329b3..efd469d5 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -345,9 +345,10 @@ static int curl_download_internal(struct dload_payload *payload, payload->tempfile_openmode = "wb"; if(!payload->remote_name) { - payload->remote_name = strdup(get_filename(payload->fileurl)); + STRDUP(payload->remote_name, get_filename(payload->fileurl), + RET_ERR(handle, ALPM_ERR_MEMORY, -1)); } - if(!payload->remote_name || curl_gethost(payload->fileurl, hostname, sizeof(hostname)) != 0) { + if(curl_gethost(payload->fileurl, hostname, sizeof(hostname)) != 0) { _alpm_log(handle, ALPM_LOG_ERROR, _("url '%s' is invalid\n"), payload->fileurl); RET_ERR(handle, ALPM_ERR_SERVER_BAD_URL, -1); } -- cgit v1.2.3-70-g09d2