diff mbox series

PR29022: 000-permissions files cause problems for backups

Message ID 20220406003407.292374-1-amerey@redhat.com
State Committed
Headers
Series PR29022: 000-permissions files cause problems for backups |

Commit Message

Aaron Merey April 6, 2022, 12:34 a.m. UTC 
  000-permission files currently used for negative caching can cause
permission problems for some backup software and disk usage checkers.

Fix this by using empty files to for negative caching instead.

https://sourceware.org/bugzilla/show_bug.cgi?id=29022

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 debuginfod/ChangeLog                          |  5 ++
 debuginfod/debuginfod-client.c                | 90 +++++++++++--------
 tests/ChangeLog                               |  5 ++
 tests/Makefile.am                             |  4 +-
 tests/run-debuginfod-federation-link.sh       |  4 +-
 tests/run-debuginfod-federation-metrics.sh    |  4 +-
 tests/run-debuginfod-federation-sqlite.sh     |  4 +-
 ...on.sh => run-debuginfod-negative-cache.sh} |  6 +-
 tests/run-debuginfod-tmp-home.sh              |  2 +-
 9 files changed, 77 insertions(+), 47 deletions(-)
 rename tests/{run-debuginfod-000-permission.sh => run-debuginfod-negative-cache.sh} (92%)

Comments

Aaron Merey April 8, 2022, 11:37 p.m. UTC | #1 
I've revised this patch so that the negative-hit file's mtime is used
to calculate time since last download attempt instead of the cache_miss_s
file. I've also added a check for old 000-permission files so that they
are unlinked immediately if found.

Aaron

---
PR29022: 000-permissions files cause problems for backups

000-permission files currently used for negative caching can cause
permission problems for some backup software and disk usage checkers.

Fix this by using empty files to for negative caching instead.

https://sourceware.org/bugzilla/show_bug.cgi?id=29022

Signed-off-by: Aaron Merey <amerey@redhat.com>
---
 debuginfod/ChangeLog                          |  5 +
 debuginfod/debuginfod-client.c                | 94 ++++++++++++-------
 tests/ChangeLog                               |  5 +
 tests/Makefile.am                             |  4 +-
 tests/run-debuginfod-federation-link.sh       |  4 +-
 tests/run-debuginfod-federation-metrics.sh    |  4 +-
 tests/run-debuginfod-federation-sqlite.sh     |  4 +-
 ...on.sh => run-debuginfod-negative-cache.sh} |  6 +-
 tests/run-debuginfod-tmp-home.sh              |  2 +-
 9 files changed, 81 insertions(+), 47 deletions(-)
 rename tests/{run-debuginfod-000-permission.sh => run-debuginfod-negative-cache.sh} (92%)

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 578d951d..6c2edbdf 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@
+2022-04-05  Aaron Merey  <amerey@redhat.com>
+
+	* debuginfod-client.c: Represent negative caching with empty files
+	instead of 000-permission files.
+
 2022-04-03  Frank Ch. Eigler <fche@redhat.com>
 
 	* debuginfod.cxx (main): Use single dual-stack daemon setup,
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 41ba88a5..0d213bc9 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -138,7 +138,7 @@ static const char *cache_clean_interval_filename = "cache_clean_interval_s";
 static const long cache_clean_default_interval_s = 86400; /* 1 day */
 
 /* The cache_miss_default_s within the debuginfod cache specifies how
-   frequently the 000-permision file should be released.*/
+   frequently the empty file should be released.*/
 static const long cache_miss_default_s = 600; /* 10 min */
 static const char *cache_miss_filename = "cache_miss_s";
 
