June 2016 - Pacman-dev - lists.archlinux.org

[pacman-dev] [PATCH] Reject files larger than INT_MAX in read_sigfile.
by Tobias Stoeckmann 23 Jun '16

23 Jun '16

Signature files larger than INT_MAX are already suspicious, but if they are larger than SIZE_MAX, this code couldn't even copy them into memory, accepting them as "blank" files at worst. While adding the INT_MAX check, I also rearranged the code to avoid a quite harmless TOCTOU race condition between stat() and fopen(). Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- lib/libalpm/be_package.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index c9ed770..055fb1e 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -700,17 +700,16 @@ static int read_sigfile(const char *sigpath, unsigned char **sig) struct stat st; FILE *fp; - if(stat(sigpath, &st) != 0) { + if((fp = fopen(sigpath, "rb")) == NULL) { return -1; } - MALLOC(*sig, st.st_size, return -1); - - if((fp = fopen(sigpath, "rb")) == NULL) { - free(*sig); + if(fstat(fileno(fp), &st) != 0 || st.st_size > INT_MAX) { return -1; } + MALLOC(*sig, st.st_size, return -1); + if(fread(*sig, st.st_size, 1, fp) != 1) { free(*sig); fclose(fp); -- 2.8.3

4 7

[pacman-dev] [PATCH] Fix out of boundary reads in pacsort.
by Tobias Stoeckmann 18 Jun '16

18 Jun '16

