[pacman-dev] [PATCH] unlink_file: strip trailing slashes

[Andrew Gregory]

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 at 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

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")
-- 
2.12.1