@@ -767,42 +767,66 @@ debuginfod_query_server (debuginfod_client *c,
   if (rc != 0)
     goto out;
 
-  struct stat st;
-  /* Check if the file exists and it's of permission 000; must check
-     explicitly rather than trying to open it first (PR28240). */
-  if (stat(target_cache_path, &st) == 0
-      && (st.st_mode & 0777) == 0)
+  /* Check if the target is already in the cache. */
+  int fd = open(target_cache_path, O_RDONLY);
+  if (fd >= 0)
     {
-      time_t cache_miss;
-
-      rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
-      if (rc < 0)
-        goto out;
+      struct stat st;
+      if (fstat(fd, &st) != 0)
+        {
+          close (fd);
+          rc = -errno;
+          goto out;
+        }
 
-      cache_miss = (time_t)rc;
-      if (time(NULL) - st.st_mtime <= cache_miss)
+      /* If the file is non-empty, then we are done. */
+      if (st.st_size > 0)
         {
-         rc = -ENOENT;
-         goto out;
-       }
+          if (path != NULL)
+            {
+              *path = strdup(target_cache_path);
+              if (*path == NULL)
+                {
+                  close (fd);
+                  rc = -errno;
+                  goto out;
+                }
+            }
+          /* Success!!!! */
+          rc = fd;
+          goto out;
+        }
       else
-        /* TOCTOU non-problem: if another task races, puts a working
-           download or a 000 file in its place, unlinking here just
-           means WE will try to download again as uncached. */
-        unlink(target_cache_path);
-    }
-  
-  /* If the target is already in the cache (known not-000 - PR28240), 
-     then we are done. */
-  int fd = open (target_cache_path, O_RDONLY);
-  if (fd >= 0)
-    {
-      /* Success!!!! */
-      if (path != NULL)
-        *path = strdup(target_cache_path);
-      rc = fd;
-      goto out;
+        {
+          /* The file is empty. Attempt to download only if enough time
+             has passed since the last attempt. */
+          time_t cache_miss;
+          time_t target_mtime = st.st_mtime;
+          rc = debuginfod_config_cache(cache_miss_path,
+                                       cache_miss_default_s, &st);
+          if (rc < 0)
+            {
+              close(fd);
+              goto out;
+            }
+
+          cache_miss = (time_t)rc;
+          if (time(NULL) - target_mtime <= cache_miss)
+            {
+              close(fd);
+              rc = -ENOENT;
+              goto out;
+            }
+          else
+            /* TOCTOU non-problem: if another task races, puts a working
+               download or an empty file in its place, unlinking here just
+               means WE will try to download again as uncached. */
+            unlink(target_cache_path);
+        }
     }
+  else if (errno == EACCES)
+    /* Ensure old 000-permission files are not lingering in the cache. */
+    unlink(target_cache_path);
 
   long timeout = default_timeout;
   const char* timeout_envvar = getenv(DEBUGINFOD_TIMEOUT_ENV_VAR);
@@ -1298,11 +1322,11 @@ debuginfod_query_server (debuginfod_client *c,
         }
     } while (num_msg > 0);
 
-  /* Create a 000-permission file named as $HOME/.cache if the query
-     fails with ENOENT.*/
+  /* Create an empty file named as $HOME/.cache if the query fails
+     with ENOENT.*/
   if (rc == -ENOENT)
     {
-      int efd = open (target_cache_path, O_CREAT|O_EXCL, 0000);
+      int efd = open (target_cache_path, O_CREAT|O_EXCL, DEFFILEMODE);
       if (efd >= 0)
         close(efd);
     }
diff --git a/tests/ChangeLog b/tests/ChangeLog
index c195f9f7..91ce1ffb 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@
+2022-04-05  Aaron Merey  <amerey@redhat.com>
+
+	* run-debuginfod-000-permissions.sh: Delete.
+	* run-debuginfod-negative-cache.sh: New test.
+
 2022-03-20  Mark Wielaard  <mark@klomp.org>
 
 	* run-large-elf-file.sh: Check elf class of addsections binary.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index b2da2c83..84c3950a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -227,7 +227,7 @@ TESTS += run-debuginfod-dlopen.sh \
 	 run-debuginfod-file.sh \
 	 run-debuginfod-sizetime.sh \
 	 run-debuginfod-malformed.sh \
