[pacman-dev] [PATCH] Allow query of file owners to work with non-existing files
Previously, attempting to query the owner of a file owned by some package but absent from the filesystem would fail. This could lead to a small annoyance - if a user or misbehaving software accidentally deleted a file owned by some package, and the user wishes to know which package it belonged to so that it can be restored, pacman would refuse to provide this information if the file no longer exists. Thus, allow file owner query to continue if the file does not exist. Print a warning in this situation and continue lookup using the exact user-provided path. Add test for this functionality accordingly. Signed-off-by: Vladimir Panteleev <archlinux@thecybershadow.net> --- src/pacman/query.c | 14 +++++++++++--- test/pacman/tests/TESTS | 1 + test/pacman/tests/query013.py | 10 ++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 test/pacman/tests/query013.py diff --git a/src/pacman/query.c b/src/pacman/query.c index 024d3e21..bc27df8e 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -180,9 +180,16 @@ static int query_fileowner(alpm_list_t *targets) goto targcleanup; } } else { - pm_printf(ALPM_LOG_ERROR, _("failed to read file '%s': %s\n"), - filename, strerror(errno)); - goto targcleanup; + if (errno == ENOENT) { + pm_printf(ALPM_LOG_WARNING, _("file '%s' does not exist, not canonicalizing\n"), + filename); + strcpy(rpath, filename); + goto noent; + } else { + pm_printf(ALPM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + goto targcleanup; + } } } @@ -192,6 +199,7 @@ static int query_fileowner(alpm_list_t *targets) goto targcleanup; } +noent: if(strncmp(rpath, root, rootlen) != 0) { /* file is outside root, we know nothing can own it */ pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename); diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index 309eb17e..344b1b49 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -109,6 +109,7 @@ TESTS += test/pacman/tests/query007.py TESTS += test/pacman/tests/query010.py TESTS += test/pacman/tests/query011.py TESTS += test/pacman/tests/query012.py +TESTS += test/pacman/tests/query013.py TESTS += test/pacman/tests/querycheck001.py TESTS += test/pacman/tests/querycheck002.py TESTS += test/pacman/tests/querycheck_fast_file_type.py diff --git a/test/pacman/tests/query013.py b/test/pacman/tests/query013.py new file mode 100644 index 00000000..b3c0a335 --- /dev/null +++ b/test/pacman/tests/query013.py @@ -0,0 +1,10 @@ +self.description = "Query ownership of non-existing file" + +sp = pmpkg("dummy", "1.0-1") +sp.files = ["etc/config"] +self.addpkg2db("local", sp) + +self.args = "-Qo %s/etc/config" % self.root + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PACMAN_OUTPUT=/etc/config is owned by dummy 1.0-1$") -- 2.13.3
On 02/08/17 12:54, Vladimir Panteleev wrote:
Previously, attempting to query the owner of a file owned by some package but absent from the filesystem would fail. This could lead to a small annoyance - if a user or misbehaving software accidentally deleted a file owned by some package, and the user wishes to know which package it belonged to so that it can be restored, pacman would refuse to provide this information if the file no longer exists.
Thus, allow file owner query to continue if the file does not exist. Print a warning in this situation and continue lookup using the exact user-provided path.
Add test for this functionality accordingly.
Signed-off-by: Vladimir Panteleev <archlinux@thecybershadow.net> --- src/pacman/query.c | 14 +++++++++++--- test/pacman/tests/TESTS | 1 + test/pacman/tests/query013.py | 10 ++++++++++ 3 files changed, 22 insertions(+), 3 deletions(-) create mode 100644 test/pacman/tests/query013.py
diff --git a/src/pacman/query.c b/src/pacman/query.c index 024d3e21..bc27df8e 100644 --- a/src/pacman/query.c +++ b/src/pacman/query.c @@ -180,9 +180,16 @@ static int query_fileowner(alpm_list_t *targets) goto targcleanup; } } else { - pm_printf(ALPM_LOG_ERROR, _("failed to read file '%s': %s\n"), - filename, strerror(errno)); - goto targcleanup; + if (errno == ENOENT) { + pm_printf(ALPM_LOG_WARNING, _("file '%s' does not exist, not canonicalizing\n"), + filename);
We should bail here if the filepath does not start in "/" since we can not canonicalize the path. Should we also look for "/../" in the string?
+ strcpy(rpath, filename); + goto noent; + } else { + pm_printf(ALPM_LOG_ERROR, _("failed to read file '%s': %s\n"), + filename, strerror(errno)); + goto targcleanup; + } } }
@@ -192,6 +199,7 @@ static int query_fileowner(alpm_list_t *targets) goto targcleanup; }
+noent: if(strncmp(rpath, root, rootlen) != 0) { /* file is outside root, we know nothing can own it */ pm_printf(ALPM_LOG_ERROR, _("No package owns %s\n"), filename); diff --git a/test/pacman/tests/TESTS b/test/pacman/tests/TESTS index 309eb17e..344b1b49 100644 --- a/test/pacman/tests/TESTS +++ b/test/pacman/tests/TESTS @@ -109,6 +109,7 @@ TESTS += test/pacman/tests/query007.py TESTS += test/pacman/tests/query010.py TESTS += test/pacman/tests/query011.py TESTS += test/pacman/tests/query012.py +TESTS += test/pacman/tests/query013.py TESTS += test/pacman/tests/querycheck001.py TESTS += test/pacman/tests/querycheck002.py TESTS += test/pacman/tests/querycheck_fast_file_type.py diff --git a/test/pacman/tests/query013.py b/test/pacman/tests/query013.py new file mode 100644 index 00000000..b3c0a335 --- /dev/null +++ b/test/pacman/tests/query013.py @@ -0,0 +1,10 @@ +self.description = "Query ownership of non-existing file" + +sp = pmpkg("dummy", "1.0-1") +sp.files = ["etc/config"] +self.addpkg2db("local", sp) + +self.args = "-Qo %s/etc/config" % self.root + +self.addrule("PACMAN_RETCODE=0") +self.addrule("PACMAN_OUTPUT=/etc/config is owned by dummy 1.0-1$")
This test passes before and after your patch... And the etc/config file is installed into the test root, so you are not testing a -Qo on a missing file.
On 2017-09-14 04:21, Allan McRae wrote:
This test passes before and after your patch... And the etc/config file is installed into the test root, so you are not testing a -Qo on a missing file.
You're right, thanks. What would be a good way to test this? I don't think there's a way to do it with the existing pmtest/pmpkg methods. I figured adding a "deleted" flag to getfileinfo would be one way, however filesystem entries are processed before package entries, and though I did get things to work with `sp.files = ["etc/foo", "etc/config", "etc/config-"]` (i.e. create the file and then delete it during the simulated package installation), that's probably too clever. Should there be a pmtest.filesystem_delete to complement pmtest.filesystem? -- Best regards, Vladimir
On 14/09/17 16:16, Vladimir Panteleev wrote:
On 2017-09-14 04:21, Allan McRae wrote:
This test passes before and after your patch... And the etc/config file is installed into the test root, so you are not testing a -Qo on a missing file.
You're right, thanks.
What would be a good way to test this? I don't think there's a way to do it with the existing pmtest/pmpkg methods. I figured adding a "deleted" flag to getfileinfo would be one way, however filesystem entries are processed before package entries, and though I did get things to work with `sp.files = ["etc/foo", "etc/config", "etc/config-"]` (i.e. create the file and then delete it during the simulated package installation), that's probably too clever.
Should there be a pmtest.filesystem_delete to complement pmtest.filesystem?
@Andrew - can you help out here? You know the internals of the testing system better than I do...
On 09/14/17 at 04:20pm, Allan McRae wrote:
On 14/09/17 16:16, Vladimir Panteleev wrote:
On 2017-09-14 04:21, Allan McRae wrote:
This test passes before and after your patch... And the etc/config file is installed into the test root, so you are not testing a -Qo on a missing file.
You're right, thanks.
What would be a good way to test this? I don't think there's a way to do it with the existing pmtest/pmpkg methods. I figured adding a "deleted" flag to getfileinfo would be one way, however filesystem entries are processed before package entries, and though I did get things to work with `sp.files = ["etc/foo", "etc/config", "etc/config-"]` (i.e. create the file and then delete it during the simulated package installation), that's probably too clever.
Should there be a pmtest.filesystem_delete to complement pmtest.filesystem?
@Andrew - can you help out here? You know the internals of the testing system better than I do...
Sorry, this slipped under my radar. Unfortuntately, I don't think there's any way to get our test suite to do this without extending it. Frankly, though, this is pretty non-essential, I would not be bothered leaving this out of our test suite. Since I missed this for so long, we now have a second patch for querying missing files: https://patchwork.archlinux.org/patch/290/ apg
participants (3)
-
Allan McRae
-
Andrew Gregory
-
Vladimir Panteleev