| Message ID | 20170409234917.28781-1-andrew.gregory.8@gmail.com |
|---|---|
| State | Accepted, archived |
| Headers | show |
On 10/04/17 09:49, Andrew Gregory wrote: > Subtle problems like this could be avoided by removing the trailing slashes > from libalpm's internal file lists. Is there any reason not to remove them and > use a field in the file_t struct to indicate type? It already has a mode field > that we could use. At the moment, that is the only way we know something should be a directory from the local db. If the local db contained that information in another format, I'd be happy to remove the trailing slash. A
On 12/04/17 12:00, Allan McRae wrote: > On 10/04/17 09:49, Andrew Gregory wrote: >> Subtle problems like this could be avoided by removing the trailing slashes >> from libalpm's internal file lists. Is there any reason not to remove them and >> use a field in the file_t struct to indicate type? It already has a mode field >> that we could use. > > At the moment, that is the only way we know something should be a > directory from the local db. If the local db contained that information > in another format, I'd be happy to remove the trailing slash. > I forgot about mtree files! There a possibility of a package being installed for a long time with no mtree file. To make this change to the local database files, we would need to enforce mtree files being present. At least one release with a deprecation message. A
On 04/12/17 at 12:08pm, Allan McRae wrote: > On 12/04/17 12:00, Allan McRae wrote: > > On 10/04/17 09:49, Andrew Gregory wrote: > >> Subtle problems like this could be avoided by removing the trailing slashes > >> from libalpm's internal file lists. Is there any reason not to remove them and > >> use a field in the file_t struct to indicate type? It already has a mode field > >> that we could use. > > > > At the moment, that is the only way we know something should be a > > directory from the local db. If the local db contained that information > > in another format, I'd be happy to remove the trailing slash. > > I forgot about mtree files! > > There a possibility of a package being installed for a long time with no > mtree file. To make this change to the local database files, we would > need to enforce mtree files being present. At least one release with a > deprecation message. We should be able to leave the db format alone. I think all we need to do is have the various filelist builders strip the trailing slash, or not add it as the case may be. The major drawback that I see is breaking libalpm front-ends that rely on the current behavior. apg
diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 8ce10f84..ffe92518 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -440,8 +440,19 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, { struct stat buf; char file[PATH_MAX]; + int file_len; - snprintf(file, PATH_MAX, "%s%s", handle->root, fileobj->name); + file_len = snprintf(file, PATH_MAX, "%s%s", handle->root, fileobj->name); + if(file_len <= 0 || file_len >= PATH_MAX) { + /* 0 is a valid value from snprintf, but should be impossible here */ + _alpm_log(handle, ALPM_LOG_DEBUG, "path too long to unlink %s%s\n", + handle->root, fileobj->name); + return -1; + } else if(file[file_len-1] == '/') { + /* trailing slashes cause errors and confusing messages if the user has + * replaced a directory with a symlink */ + file[--file_len] = '\0'; + } if(llstat(file, &buf)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file); diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index 11d1c38c..f926e162 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -110,6 +110,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-directory-replaced-with-symlink.py TESTS += test/pacman/tests/remove-optdepend-of-installed-package.py TESTS += test/pacman/tests/remove-recursive-cycle.py TESTS += test/pacman/tests/remove001.py diff --git a/test/pacman/tests/remove-directory-replaced-with-symlink.py b/test/pacman/tests/remove-directory-replaced-with-symlink.py new file mode 100644 index 00000000..37be7579 --- /dev/null +++ b/test/pacman/tests/remove-directory-replaced-with-symlink.py @@ -0,0 +1,16 @@ +self.description = "remove a package with a directory that has been replaced with a symlink" + +self.filesystem = [ "var/", "srv -> var/" ] + +lpkg = pmpkg("pkg1") +lpkg.files = ["srv/"] +self.addpkg2db("local", lpkg) + +self.args = "-R %s" % (lpkg.name) + +self.addrule("PACMAN_RETCODE=0") +self.addrule("DIR_EXIST=var/") +self.addrule("!LINK_EXIST=srv") +self.addrule("!FILE_EXIST=srv") +self.addrule("!DIR_EXIST=srv") +self.addrule("!PACMAN_OUTPUT=cannot remove")
If the user replaces a directory with a symlink, libalpm would get confused because the trailing slash causes system calls to resolve the symlink. This leads to errors and a misleading message during upgrades. Even though libalpm does not support this, it should not be giving misleading errors. Also adds an overflow check. Fixes FS#51377 Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- See https://bbs.archlinux.org/viewtopic.php?id=224843 for another example. Subtle problems like this could be avoided by removing the trailing slashes from libalpm's internal file lists. Is there any reason not to remove them and use a field in the file_t struct to indicate type? It already has a mode field that we could use. lib/libalpm/remove.c | 13 ++++++++++++- test/pacman/tests/TESTS | 1 + .../tests/remove-directory-replaced-with-symlink.py | 16 ++++++++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 test/pacman/tests/remove-directory-replaced-with-symlink.py