-	 run-debuginfod-000-permission.sh \
+	 run-debuginfod-negative-cache.sh \
 	 run-debuginfod-tmp-home.sh \
 	 run-debuginfod-writable.sh \
 	 run-debuginfod-no-urls.sh \
@@ -529,7 +529,7 @@ EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     run-debuginfod-sizetime.sh \
 	     run-debuginfod-dlopen.sh \
 	     run-debuginfod-malformed.sh \
-	     run-debuginfod-000-permission.sh \
+	     run-debuginfod-negative-cache.sh \
 	     run-debuginfod-tmp-home.sh \
 	     run-debuginfod-writable.sh \
 	     run-debuginfod-no-urls.sh \
diff --git a/tests/run-debuginfod-federation-link.sh b/tests/run-debuginfod-federation-link.sh
index 4f043741..d7827436 100755
--- a/tests/run-debuginfod-federation-link.sh
+++ b/tests/run-debuginfod-federation-link.sh
@@ -136,7 +136,7 @@ file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -144,7 +144,7 @@ testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
diff --git a/tests/run-debuginfod-federation-metrics.sh b/tests/run-debuginfod-federation-metrics.sh
index 3da457e8..3d716246 100755
--- a/tests/run-debuginfod-federation-metrics.sh
+++ b/tests/run-debuginfod-federation-metrics.sh
@@ -130,7 +130,7 @@ file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -138,7 +138,7 @@ testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 # test parallel queries in client
diff --git a/tests/run-debuginfod-federation-sqlite.sh b/tests/run-debuginfod-federation-sqlite.sh
index 449df5db..bb3cda12 100755
--- a/tests/run-debuginfod-federation-sqlite.sh
+++ b/tests/run-debuginfod-federation-sqlite.sh
@@ -117,7 +117,7 @@ file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -125,7 +125,7 @@ testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 # test parallel queries in client
diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-debuginfod-negative-cache.sh
similarity index 92%
rename from tests/run-debuginfod-000-permission.sh
rename to tests/run-debuginfod-negative-cache.sh
index 1f46c341..f40e99c5 100755
--- a/tests/run-debuginfod-000-permission.sh
+++ b/tests/run-debuginfod-negative-cache.sh
@@ -46,15 +46,15 @@ ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
 # PR25628
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 
-# The query is designed to fail, while the 000-permission file should be created.
+# The query is designed to fail, while the empty file should be created.
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567 || true
 if [ ! -f $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
   echo "could not find cache in $DEBUGINFOD_CACHE_PATH"
   err
 fi
 
-if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "----------" ]; then
-  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable"
+if [ `stat -c "%s" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != 0 ]; then
+  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is not empty"
   err
 fi
 
diff --git a/tests/run-debuginfod-tmp-home.sh b/tests/run-debuginfod-tmp-home.sh
index 4256f6f2..5946777a 100755
--- a/tests/run-debuginfod-tmp-home.sh
+++ b/tests/run-debuginfod-tmp-home.sh
@@ -104,7 +104,7 @@ fi
 # priority over $HOME/.cache, $XDG_CACHE_HOME.
 cp -vr $DEBUGINFOD_CACHE_PATH tmphome/.debuginfod_client_cache || true
 # ||true is for tolerating errors, such a valgrind or something else
-#        leaving 000-perm files in there
+#        leaving negative-hit files in there
 
 # Add a file that doesn't exist in $HOME/.cache, $XDG_CACHE_HOME.
 mkdir tmphome/.debuginfod_client_cache/deadbeef
Milian Wolff April 9, 2022, 1:06 p.m. UTC | #2 
On Samstag, 9. April 2022 01:37:11 CEST Aaron Merey via Elfutils-devel wrote:
> I've revised this patch so that the negative-hit file's mtime is used
> to calculate time since last download attempt instead of the cache_miss_s
> file. I've also added a check for old 000-permission files so that they
> are unlinked immediately if found.

Ah, since this fixes my issue, I don't need to provide another patch for that 
purpose I guess. Mark, will you merge this then please? I tested it too, so 
feel free to include the following, if you want:

Tested-by: Milian Wolff <mail@milianw.de>

Cheers, and thanks for the fix Aaron!
Mark Wielaard April 13, 2022, 2:55 p.m. UTC | #3 
Hi Aaron,

On Fri, 2022-04-08 at 19:37 -0400, Aaron Merey via Elfutils-devel
wrote:
> I've revised this patch so that the negative-hit file's mtime is used
> to calculate time since last download attempt instead of the cache_miss_s
> file. I've also added a check for old 000-permission files so that they
> are unlinked immediately if found.
> 
> Aaron
> 
> ---
> PR29022: 000-permissions files cause problems for backups
> 
> 000-permission files currently used for negative caching can cause
> permission problems for some backup software and disk usage checkers.
> 
> Fix this by using empty files to for negative caching instead.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=29022
> 
> Signed-off-by: Aaron Merey <amerey@redhat.com>

Please also mention the fix for the mtime/stat/cache_miss_s issue in
the commit message.

> ---
>  debuginfod/ChangeLog                          |  5 +
>  debuginfod/debuginfod-client.c                | 94 ++++++++++++-----
> --
>  tests/ChangeLog                               |  5 +
>  tests/Makefile.am                             |  4 +-
>  tests/run-debuginfod-federation-link.sh       |  4 +-
>  tests/run-debuginfod-federation-metrics.sh    |  4 +-
>  tests/run-debuginfod-federation-sqlite.sh     |  4 +-
>  ...on.sh => run-debuginfod-negative-cache.sh} |  6 +-
>  tests/run-debuginfod-tmp-home.sh              |  2 +-
>  9 files changed, 81 insertions(+), 47 deletions(-)
>  rename tests/{run-debuginfod-000-permission.sh => run-debuginfod-
> negative-cache.sh} (92%)
> 
> diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
> index 578d951d..6c2edbdf 100644
> --- a/debuginfod/ChangeLog
> +++ b/debuginfod/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-04-05  Aaron Merey  <amerey@redhat.com>
> +
> +	* debuginfod-client.c: Represent negative caching with empty
> files
> +	instead of 000-permission files.

A ChangeLog entry really should mention the how things were
changed/fixed, not just restate the what, which is what the commit
message is for. I admit that I am probably the only person still seeing
value in ChangeLog entries, but if we are doing them I would suggest
something like:

	* debuginfod-client.c (debuginfod_query_server):
	Drop st_mode check. Add st_size > 0 check.
	Save target_mtime before calling
	debuginfod_config_cache. unlink target_cache_path
	on EACCESS. Create target_cache_path with DEFFILEMODE.

> @@ -767,42 +767,66 @@ debuginfod_query_server (debuginfod_client *c,
>    if (rc != 0)
>      goto out;
>  
> -  struct stat st;
> -  /* Check if the file exists and it's of permission 000; must check
> -     explicitly rather than trying to open it first (PR28240). */
> -  if (stat(target_cache_path, &st) == 0
> -      && (st.st_mode & 0777) == 0)
> +  /* Check if the target is already in the cache. */
> +  int fd = open(target_cache_path, O_RDONLY);
> +  if (fd >= 0)
>      {
> -      time_t cache_miss;
> -
> -      rc = debuginfod_config_cache(cache_miss_path,
> cache_miss_default_s, &st);
> -      if (rc < 0)
> -        goto out;
> +      struct stat st;
> +      if (fstat(fd, &st) != 0)
> +        {
> +          close (fd);
> +          rc = -errno;
> +          goto out;
> +        }

Call close after setting rc = -errno. close might also set errno,
but we want to make sure to report the original errno that fstat
returned (close probably will report the same errno, but might not,
it might also set errno to 0 which is super confusing).
 
> -      cache_miss = (time_t)rc;
> -      if (time(NULL) - st.st_mtime <= cache_miss)
> +      /* If the file is non-empty, then we are done. */
> +      if (st.st_size > 0)
>          {
> -         rc = -ENOENT;
> -         goto out;
> -       }
> +          if (path != NULL)
> +            {
> +              *path = strdup(target_cache_path);
> +              if (*path == NULL)
> +                {
> +                  close (fd);
> +                  rc = -errno;
> +                  goto out;
> +                }

Likewise.

> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index c195f9f7..91ce1ffb 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,8 @@
> +2022-04-05  Aaron Merey  <amerey@redhat.com>
> +
> +	* run-debuginfod-000-permissions.sh: Delete.
> +	* run-debuginfod-negative-cache.sh: New test.

Missing:
	* Makefile.am (TESTS): Remove run-debuginfod-000-permission.sh
	and add run-debuginfod-negative-cache.sh.
	(EXTRA_DIST): Likewise.
	* run-debuginfod-federation-link.sh: Update comments about
	negative-hit file.
	* run-debuginfod-federation-metrics.sh: Likewise.
	* run-debuginfod-federation-sqlite.sh: Likewise.
	* run-debuginfod-tmp-home.sh: Likewise.
 
> -if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "----------" ]; then
> -  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable"
> +if [ `stat -c "%s" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != 0 ]; then
> +  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is not empty"
>    err
>  fi