This patch fixes two out of bound reads in pacsort (as they are of the same error type, I didn't see a reason to split it into two commits). The first one is triggered by a very large "key column" and the delimiter '\0'. The return value of strchr() is not checked for the special case in which it reaches the end of the version string. I recommend to simply not allow the '\0' separator, because file names cannot contain it. And therefore, it cannot be a sane column separator in file names for versions either. You can trigger it by running this command: $ echo -e "-.pkg.tar.xz\n-.pkg.tar.xz" | pacsort -k 999999 -t '\0' Segmentation fault The second one is triggered due to a wrong assumption in fnmatch pattern. It is assumed that "*-*.pkg.tar.?z" will only match file names which contain a dash. But in fact, it also accepts files without one, if at least one of the directories contains it. One example would be "/my-path/file.pkg.tar.xz". While parsing the version string, the return value of strrchr() looking for the dash is not checked anymore. In this case, it would return NULL and further pointer arithmetic on it turns it into SIZE_MAX. Eventually, pacman segfaults while traversing this memory area for needed chars: $ echo "-/.pkg.tar.xz" | pacsort Segmentation fault Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- src/util/pacsort.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/util/pacsort.c b/src/util/pacsort.c index 662b250..65f67bd 100644 --- a/src/util/pacsort.c +++ b/src/util/pacsort.c @@ -228,6 +228,9 @@ static struct input_t *input_new(const char *path, int pathlen) } pkgver_end = strrchr(in->pkgname, '-'); + if(pkgver_end == NULL || pkgver_end == in->pkgname) { + return in; + } /* read backwards through pkgrel */ for(in->pkgver = pkgver_end - 1; @@ -410,8 +413,6 @@ static char escape_char(const char *string) return '\n'; case 'v': return '\v'; - case '0': - return '\0'; default: return INVALD_ESCAPE_CHAR; } -- 2.8.3

5 8

[pacman-dev] [PATCH] Release resources on error paths.
by Tobias Stoeckmann 13 Jun '16

13 Jun '16

Some resources (memory or file descriptors) are not released on all error paths. Signed-off-by: Tobias Stoeckmann <tobias(a)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) /* 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); + if(newfiles == NULL) { + goto nomem; + } + 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

3 3

[pacman-dev] [PATCH] pacman-key: reject armored signatures
by Allan McRae 13 Jun '16

13 Jun '16

pacman expectes and unarmored signature. makepkg forces the generation of unarmored signatures, and repo-add will reject any armored signature. For consistency pacman-key should also reject armored signatures. Signed-off-by: Allan McRae <allan(a)archlinux.org> --- scripts/pacman-key.sh.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/pacman-key.sh.in b/scripts/pacman-key.sh.in index 0db0952..ffefc68 100644 --- a/scripts/pacman-key.sh.in +++ b/scripts/pacman-key.sh.in @@ -483,6 +483,10 @@ verify_sig() { local ret=0 for sig; do msg "Checking %s..." "$sig" + if grep -q 'BEGIN PGP SIGNATURE' "$sig"; then + error "$(gettext "Cannot use armored signatures for packages: %s")" "$sig" + return 1 + fi if ! "${GPG_PACMAN[@]}" --status-fd 1 --verify "$sig" | grep -qE '^\[GNUPG:\] TRUST_(FULLY|ULTIMATE).*$'; then error "$(gettext "The signature identified by %s could not be verified.")" "$sig" ret=1 -- 2.8.3

2 2

[pacman-dev] [PATCH] Files that contain references to srcdir and pkgdir are listed
by Ashley Whetter 12 Jun '16

12 Jun '16

Fixes FS#31558 Signed-off-by: Ashley Whetter <ashley(a)awhetter.co.uk> --- .../libmakepkg/lint_package/build_references.sh.in | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/scripts/libmakepkg/lint_package/build_references.sh.in b/scripts/libmakepkg/lint_package/build_references.sh.in index 67c14e6..dd6b88c 100644 --- a/scripts/libmakepkg/lint_package/build_references.sh.in +++ b/scripts/libmakepkg/lint_package/build_references.sh.in @@ -29,10 +29,24 @@ source "$LIBRARY/util/message.sh" lint_package_functions+=('warn_build_references') warn_build_references() { - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${srcdir}" ; then - warning "$(gettext "Package contains reference to %s")" "\$srcdir" + warn_single_build_references "$srcdir" srcdir + warn_single_build_references "$pkgdirbase" pkgdir +} + +warn_single_build_references() { + local to_find=$1 ref_name=$2 + + mapfile -t refs < <(cd "$pkgdir" && find_files_that_ref "$to_find") + if (( ${#refs[*]} )); then + warning "$(gettext "Package contains reference to %s")" "\$$ref_name" + printf " %s\n" "${refs[@]}" fi - if find "${pkgdir}" -type f -print0 | xargs -0 grep -q -I "${pkgdirbase}" ; then - warning "$(gettext "Package contains reference to %s")" "\$pkgdir" +} + +find_files_that_ref() { + if [[ -z "$1" ]]; then + return 1 fi + + find -type f -exec grep -Il "$1" {} + } -- 2.8.2

2 1

[pacman-dev] [PATCH] Can place local source files in directories
by Ashley Whetter 12 Jun '16

12 Jun '16

For example a source file 'mydir/myfile.sh' is now valid, whereas before 'myfile.sh' would have needed to be placed in the same directory as the PKGBUILD. Fixes FS#43020 Signed-off-by: Ashley Whetter <ashley(a)awhetter.co.uk> --- I've tested this by generating checksums, making a package, and making a source package, all with downloadable and local files. Local files were tested in both the PKGBUILD directory and a subdirectory. This doesn't work for files that have been placed under a fake protocol to force makepkg not to include the file in the source package (for example 'file://a_sub_dir/my_huge_source_file.tar.gz'). scripts/libmakepkg/source/file.sh.in | 4 +++- scripts/libmakepkg/source/local.sh.in | 2 +- scripts/libmakepkg/util/source.sh.in | 17 +++++++++++++++++ scripts/makepkg.sh.in | 8 ++++++-- 4 files changed, 27 insertions(+), 4 deletions(-) diff --git a/scripts/libmakepkg/source/file.sh.in b/scripts/libmakepkg/source/file.sh.in index 20493a6..6f17884 100644 --- a/scripts/libmakepkg/source/file.sh.in +++ b/scripts/libmakepkg/source/file.sh.in @@ -87,7 +87,9 @@ extract_file() { local filepath=$(get_filepath "$file") rm -f "$srcdir/${file}" - ln -s "$filepath" "$srcdir/" + local intermediatedirs=$(get_intermediate_dirs "$file") + mkdir -p "$srcdir/$intermediatedirs" + ln -s "$filepath" "$srcdir/$intermediatedirs" if in_array "$file" "${noextract[@]}"; then # skip source files in the noextract=() array diff --git a/scripts/libmakepkg/source/local.sh.in b/scripts/libmakepkg/source/local.sh.in index 6a4b882..ef4829a 100644 --- a/scripts/libmakepkg/source/local.sh.in +++ b/scripts/libmakepkg/source/local.sh.in @@ -33,7 +33,7 @@ download_local() { local filepath=$(get_filepath "$netfile") if [[ -n "$filepath" ]]; then - msg2 "$(gettext "Found %s")" "${filepath##*/}" + msg2 "$(gettext "Found %s")" "$netfile" else local filename=$(get_filename "$netfile") error "$(gettext "%s was not found in the build directory and is not a URL.")" "$filename" diff --git a/scripts/libmakepkg/util/source.sh.in b/scripts/libmakepkg/util/source.sh.in index 9d4ba4a..f16d3c6 100644 --- a/scripts/libmakepkg/util/source.sh.in +++ b/scripts/libmakepkg/util/source.sh.in @@ -74,6 +74,10 @@ get_filename() { filename=${filename%%.git*} fi ;; + local) + # If it is a local file then leave the full path as is + filename=$netfile + ;; *) # if it is just an URL, we only keep the last component filename="${netfile##*/}" @@ -142,3 +146,16 @@ get_downloadclient() { printf "%s\n" "$agent" } + +# Get the directories that exist before a source file +get_intermediate_dirs() { + local netfile=$1 + local intermediatedirs + + if [[ "$(get_protocol "$netfile")" = 'local' ]] && [[ "$netfile" = */* ]] + then + intermediatedirs=${netfile%/*} + fi + + printf "%s\n" "$intermediatedirs" +} diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f80e37a..392b1b3 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1231,8 +1231,12 @@ create_srcpackage() { if [[ "$file" = "$(get_filename "$file")" ]] || (( SOURCEONLY == 2 )); then local absfile absfile=$(get_filepath "$file") || missing_source_file "$file" - msg2 "$(gettext "Adding %s...")" "${absfile##*/}" - ln -s "$absfile" "$srclinks/$pkgbase" + msg2 "$(gettext "Adding %s...")" "$(get_filename "$file")" + + local intermediatedirs="$(get_intermediate_dirs "$file")" + mkdir -p "$srclinks/$pkgbase/$intermediatedirs" + + ln -s "$absfile" "$srclinks/$pkgbase/$intermediatedirs" fi done -- 2.8.0

2 1

[pacman-dev] [PATCH] callback.c: only print download status message once with --noprogressbar
by Ivy Foster 10 Jun '16

10 Jun '16

Syncing repos and downloading packages with --noprogressbar specified often prints the status message "downloading $foo..." to several lines in a row. --- src/pacman/callback.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/pacman/callback.c b/src/pacman/callback.c index ab3e14f..468d657 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -680,8 +680,10 @@ void cb_dl_progress(const char *filename, off_t file_xfered, off_t file_total) if(config->noprogressbar || cols == 0 || file_total == -1) { if(file_xfered == 0) { - printf(_("downloading %s...\n"), filename); + printf(_("downloading %s...\r"), filename); fflush(stdout); + } else if(file_xfered == file_total) { + putchar('\n'); } return; } -- 2.8.3

2 2

[pacman-dev] [PATCH] Fix OOB read and endless loop in signature parser.
by Tobias Stoeckmann 06 Jun '16

06 Jun '16

The parser in alpm_extract_keyid does not properly check boundaries, which makes it vulnerable to malicious input for out ouf bondary reads. I did not attach a proof of concept for this one, because it is very system-specific if you see a segmentation fault or not. If in doubt, you won't notice it. ;) It is also possible to trigger an endless loop with a malicious signature file on 32 bit systems. A proof of concept for this can be triggered through this crafted file (SigLevel must be properly set for local files): $ uname -m i686 $ PKG=package-1.0.tar.xz $ touch $PKG $ echo "iQEcBAABCAAGBQJXTxJiAAr/////+wA=" | base64 -d - > $PKG.sig $ sudo pacman -U $PKG _ This proof of concept uses the ability to overflow position increments. By setting lengths to specific values ((size_t)-5 in this example), the position incrementation effectively ends up at its old position. This patch does not use GCC-specific overflow checking routines, because I am not sure if that's applicable for pacman's design goals. Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- Feedback is very appreciated. :) --- lib/libalpm/signing.c | 46 ++++++++++++++++++++++++++++++++++++---------- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 0267158..7dc7694 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -982,6 +982,21 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist) return 0; } +#define LENGTH_CHECK(l, p, a) \ + do { \ + if ((a) == 0 || (l) - (p) <= (a)) { \ + _alpm_log(handle, ALPM_LOG_ERROR, \ + _("%s: signature format error"), identifier); \ + return -1; \ + } \ + } while(0) + +#define SAFE_ADD(l, p, a) \ + do { \ + LENGTH_CHECK((l), p, (a)); \ + p = p + (a); \ + } while(0) + /** * Extract the Issuer Key ID from a signature * @param sig PGP signature @@ -1018,18 +1033,21 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, switch(sig[pos] & 0x03) { case 0: + LENGTH_CHECK(len, pos, 1); blen = sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); break; case 1: + LENGTH_CHECK(len, pos, 2); blen = (sig[pos + 1] << 8) | sig[pos + 2]; - pos = pos + 3; + SAFE_ADD(len, pos, 3); break; case 2: + LENGTH_CHECK(len, pos, 4); blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4]; - pos = pos + 5; + SAFE_ADD(len, pos, 5); break; case 3: @@ -1046,6 +1064,7 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } + LENGTH_CHECK(len, pos, 1); if(sig[pos + 1] != 0x00) { /* not a signature of a binary document */ _alpm_log(handle, ALPM_LOG_ERROR, @@ -1053,32 +1072,38 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, return -1; } - pos = pos + 4; + SAFE_ADD(len, pos, 4); + LENGTH_CHECK(len, pos, 1); hlen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + hlen + 2; + SAFE_ADD(len, pos, hlen + 2); + LENGTH_CHECK(len, pos, 1); ulen = (sig[pos] << 8) | sig[pos + 1]; - pos = pos + 2; + SAFE_ADD(len, pos, 2); spos = pos; + LENGTH_CHECK(len, pos, ulen); while(spos < pos + ulen) { if(sig[spos] < 192) { slen = sig[spos]; spos = spos + 1; } else if(sig[spos] < 255) { + LENGTH_CHECK(pos + ulen, spos, 1); slen = (sig[spos] << 8) | sig[spos + 1]; - spos = spos + 2; + SAFE_ADD(pos + ulen, spos, 2); } else { + LENGTH_CHECK(pos + ulen, spos, 4); slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4]; - spos = spos + 5; + SAFE_ADD(pos + ulen, spos, 5); } if(sig[spos] == 16) { /* issuer key ID */ char key[17]; size_t i; + LENGTH_CHECK(pos + ulen, spos, 8); for (i = 0; i < 8; i++) { sprintf(&key[i * 2], "%02X", sig[spos + i + 1]); } @@ -1086,10 +1111,11 @@ int SYMEXPORT alpm_extract_keyid(alpm_handle_t *handle, const char *identifier, break; } - spos = spos + slen; + SAFE_ADD(pos + ulen + 1, spos, slen); } - pos = pos + (blen - hlen - 8); + LENGTH_CHECK(blen, hlen, 8); + SAFE_ADD(len + 1, pos, blen - hlen - 8); } return 0; -- 2.8.3

1 0

[pacman-dev] [PATCH] Always use proper error code in alpm_initialize.
by Tobias Stoeckmann 06 Jun '16

06 Jun '16

In out of memory conditions, an undefined error value is written into *err, because myerr is never explicitly set in these cases. I have also converted a calloc into a MALLOC call, because the memory will be properly filled by the snprintf call right after it. Signed-off-by: Tobias Stoeckmann <tobias(a)stoeckmann.org> --- lib/libalpm/alpm.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c index 6f02913..6b7fa7a 100644 --- a/lib/libalpm/alpm.c +++ b/lib/libalpm/alpm.c @@ -55,8 +55,7 @@ alpm_handle_t SYMEXPORT *alpm_initialize(const char *root, const char *dbpath, alpm_handle_t *myhandle = _alpm_handle_new(); if(myhandle == NULL) { - myerr = ALPM_ERR_MEMORY; - goto cleanup; + goto nomem; } if((myerr = _alpm_set_directory_option(root, &(myhandle->root), 1))) { goto cleanup; @@ -68,15 +67,15 @@ alpm_handle_t SYMEXPORT *alpm_initialize(const char *root, const char *dbpath, /* to contatenate myhandle->root (ends with a slash) with SYSHOOKDIR (starts * with a slash) correctly, we skip SYSHOOKDIR[0]; the regular +1 therefore * disappears from the allocation */ - MALLOC(hookdir, strlen(myhandle->root) + strlen(SYSHOOKDIR), goto cleanup); + MALLOC(hookdir, strlen(myhandle->root) + strlen(SYSHOOKDIR), goto nomem); sprintf(hookdir, "%s%s", myhandle->root, SYSHOOKDIR + 1); myhandle->hookdirs = alpm_list_add(NULL, hookdir); /* set default database extension */ - STRDUP(myhandle->dbext, ".db", goto cleanup); + STRDUP(myhandle->dbext, ".db", goto nomem); lockfilelen = strlen(myhandle->dbpath) + strlen(lf) + 1; - myhandle->lockfile = calloc(lockfilelen, sizeof(char)); + MALLOC(myhandle->lockfile, lockfilelen, goto nomem); snprintf(myhandle->lockfile, lockfilelen, "%s%s", myhandle->dbpath, lf); if(_alpm_db_register_local(myhandle) == NULL) { @@ -90,9 +89,11 @@ alpm_handle_t SYMEXPORT *alpm_initialize(const char *root, const char *dbpath, return myhandle; +nomem: + myerr = ALPM_ERR_MEMORY; cleanup: _alpm_handle_free(myhandle); - if(err && myerr) { + if(err) { *err = myerr; } return NULL; -- 2.8.3

2 1

[pacman-dev] [PATCH] recursedeps: include cyclic dependencies
by Andrew Gregory 04 Jun '16

04 Jun '16

Cyclic dependencies (A depends on B, B depends on A) were not selected because neither package could be removed individually, so can_remove_package would always return false for both. By preselecting all dependencies then filtering back out any dependencies still required by any packages that will not be uninstalled, groups of unneeded cyclic dependencies can be found. Fixes FS#41031 Signed-off-by: Andrew Gregory <andrew.gregory.8(a)gmail.com> --- lib/libalpm/deps.c | 104 ++++++++++++++-------------- test/pacman/tests/TESTS | 1 + test/pacman/tests/remove-recursive-cycle.py | 41 +++++++++++ 3 files changed, 94 insertions(+), 52 deletions(-) create mode 100644 test/pacman/tests/remove-recursive-cycle.py diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index c35a275..ae5d60b 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -551,44 +551,29 @@ error: return NULL; } -/* These parameters are messy. We check if this package, given a list of - * targets and a db is safe to remove. We do NOT remove it if it is in the - * target list, or if the package was explicitly installed and - * include_explicit == 0 */ -static int can_remove_package(alpm_db_t *db, alpm_pkg_t *pkg, - alpm_list_t *targets, int include_explicit) +/** Move package dependencies from one list to another + * @param from list to scan for dependencies + * @param to list to add dependencies to + * @param pkg package whose dependencies are moved + * @param explicit if 0, explicitly installed packages are not moved + */ +static void _alpm_select_depends(alpm_list_t **from, alpm_list_t **to, + alpm_pkg_t *pkg, int explicit) { - alpm_list_t *i; - - if(alpm_pkg_find(targets, pkg->name)) { - return 0; - } - - if(!include_explicit) { - /* see if it was explicitly installed */ - if(alpm_pkg_get_reason(pkg) == ALPM_PKG_REASON_EXPLICIT) { - _alpm_log(db->handle, ALPM_LOG_DEBUG, - "excluding %s -- explicitly installed\n", pkg->name); - return 0; + alpm_list_t *i, *next; + if(!alpm_pkg_get_depends(pkg)) { + return; + } + for(i = *from; i; i = next) { + alpm_pkg_t *deppkg = i->data; + next = i->next; + if((explicit || alpm_pkg_get_reason(deppkg) != ALPM_PKG_REASON_EXPLICIT) + && _alpm_pkg_depends_on(pkg, deppkg)) { + *to = alpm_list_add(*to, deppkg); + *from = alpm_list_remove_item(*from, i); + free(i); } } - - /* TODO: checkdeps could be used here, it handles multiple providers - * better, but that also makes it slower. - * Also this would require to first add the package to the targets list, - * then call checkdeps with it, then remove the package from the targets list - * if checkdeps detected it would break something */ - - /* see if other packages need it */ - for(i = _alpm_db_get_pkgcache(db); i; i = i->next) { - alpm_pkg_t *lpkg = i->data; - if(_alpm_pkg_depends_on(lpkg, pkg) && !alpm_pkg_find(targets, lpkg->name)) { - return 0; - } - } - - /* it's ok to remove */ - return 1; } /** @@ -604,31 +589,46 @@ static int can_remove_package(alpm_db_t *db, alpm_pkg_t *pkg, */ int _alpm_recursedeps(alpm_db_t *db, alpm_list_t **targs, int include_explicit) { - alpm_list_t *i, *j; + alpm_list_t *i, *keep, *rem = NULL; if(db == NULL || targs == NULL) { return -1; } + keep = alpm_list_copy(_alpm_db_get_pkgcache(db)); for(i = *targs; i; i = i->next) { - alpm_pkg_t *pkg = i->data; - for(j = _alpm_db_get_pkgcache(db); j; j = j->next) { - alpm_pkg_t *deppkg = j->data; - if(_alpm_pkg_depends_on(pkg, deppkg) - && can_remove_package(db, deppkg, *targs, include_explicit)) { - alpm_pkg_t *copy = NULL; - _alpm_log(db->handle, ALPM_LOG_DEBUG, "adding '%s' to the targets\n", - deppkg->name); - /* add it to the target list */ - if(_alpm_pkg_dup(deppkg, &copy)) { - /* we return memory on "non-fatal" error in _alpm_pkg_dup */ - _alpm_pkg_free(copy); - return -1; - } - *targs = alpm_list_add(*targs, copy); - } + keep = alpm_list_remove(keep, i->data, _alpm_pkg_cmp, NULL); + } + + /* recursively select all dependencies for removal */ + for(i = *targs; i; i = i->next) { + _alpm_select_depends(&keep, &rem, i->data, include_explicit); + } + for(i = rem; i; i = i->next) { + _alpm_select_depends(&keep, &rem, i->data, include_explicit); + } + + /* recursively select any still needed packages to keep */ + for(i = keep; i && rem; i = i->next) { + _alpm_select_depends(&rem, &keep, i->data, 1); + } + alpm_list_free(keep); + + /* copy selected packages into the target list */ + for(i = rem; i; i = i->next) { + alpm_pkg_t *pkg = i->data, *copy = NULL; + _alpm_log(db->handle, ALPM_LOG_DEBUG, + "adding '%s' to the targets\n", pkg->name); + if(_alpm_pkg_dup(pkg, &copy)) { + /* we return memory on "non-fatal" error in _alpm_pkg_dup */ + _alpm_pkg_free(copy); + alpm_list_free(rem); + return -1; } + *targs = alpm_list_add(*targs, copy); } + alpm_list_free(rem); + return 0; } diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index 62d1f2a..bd5a0b6 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -109,6 +109,7 @@ TESTS += test/pacman/tests/querycheck002.py TESTS += test/pacman/tests/querycheck_fast_file_type.py TESTS += test/pacman/tests/reason001.py TESTS += test/pacman/tests/remove-assumeinstalled.py +TESTS += test/pacman/tests/remove-recursive-cycle.py TESTS += test/pacman/tests/remove001.py TESTS += test/pacman/tests/remove002.py TESTS += test/pacman/tests/remove010.py diff --git a/test/pacman/tests/remove-recursive-cycle.py b/test/pacman/tests/remove-recursive-cycle.py new file mode 100644 index 0000000..b9864c8 --- /dev/null +++ b/test/pacman/tests/remove-recursive-cycle.py @@ -0,0 +1,41 @@ +self.description = "Recursively remove a package with cyclical dependencies" + +lpkg1 = pmpkg('pkg1') +self.addpkg2db('local', lpkg1) +lpkg1.depends = [ 'dep1' ] + +lpkg2 = pmpkg('pkg2') +self.addpkg2db('local', lpkg2) +lpkg2.depends = [ 'dep3' ] + +# cyclic dependency 1 +ldep1 = pmpkg('dep1') +self.addpkg2db('local', ldep1) +ldep1.depends = [ 'dep2', 'dep3', 'dep4' ] +ldep1.reason = 1 + +# cyclic dependency 2 +ldep2 = pmpkg('dep2') +self.addpkg2db('local', ldep2) +ldep2.depends = [ 'dep1' ] +ldep2.reason = 1 + +# dependency required by another package +ldep3 = pmpkg('dep3') +self.addpkg2db('local', ldep3) +ldep3.reason = 1 + +# explicitly installed dependency +ldep4 = pmpkg('dep4') +self.addpkg2db('local', ldep4) +ldep4.reason = 0 + +self.args = "-Rs pkg1" + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PKG_EXIST=pkg2") +self.addrule("PKG_EXIST=dep3") +self.addrule("PKG_EXIST=dep4") +self.addrule("!PKG_EXIST=pkg1") +self.addrule("!PKG_EXIST=dep1") +self.addrule("!PKG_EXIST=dep2") -- 2.8.2

2 1