[pacman-dev] [PATCH 1/5] add test for file type check with -Qk
If a directory has been replaced by a symlink, -Qk currently stats the symlink target rather than the symlink itself and doesn't check that the actual file type matches the package file list. This will make it difficult to discover errors once 4.2 is released and replacing directories with symlinks is no longer supported. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- test/pacman/tests/TESTS | 1 + test/pacman/tests/querycheck_fast_file_type.py | 14 ++++++++++++++ 2 files changed, 15 insertions(+) create mode 100644 test/pacman/tests/querycheck_fast_file_type.py diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index dc47294..1b5a81f 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -90,6 +90,7 @@ TESTS += test/pacman/tests/query011.py TESTS += test/pacman/tests/query012.py TESTS += test/pacman/tests/querycheck001.py 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/remove001.py TESTS += test/pacman/tests/remove002.py diff --git a/test/pacman/tests/querycheck_fast_file_type.py b/test/pacman/tests/querycheck_fast_file_type.py new file mode 100644 index 0000000..a19fcee --- /dev/null +++ b/test/pacman/tests/querycheck_fast_file_type.py @@ -0,0 +1,14 @@ +self.description = "check file type without mtree" + +self.filesystem = [ "bar/", "foo -> bar/" ] + +pkg = pmpkg("dummy") +pkg.files = [ "foo/" ] +self.addpkg2db("local",pkg) + +self.args = "-Qk" + +self.addrule("PACMAN_RETCODE=1") +self.addrule("PACMAN_OUTPUT=warning.*(File type mismatch)") + +self.expectfailure = True -- 2.0.0
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- lib/libalpm/add.c | 4 ++-- lib/libalpm/conflict.c | 2 +- lib/libalpm/diskspace.c | 2 +- lib/libalpm/remove.c | 2 +- lib/libalpm/util.c | 25 ------------------------- lib/libalpm/util.h | 2 -- src/common/util-common.c | 25 +++++++++++++++++++++++++ src/common/util-common.h | 4 ++++ 8 files changed, 34 insertions(+), 32 deletions(-) diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c index 4f557a4..bbf2a51 100644 --- a/lib/libalpm/add.c +++ b/lib/libalpm/add.c @@ -209,7 +209,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, * F/N | 3 | 4 * D | 5 | 6 * - * 1,2- extract, no magic necessary. lstat (_alpm_lstat) will fail here. + * 1,2- extract, no magic necessary. lstat (llstat) will fail here. * 3,4- conflict checks should have caught this. either overwrite * or backup the file. * 5- file replacing directory- don't allow it. @@ -217,7 +217,7 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive, */ struct stat lsbuf; - if(_alpm_lstat(filename, &lsbuf) != 0) { + if(llstat(filename, &lsbuf) != 0) { /* cases 1,2: file doesn't exist, skip all backup checks */ } else { if(S_ISDIR(lsbuf.st_mode)) { diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index 2611aef..961e3a7 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -495,7 +495,7 @@ alpm_list_t *_alpm_db_find_fileconflicts(alpm_handle_t *handle, relative_path = path + rootlen; /* stat the file - if it exists, do some checks */ - if(_alpm_lstat(path, &lsbuf) != 0) { + if(llstat(path, &lsbuf) != 0) { continue; } diff --git a/lib/libalpm/diskspace.c b/lib/libalpm/diskspace.c index d07b188..606f2dc 100644 --- a/lib/libalpm/diskspace.c +++ b/lib/libalpm/diskspace.c @@ -235,7 +235,7 @@ static int calculate_removed_size(alpm_handle_t *handle, const char *filename = file->name; snprintf(path, PATH_MAX, "%s%s", handle->root, filename); - _alpm_lstat(path, &st); + llstat(path, &st); /* skip directories and symlinks to be consistent with libarchive that * reports them to be zero size */ diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c index 5cbeeb9..dd061e5 100644 --- a/lib/libalpm/remove.c +++ b/lib/libalpm/remove.c @@ -457,7 +457,7 @@ static int unlink_file(alpm_handle_t *handle, alpm_pkg_t *oldpkg, return 1; } - if(_alpm_lstat(file, &buf)) { + if(llstat(file, &buf)) { _alpm_log(handle, ALPM_LOG_DEBUG, "file %s does not exist\n", file); return 1; } diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c index d9f741b..6dab0de 100644 --- a/lib/libalpm/util.c +++ b/lib/libalpm/util.c @@ -742,31 +742,6 @@ const char *_alpm_filecache_setup(alpm_handle_t *handle) return cachedir; } -/** lstat wrapper that treats /path/dirsymlink/ the same as /path/dirsymlink. - * Linux lstat follows POSIX semantics and still performs a dereference on - * the first, and for uses of lstat in libalpm this is not what we want. - * @param path path to file to lstat - * @param buf structure to fill with stat information - * @return the return code from lstat - */ -int _alpm_lstat(const char *path, struct stat *buf) -{ - int ret; - size_t len = strlen(path); - - /* strip the trailing slash if one exists */ - if(len != 0 && path[len - 1] == '/') { - char *newpath = strdup(path); - newpath[len - 1] = '\0'; - ret = lstat(newpath, buf); - free(newpath); - } else { - ret = lstat(path, buf); - } - - return ret; -} - #ifdef HAVE_LIBSSL /** Compute the MD5 message digest of a file. * @param path file path of file to compute MD5 digest of diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h index 20f63f6..6f47073 100644 --- a/lib/libalpm/util.h +++ b/lib/libalpm/util.h @@ -35,7 +35,6 @@ #include <stdarg.h> #include <stddef.h> /* size_t */ #include <sys/types.h> -#include <sys/stat.h> /* struct stat */ #include <math.h> /* fabs */ #include <float.h> /* DBL_EPSILON */ #include <fcntl.h> /* open, close */ @@ -128,7 +127,6 @@ int _alpm_ldconfig(alpm_handle_t *handle); int _alpm_str_cmp(const void *s1, const void *s2); char *_alpm_filecache_find(alpm_handle_t *handle, const char *filename); const char *_alpm_filecache_setup(alpm_handle_t *handle); -int _alpm_lstat(const char *path, struct stat *buf); int _alpm_test_checksum(const char *filepath, const char *expected, alpm_pkgvalidation_t type); int _alpm_archive_fgets(struct archive *a, struct archive_read_buffer *b); int _alpm_splitname(const char *target, char **name, char **version, diff --git a/src/common/util-common.c b/src/common/util-common.c index c5097dd..e6f9519 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -73,6 +73,31 @@ char *mdirname(const char *path) return strdup("."); } +/** lstat wrapper that treats /path/dirsymlink/ the same as /path/dirsymlink. + * Linux lstat follows POSIX semantics and still performs a dereference on + * the first, and for uses of lstat in libalpm this is not what we want. + * @param path path to file to lstat + * @param buf structure to fill with stat information + * @return the return code from lstat + */ +int llstat(const char *path, struct stat *buf) +{ + int ret; + size_t len = strlen(path); + + /* strip the trailing slash if one exists */ + if(len != 0 && path[len - 1] == '/') { + char *newpath = strdup(path); + newpath[len - 1] = '\0'; + ret = lstat(newpath, buf); + free(newpath); + } else { + ret = lstat(path, buf); + } + + return ret; +} + #ifndef HAVE_STRNDUP /* A quick and dirty implementation derived from glibc */ /** Determines the length of a fixed-size string. diff --git a/src/common/util-common.h b/src/common/util-common.h index e28c60d..5f04b00 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -20,9 +20,13 @@ #ifndef _PM_UTIL_COMMON_H #define _PM_UTIL_COMMON_H +#include <sys/stat.h> /* struct stat */ + const char *mbasename(const char *path); char *mdirname(const char *path); +int llstat(const char *path, struct stat *buf); + #ifndef HAVE_STRNDUP char *strndup(const char *s, size_t n); #endif -- 2.0.0
This makes llstat's signature differ from lstat's, but we never actually use it on a const string and this saves a large number of strdup's. This also allows stripping multiple trailing slashes and corrects a bug where calling llstat on "/" would result in calling lstat on an empty string. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/common/util-common.c | 18 +++++++++++------- src/common/util-common.h | 2 +- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/common/util-common.c b/src/common/util-common.c index e6f9519..3316eae 100644 --- a/src/common/util-common.c +++ b/src/common/util-common.c @@ -80,17 +80,21 @@ char *mdirname(const char *path) * @param buf structure to fill with stat information * @return the return code from lstat */ -int llstat(const char *path, struct stat *buf) +int llstat(char *path, struct stat *buf) { int ret; + char *c = NULL; size_t len = strlen(path); - /* strip the trailing slash if one exists */ - if(len != 0 && path[len - 1] == '/') { - char *newpath = strdup(path); - newpath[len - 1] = '\0'; - ret = lstat(newpath, buf); - free(newpath); + while(len > 1 && path[len - 1] == '/') { + --len; + c = path + len; + } + + if(c) { + *c = '\0'; + ret = lstat(path, buf); + *c = '/'; } else { ret = lstat(path, buf); } diff --git a/src/common/util-common.h b/src/common/util-common.h index 5f04b00..576702f 100644 --- a/src/common/util-common.h +++ b/src/common/util-common.h @@ -25,7 +25,7 @@ const char *mbasename(const char *path); char *mdirname(const char *path); -int llstat(const char *path, struct stat *buf); +int llstat(char *path, struct stat *buf); #ifndef HAVE_STRNDUP char *strndup(const char *s, size_t n); -- 2.0.0
Paths are constructed directly from package file lists and may contain trailing slashes, causing lstat to dereference symlinks. Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/check.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/pacman/check.c b/src/pacman/check.c index a7c66ba..60a038a 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -26,11 +26,11 @@ #include "conf.h" #include "util.h" -static int check_file_exists(const char *pkgname, const char * filepath, - struct stat * st) +static int check_file_exists(const char *pkgname, char *filepath, + struct stat *st) { /* use lstat to prevent errors from symlinks */ - if(lstat(filepath, st) != 0) { + if(llstat(filepath, st) != 0) { if(config->quiet) { printf("%s %s\n", pkgname, filepath); } else { -- 2.0.0
Signed-off-by: Andrew Gregory <andrew.gregory.8@gmail.com> --- src/pacman/check.c | 15 +++++++++++++-- test/pacman/tests/querycheck_fast_file_type.py | 2 -- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/pacman/check.c b/src/pacman/check.c index 60a038a..7109c7a 100644 --- a/src/pacman/check.c +++ b/src/pacman/check.c @@ -210,14 +210,25 @@ int check_pkg_fast(alpm_pkg_t *pkg) const alpm_file_t *file = filelist->files + i; struct stat st; const char *path = file->name; + size_t plen = strlen(path); - if(rootlen + 1 + strlen(path) > PATH_MAX) { + if(rootlen + 1 + plen > PATH_MAX) { pm_printf(ALPM_LOG_WARNING, _("path too long: %s%s\n"), root, path); continue; } strcpy(filepath + rootlen, path); - errors += check_file_exists(pkgname, filepath, &st); + if(check_file_exists(pkgname, filepath, &st) == 0) { + int expect_dir = path[plen - 1] == '/' ? 1 : 0; + int is_dir = S_ISDIR(st.st_mode) ? 1 : 0; + if(expect_dir != is_dir) { + pm_printf(ALPM_LOG_WARNING, _("%s: %s (File type mismatch)\n"), + pkgname, filepath); + ++errors; + } + } else { + ++errors; + } } if(!config->quiet) { diff --git a/test/pacman/tests/querycheck_fast_file_type.py b/test/pacman/tests/querycheck_fast_file_type.py index a19fcee..f53bada 100644 --- a/test/pacman/tests/querycheck_fast_file_type.py +++ b/test/pacman/tests/querycheck_fast_file_type.py @@ -10,5 +10,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("PACMAN_OUTPUT=warning.*(File type mismatch)") - -self.expectfailure = True -- 2.0.0
On 27/06/14 02:52, Andrew Gregory wrote:
If a directory has been replaced by a symlink, -Qk currently stats the symlink target rather than the symlink itself and doesn't check that the actual file type matches the package file list. This will make it difficult to discover errors once 4.2 is released and replacing directories with symlinks is no longer supported.
I like the bug fixing part (lstat directory replaced by symlink), but I am not convinced about the file type checking for directories only being added to -Qk. We should just handle this with -Qkk. So I am "happy" with patches 2-4 in this series (without a detailed review), but think we should skip 1 and 5. I realise that not all Arch packages have MTREE files. I am going to start a rebuild to get this done. Would you be fine keeping -Qk to just file existence checking if that was done? A
On 06/29/14 at 11:39am, Allan McRae wrote:
On 27/06/14 02:52, Andrew Gregory wrote:
If a directory has been replaced by a symlink, -Qk currently stats the symlink target rather than the symlink itself and doesn't check that the actual file type matches the package file list. This will make it difficult to discover errors once 4.2 is released and replacing directories with symlinks is no longer supported.
I like the bug fixing part (lstat directory replaced by symlink), but I am not convinced about the file type checking for directories only being added to -Qk. We should just handle this with -Qkk.
So I am "happy" with patches 2-4 in this series (without a detailed review), but think we should skip 1 and 5.
I realise that not all Arch packages have MTREE files. I am going to start a rebuild to get this done. Would you be fine keeping -Qk to just file existence checking if that was done?
A
The only problem I have with relying on -Qkk is that most of the things it checks are irrelevant to pacman and therefore noise when one just wants to verify that the filesystem matches the database at the level at which pacman operates. apg
On 29/06/14 12:25, Andrew Gregory wrote:
On 06/29/14 at 11:39am, Allan McRae wrote:
On 27/06/14 02:52, Andrew Gregory wrote:
If a directory has been replaced by a symlink, -Qk currently stats the symlink target rather than the symlink itself and doesn't check that the actual file type matches the package file list. This will make it difficult to discover errors once 4.2 is released and replacing directories with symlinks is no longer supported.
I like the bug fixing part (lstat directory replaced by symlink), but I am not convinced about the file type checking for directories only being added to -Qk. We should just handle this with -Qkk.
So I am "happy" with patches 2-4 in this series (without a detailed review), but think we should skip 1 and 5.
I realise that not all Arch packages have MTREE files. I am going to start a rebuild to get this done. Would you be fine keeping -Qk to just file existence checking if that was done?
A
The only problem I have with relying on -Qkk is that most of the things it checks are irrelevant to pacman and therefore noise when one just wants to verify that the filesystem matches the database at the level at which pacman operates.
OK - patches accepted with the minor modification needed to the man page. Allan
participants (2)
-
Allan McRae
-
Andrew Gregory