Changes look good, please commit with the changes suggested above and
add Tested-by: Milian Wolff <mail@milianw.de>

Thanks,

Mark
Aaron Merey April 13, 2022, 7:18 p.m. UTC | #4 
Hi Mark,

Thanks for the review. I fixed the issues you pointed out (good catch re.
setting rc before closing fd) and pushed as commit 8b568fdea8 with
Tested-by: Milian Wolff <mail@milianw.de> added. Milian, thanks again
for taking a look at this.

Aaron
diff mbox series

Patch

diff --git a/debuginfod/ChangeLog b/debuginfod/ChangeLog
index 578d951d..6c2edbdf 100644
--- a/debuginfod/ChangeLog
+++ b/debuginfod/ChangeLog
@@ -1,3 +1,8 @@ 
+2022-04-05  Aaron Merey  <amerey@redhat.com>
+
+	* debuginfod-client.c: Represent negative caching with empty files
+	instead of 000-permission files.
+
 2022-04-03  Frank Ch. Eigler <fche@redhat.com>
 
 	* debuginfod.cxx (main): Use single dual-stack daemon setup,
diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
index 41ba88a5..0fe791a0 100644
--- a/debuginfod/debuginfod-client.c
+++ b/debuginfod/debuginfod-client.c
@@ -138,7 +138,7 @@  static const char *cache_clean_interval_filename = "cache_clean_interval_s";
 static const long cache_clean_default_interval_s = 86400; /* 1 day */
 
 /* The cache_miss_default_s within the debuginfod cache specifies how
-   frequently the 000-permision file should be released.*/
+   frequently the empty file should be released.*/
 static const long cache_miss_default_s = 600; /* 10 min */
 static const char *cache_miss_filename = "cache_miss_s";
 
