diff mbox

[pacman-dev] Allow query of file owners to work with non-existing files

Message ID 20170802025426.18681-1-archlinux@thecybershadow.net
State Superseded, archived
Delegated to: Andrew Gregory
Headers show

Commit Message

Vladimir Panteleev Aug. 2, 2017, 2:54 a.m. UTC 
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

Comments

Allan McRae Sept. 14, 2017, 4:21 a.m. UTC | #1 
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.
Vladimir Panteleev Sept. 14, 2017, 6:16 a.m. UTC | #2 
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?
Allan McRae Sept. 14, 2017, 6:20 a.m. UTC | #3 
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...
Andrew Gregory Jan. 5, 2018, 12:38 a.m. UTC | #4 
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
diff mbox

Patch

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