[pacman-dev] [PATCH] remove: use strcmp for files in skip_remove
Paths from noupgrade, the transaction skip_remove, and package backup lists were combined into a single list matched using fnmatch causing paths with glob characters to match unrelated files. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/remove.c | 82 ++++++++++-------------- test/pacman/tests/skip-remove-with-glob-chars.py | 19 ++++++ 2 files changed, 52 insertions(+), 49 deletions(-) create mode 100644 test/pacman/tests/skip-remove-with-glob-chars.py diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index f9b24ef..42978ae 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -317,20 +317,13 @@ static int dir_is_mountpoint(alpm_handle_t *handle, const char *directory, * * @param handle the context handle * @param file file to be removed - * @param skip_remove list of files that will not be removed * * @return 1 if the file can be deleted, 0 if it cannot be deleted */ -static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file, - alpm_list_t *skip_remove) +static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file) { char filepath[PATH_MAX]; - if(_alpm_fnmatch_patterns(skip_remove, file->name) == 0) { - /* return success because we will never actually remove this file */ - return 1; - } - snprintf(filepath, PATH_MAX, "%s%s", handle->root, file->name); if(file->name[strlen(file->name) - 1] == '/' && @@ -433,30 +426,19 @@ cleanup: * @param oldpkg the package being removed * @param newpkg the package replacing \a oldpkg * @param fileobj file to remove - * @param skip_remove list of files that shouldn't be removed * @param nosave whether files should be backed up * * @return 0 on success, -1 if there was an error unlinking the file, 1 if the * file was skipped or did not exist */ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, - alpm_pkg_t *newpkg, const alpm_file_t *fileobj, alpm_list_t *skip_remove, - int nosave) + alpm_pkg_t *newpkg, const alpm_file_t *fileobj, int nosave) { struct stat buf; char file[PATH_MAX]; snprintf(file, PATH_MAX, "%s%s", handle->root, fileobj->name); - /* check the remove skip list before removing the file. - * see the big comment block in db_find_fileconflicts() for an - * explanation. */ - if(_alpm_fnmatch_patterns(skip_remove, fileobj->name) == 0) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "%s is in skip_remove, skipping removal\n", file); - return 1; - } - if(llstat(file, &buf)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file); return 1; @@ -564,6 +546,24 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, } /** + * @brief Check if a package file should be removed. + * + * @param handle the context handle + * @param newpkg the package replacing the current owner of \a path + * @param path file to be removed + * + * @return 1 if the file should be skipped, 0 if it should be removed + */ +static int _should_skip_file(alpm_handle_t *handle, + alpm_pkg_t *newpkg, const char *path) +{ + return _alpm_fnmatch_patterns(handle->noupgrade, path) == 0 + || alpm_list_find_str(handle->trans->skip_remove, path) + || (newpkg && _alpm_needbackup(path, newpkg) + && alpm_filelist_contains(alpm_pkg_get_files(newpkg), path)); +} + +/** * @brief Remove a package's files, optionally skipping its replacement's * files. * @@ -580,44 +580,19 @@ static int remove_package_files(alpm_handle_t *handle, alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg, size_t targ_count, size_t pkg_count) { - alpm_list_t *skip_remove; alpm_filelist_t *filelist; size_t i; int err = 0; int nosave = handle->trans->flags & ALPM_TRANS_FLAG_NOSAVE; - if(newpkg) { - alpm_filelist_t *newfiles; - alpm_list_t *b; - skip_remove = alpm_list_join( - alpm_list_strdup(handle->trans->skip_remove), - alpm_list_strdup(handle->noupgrade)); - /* Add files in the NEW backup array to the skip_remove array - * so this removal operation doesn't kill them */ - /* old package backup list */ - newfiles = alpm_pkg_get_files(newpkg); - for(b = alpm_pkg_get_backup(newpkg); b; b = b->next) { - const alpm_backup_t *backup = b->data; - /* safety check (fix the upgrade026 pactest) */ - if(!alpm_filelist_contains(newfiles, backup->name)) { - continue; - } - _alpm_log(handle, ALPM_LOG_DEBUG, "adding %s to the skip_remove array\n", - backup->name); - skip_remove = alpm_list_add(skip_remove, strdup(backup->name)); - } - } else { - skip_remove = alpm_list_strdup(handle->trans->skip_remove); - } - filelist = alpm_pkg_get_files(oldpkg); for(i = 0; i < filelist->count; i++) { alpm_file_t *file = filelist->files + i; - if(!can_remove_file(handle, file, skip_remove)) { + if(!_should_skip_file(handle, newpkg, file->name) + && !can_remove_file(handle, file)) { _alpm_log(handle, ALPM_LOG_DEBUG, "not removing package '%s', can't remove all files\n", oldpkg->name); - FREELIST(skip_remove); RET_ERR(handle, ALPM_ERR_PKG_CANT_REMOVE, -1); } } @@ -633,7 +608,17 @@ static int remove_package_files(alpm_handle_t *handle, /* iterate through the list backwards, unlinking files */ for(i = filelist->count; i > 0; i--) { alpm_file_t *file = filelist->files + i - 1; - if(unlink_file(handle, oldpkg, newpkg, file, skip_remove, nosave) < 0) { + + /* check the remove skip list before removing the file. + * see the big comment block in db_find_fileconflicts() for an + * explanation. */ + if(_should_skip_file(handle, newpkg, file->name)) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "%s is in skip_remove, skipping removal\n", file->name); + continue; + } + + if(unlink_file(handle, oldpkg, newpkg, file, nosave) < 0) { err++; } @@ -644,7 +629,6 @@ static int remove_package_files(alpm_handle_t *handle, percent, pkg_count, targ_count); } } - FREELIST(skip_remove); if(!newpkg) { /* set progress to 100% after we finish unlinking files */ diff --git a/test/pacman/tests/skip-remove-with-glob-chars.py b/test/pacman/tests/skip-remove-with-glob-chars.py new file mode 100644 index 0000000..039e150 --- /dev/null +++ b/test/pacman/tests/skip-remove-with-glob-chars.py @@ -0,0 +1,19 @@ +self.description = "transferred file with glob characters that match a removed file" + +lp = pmpkg("foo") +lp.files = ["foo/b*r", "foo/bar"] +self.addpkg2db("local", lp) + +sp1 = pmpkg("foo", "1.0-2") +self.addpkg(sp1) + +sp2 = pmpkg("bar", "1.0-2") +sp2.files = ["foo/b*r"] +self.addpkg(sp2) + +self.args = "-U %s %s" % (sp1.filename(), sp2.filename()) + +self.addrule("PKG_VERSION=foo|1.0-2") +self.addrule("PKG_VERSION=bar|1.0-2") +self.addrule("FILE_EXIST=foo/b*r") +self.addrule("!FILE_EXIST=foo/bar") -- 2.1.3
On 30/11/14 09:02, Andrew Gregory wrote:
Paths from noupgrade, the transaction skip_remove, and package backup lists were combined into a single list matched using fnmatch causing paths with glob characters to match unrelated files.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com>
I saw this on your 4.2-queue branch, but it was too big to be included at this late stage. Delayed for next release. Allan
On 30/11/14 09:02, Andrew Gregory wrote:
Paths from noupgrade, the transaction skip_remove, and package backup lists were combined into a single list matched using fnmatch causing paths with glob characters to match unrelated files.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/remove.c | 82 ++++++++++-------------- test/pacman/tests/skip-remove-with-glob-chars.py | 19 ++++++ 2 files changed, 52 insertions(+), 49 deletions(-) create mode 100644 test/pacman/tests/skip-remove-with-glob-chars.py
diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index f9b24ef..42978ae 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -317,20 +317,13 @@ static int dir_is_mountpoint(alpm_handle_t *handle, const char *directory, * * @param handle the context handle * @param file file to be removed - * @param skip_remove list of files that will not be removed * * @return 1 if the file can be deleted, 0 if it cannot be deleted */ -static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file, - alpm_list_t *skip_remove) +static int can_remove_file(alpm_handle_t *handle, const alpm_file_t *file) { char filepath[PATH_MAX];
- if(_alpm_fnmatch_patterns(skip_remove, file->name) == 0) { - /* return success because we will never actually remove this file */ - return 1; - } - snprintf(filepath, PATH_MAX, "%s%s", handle->root, file->name);
OK
if(file->name[strlen(file->name) - 1] == '/' && @@ -433,30 +426,19 @@ cleanup: * @param oldpkg the package being removed * @param newpkg the package replacing \a oldpkg * @param fileobj file to remove - * @param skip_remove list of files that shouldn't be removed * @param nosave whether files should be backed up * * @return 0 on success, -1 if there was an error unlinking the file, 1 if the * file was skipped or did not exist */ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, - alpm_pkg_t *newpkg, const alpm_file_t *fileobj, alpm_list_t *skip_remove, - int nosave) + alpm_pkg_t *newpkg, const alpm_file_t *fileobj, int nosave) { struct stat buf; char file[PATH_MAX];
snprintf(file, PATH_MAX, "%s%s", handle->root, fileobj->name);
- /* check the remove skip list before removing the file. - * see the big comment block in db_find_fileconflicts() for an - * explanation. */ - if(_alpm_fnmatch_patterns(skip_remove, fileobj->name) == 0) { - _alpm_log(handle, ALPM_LOG_DEBUG, - "%s is in skip_remove, skipping removal\n", file); - return 1; - } - if(llstat(file, &buf)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file); return 1;
OK.
@@ -564,6 +546,24 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, }
/** + * @brief Check if a package file should be removed. + * + * @param handle the context handle + * @param newpkg the package replacing the current owner of \a path + * @param path file to be removed + * + * @return 1 if the file should be skipped, 0 if it should be removed + */ +static int _should_skip_file(alpm_handle_t *handle,
Remove underscore at start of name
+ alpm_pkg_t *newpkg, const char *path) +{ + return _alpm_fnmatch_patterns(handle->noupgrade, path) == 0 + || alpm_list_find_str(handle->trans->skip_remove, path)
3af0268f -> _alpm_fnmatch_patterns instead of alpm_list_find_str Hrm... If I make this change tests fail all over the place. I have not investigated further.
+ || (newpkg && _alpm_needbackup(path, newpkg) + && alpm_filelist_contains(alpm_pkg_get_files(newpkg), path)); +} + +/** * @brief Remove a package's files, optionally skipping its replacement's * files. * @@ -580,44 +580,19 @@ static int remove_package_files(alpm_handle_t *handle, alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg, size_t targ_count, size_t pkg_count) { - alpm_list_t *skip_remove; alpm_filelist_t *filelist; size_t i; int err = 0; int nosave = handle->trans->flags & ALPM_TRANS_FLAG_NOSAVE;
- if(newpkg) { - alpm_filelist_t *newfiles; - alpm_list_t *b; - skip_remove = alpm_list_join( - alpm_list_strdup(handle->trans->skip_remove), - alpm_list_strdup(handle->noupgrade)); - /* Add files in the NEW backup array to the skip_remove array - * so this removal operation doesn't kill them */ - /* old package backup list */ - newfiles = alpm_pkg_get_files(newpkg); - for(b = alpm_pkg_get_backup(newpkg); b; b = b->next) { - const alpm_backup_t *backup = b->data; - /* safety check (fix the upgrade026 pactest) */ - if(!alpm_filelist_contains(newfiles, backup->name)) { - continue; - } - _alpm_log(handle, ALPM_LOG_DEBUG, "adding %s to the skip_remove array\n", - backup->name); - skip_remove = alpm_list_add(skip_remove, strdup(backup->name)); - } - } else { - skip_remove = alpm_list_strdup(handle->trans->skip_remove); - } -
OK
filelist = alpm_pkg_get_files(oldpkg); for(i = 0; i < filelist->count; i++) { alpm_file_t *file = filelist->files + i; - if(!can_remove_file(handle, file, skip_remove)) { + if(!_should_skip_file(handle, newpkg, file->name) + && !can_remove_file(handle, file)) { _alpm_log(handle, ALPM_LOG_DEBUG, "not removing package '%s', can't remove all files\n", oldpkg->name); - FREELIST(skip_remove); RET_ERR(handle, ALPM_ERR_PKG_CANT_REMOVE, -1); } } @@ -633,7 +608,17 @@ static int remove_package_files(alpm_handle_t *handle, /* iterate through the list backwards, unlinking files */ for(i = filelist->count; i > 0; i--) { alpm_file_t *file = filelist->files + i - 1; - if(unlink_file(handle, oldpkg, newpkg, file, skip_remove, nosave) < 0) { + + /* check the remove skip list before removing the file. + * see the big comment block in db_find_fileconflicts() for an + * explanation. */ + if(_should_skip_file(handle, newpkg, file->name)) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "%s is in skip_remove, skipping removal\n", file->name); + continue; + } + + if(unlink_file(handle, oldpkg, newpkg, file, nosave) < 0) { err++; }
OK
@@ -644,7 +629,6 @@ static int remove_package_files(alpm_handle_t *handle, percent, pkg_count, targ_count); } } - FREELIST(skip_remove);
if(!newpkg) { /* set progress to 100% after we finish unlinking files */ diff --git a/test/pacman/tests/skip-remove-with-glob-chars.py b/test/pacman/tests/skip-remove-with-glob-chars.py new file mode 100644 index 0000000..039e150 --- /dev/null +++ b/test/pacman/tests/skip-remove-with-glob-chars.py @@ -0,0 +1,19 @@ +self.description = "transferred file with glob characters that match a removed file" + +lp = pmpkg("foo") +lp.files = ["foo/b*r", "foo/bar"] +self.addpkg2db("local", lp) + +sp1 = pmpkg("foo", "1.0-2") +self.addpkg(sp1) + +sp2 = pmpkg("bar", "1.0-2") +sp2.files = ["foo/b*r"] +self.addpkg(sp2) + +self.args = "-U %s %s" % (sp1.filename(), sp2.filename()) + +self.addrule("PKG_VERSION=foo|1.0-2") +self.addrule("PKG_VERSION=bar|1.0-2") +self.addrule("FILE_EXIST=foo/b*r") +self.addrule("!FILE_EXIST=foo/bar")
Needs added to TESTS. One backlog review of your patches done, two to go... A
On 03/03/15 at 04:10pm, Allan McRae wrote:
On 30/11/14 09:02, Andrew Gregory wrote:
Paths from noupgrade, the transaction skip_remove, and package backup lists were combined into a single list matched using fnmatch causing paths with glob characters to match unrelated files.
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/remove.c | 82 ++++++++++-------------- test/pacman/tests/skip-remove-with-glob-chars.py | 19 ++++++ 2 files changed, 52 insertions(+), 49 deletions(-) create mode 100644 test/pacman/tests/skip-remove-with-glob-chars.py
<snip>
@@ -564,6 +546,24 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, }
/** + * @brief Check if a package file should be removed. + * + * @param handle the context handle + * @param newpkg the package replacing the current owner of \a path + * @param path file to be removed + * + * @return 1 if the file should be skipped, 0 if it should be removed + */ +static int _should_skip_file(alpm_handle_t *handle,
Remove underscore at start of name
Updated on my bugfix branch.
+ alpm_pkg_t *newpkg, const char *path) +{ + return _alpm_fnmatch_patterns(handle->noupgrade, path) == 0 + || alpm_list_find_str(handle->trans->skip_remove, path)
3af0268f -> _alpm_fnmatch_patterns instead of alpm_list_find_str
Hrm... If I make this change tests fail all over the place. I have not investigated further.
This is correct as-is. skip_remove should be searched using find_str because it contains paths, not globs.
+ || (newpkg && _alpm_needbackup(path, newpkg) + && alpm_filelist_contains(alpm_pkg_get_files(newpkg), path)); +} + +/** * @brief Remove a package's files, optionally skipping its replacement's * files. *
<snip>
diff --git a/test/pacman/tests/skip-remove-with-glob-chars.py b/test/pacman/tests/skip-remove-with-glob-chars.py new file mode 100644 index 0000000..039e150 --- /dev/null +++ b/test/pacman/tests/skip-remove-with-glob-chars.py @@ -0,0 +1,19 @@ +self.description = "transferred file with glob characters that match a removed file" + +lp = pmpkg("foo") +lp.files = ["foo/b*r", "foo/bar"] +self.addpkg2db("local", lp) + +sp1 = pmpkg("foo", "1.0-2") +self.addpkg(sp1) + +sp2 = pmpkg("bar", "1.0-2") +sp2.files = ["foo/b*r"] +self.addpkg(sp2) + +self.args = "-U %s %s" % (sp1.filename(), sp2.filename()) + +self.addrule("PKG_VERSION=foo|1.0-2") +self.addrule("PKG_VERSION=bar|1.0-2") +self.addrule("FILE_EXIST=foo/b*r") +self.addrule("!FILE_EXIST=foo/bar")
Needs added to TESTS.
Updated on my bugfix branch. apg
participants (2)
-
Allan McRae
-
Andrew Gregory