@@ -767,41 +767,61 @@  debuginfod_query_server (debuginfod_client *c,
   if (rc != 0)
     goto out;
 
-  struct stat st;
-  /* Check if the file exists and it's of permission 000; must check
-     explicitly rather than trying to open it first (PR28240). */
-  if (stat(target_cache_path, &st) == 0
-      && (st.st_mode & 0777) == 0)
+  /* Check if the target is already in the cache. */
+  int fd = open(target_cache_path, O_RDONLY);
+  if (fd >= 0)
     {
-      time_t cache_miss;
-
-      rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
-      if (rc < 0)
-        goto out;
+      struct stat st;
+      if (fstat(fd, &st) != 0)
+        {
+          close (fd);
+          rc = -errno;
+          goto out;
+        }
 
-      cache_miss = (time_t)rc;
-      if (time(NULL) - st.st_mtime <= cache_miss)
+      /* If the file is non-empty, then we are done. */
+      if (st.st_size > 0)
         {
-         rc = -ENOENT;
-         goto out;
-       }
+          if (path != NULL)
+            {
+              *path = strdup(target_cache_path);
+              if (*path == NULL)
+                {
+                  close (fd);
+                  rc = -errno;
+                  goto out;
+                }
+            }
+          /* Success!!!! */
+          rc = fd;
+          goto out;
+        }
       else
-        /* TOCTOU non-problem: if another task races, puts a working
-           download or a 000 file in its place, unlinking here just
-           means WE will try to download again as uncached. */
-        unlink(target_cache_path);
-    }
-  
-  /* If the target is already in the cache (known not-000 - PR28240), 
-     then we are done. */
-  int fd = open (target_cache_path, O_RDONLY);
-  if (fd >= 0)
-    {
-      /* Success!!!! */
-      if (path != NULL)
-        *path = strdup(target_cache_path);
-      rc = fd;
-      goto out;
+        {
+          /* The file is empty. Attempt to download only if enough time
+             has passed since the last attempt. */
+          time_t cache_miss;
+          rc = debuginfod_config_cache(cache_miss_path,
+                                       cache_miss_default_s, &st);
+          if (rc < 0)
+            {
+              close(fd);
+              goto out;
+            }
+
+          cache_miss = (time_t)rc;
+          if (time(NULL) - st.st_mtime <= cache_miss)
+            {
+              close(fd);
+              rc = -ENOENT;
+              goto out;
+            }
+          else
+            /* TOCTOU non-problem: if another task races, puts a working
+               download or an empty file in its place, unlinking here just
+               means WE will try to download again as uncached. */
+            unlink(target_cache_path);
+        }
     }
 
   long timeout = default_timeout;
