[pacman-dev] [PATCH 1/3] 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 <dan@archlinux.org> --- For maint; so valgrind messages can stay cleaner and it is a pretty low-risk, obvious fix. lib/libalpm/be_package.c | 18 ++++++++++-------- 1 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c20c703..c8e08e2 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; -- 1.7.7
These should all prevent installation, and yet two of the three tests currently fail. Not good. The best way to see what is going on here is to diff the three new tests side by side- there is only a small difference between the three tests, and that is in the destination of the symlink in question that should never be overwritten. symlink010.py: myprogsuffix -> myprog symlink011.py: myprogsuffix -> broken symlink012.py: myprogsuffix -> otherprog Signed-off-by: Dan McGee <dan@archlinux.org> --- For maint. test/pacman/tests/fileconflict007.py | 2 +- test/pacman/tests/symlink010.py | 26 ++++++++++++++++++++++++++ test/pacman/tests/symlink011.py | 26 ++++++++++++++++++++++++++ test/pacman/tests/symlink012.py | 24 ++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletions(-) create mode 100644 test/pacman/tests/symlink010.py create mode 100644 test/pacman/tests/symlink011.py create mode 100644 test/pacman/tests/symlink012.py diff --git a/test/pacman/tests/fileconflict007.py b/test/pacman/tests/fileconflict007.py index 7e6d85e..4ee4624 100644 --- a/test/pacman/tests/fileconflict007.py +++ b/test/pacman/tests/fileconflict007.py @@ -3,7 +3,7 @@ lp = pmpkg("pkg") lp.files = ["dir/realdir/", "dir/symdir -> realdir", - "dir/realdir/file"] + "dir/realdir/file"] self.addpkg2db("local", lp) p = pmpkg("pkg", "1.0-2") diff --git a/test/pacman/tests/symlink010.py b/test/pacman/tests/symlink010.py new file mode 100644 index 0000000..8c80dbc --- /dev/null +++ b/test/pacman/tests/symlink010.py @@ -0,0 +1,26 @@ +self.description = "Unowned identical symlink pointing to file in package" + +lp = pmpkg("dummy") +lp.files = ["usr/bin/myprog"] +self.addpkg2db("local", lp) + +self.filesystem = ["usr/bin/otherprog", + "usr/bin/myprogsuffix -> myprog"] + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/myprog", + "usr/bin/myprogsuffix -> myprog"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=dummy|1.0-1") +self.addrule("FILE_EXIST=usr/bin/myprog") +self.addrule("LINK_EXIST=usr/bin/myprogsuffix") +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 new file mode 100644 index 0000000..8510172 --- /dev/null +++ b/test/pacman/tests/symlink011.py @@ -0,0 +1,26 @@ +self.description = "Unowned broken symlink replaced by one in package" + +lp = pmpkg("dummy") +lp.files = ["usr/bin/myprog"] +self.addpkg2db("local", lp) + +self.filesystem = ["usr/bin/otherprog", + "usr/bin/myprogsuffix -> broken"] + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/myprog", + "usr/bin/myprogsuffix -> myprog"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=dummy|1.0-1") +self.addrule("FILE_EXIST=usr/bin/myprog") +self.addrule("LINK_EXIST=usr/bin/myprogsuffix") +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/symlink012.py b/test/pacman/tests/symlink012.py new file mode 100644 index 0000000..6a73bbb --- /dev/null +++ b/test/pacman/tests/symlink012.py @@ -0,0 +1,24 @@ +self.description = "Unowned symlink when pointing to different file" + +lp = pmpkg("dummy") +lp.files = ["usr/bin/myprog"] +self.addpkg2db("local", lp) + +self.filesystem = ["usr/bin/otherprog", + "usr/bin/myprogsuffix -> otherprog"] + +p = pmpkg("dummy", "1.0-2") +p.files = ["usr/bin/myprog", + "usr/bin/myprogsuffix -> myprog"] +self.addpkg(p) + +self.args = "-U %s" % p.filename() + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PKG_VERSION=dummy|1.0-1") +self.addrule("FILE_EXIST=usr/bin/myprog") +self.addrule("LINK_EXIST=usr/bin/myprogsuffix") +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") -- 1.7.7
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 <dan@archlinux.org> --- For maint. All other tests still pass too. :) lib/libalpm/conflict.c | 33 +++++++++++++++++++-------------- test/pacman/tests/symlink010.py | 2 -- test/pacman/tests/symlink011.py | 2 -- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 14c23f4..32f6f30 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 8c80dbc..a1e562e 100644 --- a/test/pacman/tests/symlink010.py +++ b/test/pacman/tests/symlink010.py @@ -22,5 +22,3 @@ 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 8510172..93cd9dd 100644 --- a/test/pacman/tests/symlink011.py +++ b/test/pacman/tests/symlink011.py @@ -22,5 +22,3 @@ 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 -- 1.7.7
participants (1)
-
Dan McGee