@@ -1298,11 +1318,11 @@  debuginfod_query_server (debuginfod_client *c,
         }
     } while (num_msg > 0);
 
-  /* Create a 000-permission file named as $HOME/.cache if the query
-     fails with ENOENT.*/
+  /* Create an empty file named as $HOME/.cache if the query fails
+     with ENOENT.*/
   if (rc == -ENOENT)
     {
-      int efd = open (target_cache_path, O_CREAT|O_EXCL, 0000);
+      int efd = open (target_cache_path, O_CREAT|O_EXCL, DEFFILEMODE);
       if (efd >= 0)
         close(efd);
     }
diff --git a/tests/ChangeLog b/tests/ChangeLog
index c195f9f7..91ce1ffb 100644
--- a/tests/ChangeLog
+++ b/tests/ChangeLog
@@ -1,3 +1,8 @@ 
+2022-04-05  Aaron Merey  <amerey@redhat.com>
+
+	* run-debuginfod-000-permissions.sh: Delete.
+	* run-debuginfod-negative-cache.sh: New test.
+
 2022-03-20  Mark Wielaard  <mark@klomp.org>
 
 	* run-large-elf-file.sh: Check elf class of addsections binary.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index b2da2c83..84c3950a 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -227,7 +227,7 @@  TESTS += run-debuginfod-dlopen.sh \
 	 run-debuginfod-file.sh \
 	 run-debuginfod-sizetime.sh \
 	 run-debuginfod-malformed.sh \
-	 run-debuginfod-000-permission.sh \
+	 run-debuginfod-negative-cache.sh \
 	 run-debuginfod-tmp-home.sh \
 	 run-debuginfod-writable.sh \
 	 run-debuginfod-no-urls.sh \
@@ -529,7 +529,7 @@  EXTRA_DIST = run-arextract.sh run-arsymtest.sh run-ar.sh \
 	     run-debuginfod-sizetime.sh \
 	     run-debuginfod-dlopen.sh \
 	     run-debuginfod-malformed.sh \
-	     run-debuginfod-000-permission.sh \
+	     run-debuginfod-negative-cache.sh \
 	     run-debuginfod-tmp-home.sh \
 	     run-debuginfod-writable.sh \
 	     run-debuginfod-no-urls.sh \
diff --git a/tests/run-debuginfod-federation-link.sh b/tests/run-debuginfod-federation-link.sh
index 4f043741..d7827436 100755
--- a/tests/run-debuginfod-federation-link.sh
+++ b/tests/run-debuginfod-federation-link.sh
@@ -136,7 +136,7 @@  file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -144,7 +144,7 @@  testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
diff --git a/tests/run-debuginfod-federation-metrics.sh b/tests/run-debuginfod-federation-metrics.sh
index 3da457e8..3d716246 100755
--- a/tests/run-debuginfod-federation-metrics.sh
+++ b/tests/run-debuginfod-federation-metrics.sh
@@ -130,7 +130,7 @@  file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -138,7 +138,7 @@  testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 # test parallel queries in client
diff --git a/tests/run-debuginfod-federation-sqlite.sh b/tests/run-debuginfod-federation-sqlite.sh
index 449df5db..bb3cda12 100755
--- a/tests/run-debuginfod-federation-sqlite.sh
+++ b/tests/run-debuginfod-federation-sqlite.sh
@@ -117,7 +117,7 @@  file -L L/foo
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=http://127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 
@@ -125,7 +125,7 @@  testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 export DEBUGINFOD_URLS=127.0.0.1:$PORT1
 rm -rf $DEBUGINFOD_CACHE_PATH
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID && false || true
-rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop 000-perm negative-hit file
+rm -f $DEBUGINFOD_CACHE_PATH/$BUILDID/debuginfo # drop negative-hit file
 export DEBUGINFOD_URLS=127.0.0.1:$PORT2
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo $BUILDID
 # test parallel queries in client
diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-debuginfod-negative-cache.sh
similarity index 92%
rename from tests/run-debuginfod-000-permission.sh
rename to tests/run-debuginfod-negative-cache.sh
index 1f46c341..f40e99c5 100755
--- a/tests/run-debuginfod-000-permission.sh
+++ b/tests/run-debuginfod-negative-cache.sh
@@ -46,15 +46,15 @@  ps -q $PID1 -e -L -o '%p %c %a' | grep traverse
 # PR25628
 rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
 
-# The query is designed to fail, while the 000-permission file should be created.
+# The query is designed to fail, while the empty file should be created.
 testrun ${abs_top_builddir}/debuginfod/debuginfod-find debuginfo 01234567 || true
 if [ ! -f $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
   echo "could not find cache in $DEBUGINFOD_CACHE_PATH"
   err
 fi
 
-if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "----------" ]; then
-  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is readable"
+if [ `stat -c "%s" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != 0 ]; then
+  echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is not empty"
   err
 fi
 
diff --git a/tests/run-debuginfod-tmp-home.sh b/tests/run-debuginfod-tmp-home.sh
index 4256f6f2..5946777a 100755
--- a/tests/run-debuginfod-tmp-home.sh
+++ b/tests/run-debuginfod-tmp-home.sh
@@ -104,7 +104,7 @@  fi
 # priority over $HOME/.cache, $XDG_CACHE_HOME.
 cp -vr $DEBUGINFOD_CACHE_PATH tmphome/.debuginfod_client_cache || true
 # ||true is for tolerating errors, such a valgrind or something else
-#        leaving 000-perm files in there
+#        leaving negative-hit files in there
 
 # Add a file that doesn't exist in $HOME/.cache, $XDG_CACHE_HOME.
 mkdir tmphome/.debuginfod_client_cache/deadbeef