[Bug debuginfod/28240] New: debuginfod client cache falsely sticky for root user

[Bug debuginfod/28240] New: debuginfod client cache falsely sticky for root user

[fche at redhat dot com]

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

            Bug ID: 28240
           Summary: debuginfod client cache falsely sticky for root user
           Product: elfutils
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: debuginfod
          Assignee: unassigned at sourceware dot org
          Reporter: fche at redhat dot com
                CC: elfutils-devel at sourceware dot org
  Target Milestone: ---

bug #25628 introduced a negative-hit caching facility in the debuginfod client,
which represents upstream misses with short-lived permission-000 files in the
cache.  These files are used to shortcut repeated queries that are expected to
fail, for a limited time.

The logic works for normal users, but breaks for root users.  The problem is
that the way the client recognizes a 000 negative-hit file in the cache, vs. a
good file, is by looking for -EACCES upon opening the file.  Unfortunately,
root users never get -EACCES, even for perm-000 files.  So a 000 file for root
is treated as though it was a successful fetch of a 0-length file, and poisons
the cache indefinitely.

    745   struct stat st;
    746   time_t cache_miss;
    747   /* Check if the file exists and it's of permission 000*/
    748   if (errno == EACCES
    749       && stat(target_cache_path, &st) == 0
    750       && (st.st_mode & 0777) == 0)

Probably the simplest fix is to ditch the "errno == EACCESS" part of the test,
and perform the stat every time.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug debuginfod/28240] debuginfod client cache falsely sticky for root user

[fche at redhat dot com]

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

Frank Ch. Eigler <fche at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #1 from Frank Ch. Eigler <fche at redhat dot com> ---
commit 7d64173fb11c66284a408e52d41d15b7755d65d2
Author: Frank Ch. Eigler <fche@redhat.com>
Date:   Wed Aug 18 18:29:34 2021 -0400

    PR28240: debuginfod client root-safe negative caching

    Negative cache (000-permission) files were incorrectly treated as
    valid cached files for the root user, because root can open even
    000-perm files without -EACCES.  Corrected this checking sequence.

    Fixed the debuginfod testsuite to run to completion as root or
    as an ordinary user, correcting corresponding permission checks:
        stat -c %A $FILE
    is right and
        [ -w $FILE]  [ -r $FILE ]
    were wrong.

    Signed-off-by: Frank Ch. Eigler <fche@redhat.com>

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug debuginfod/28240] debuginfod client cache falsely sticky for root user

[mark at klomp dot org]

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

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mark at klomp dot org
         Resolution|FIXED                       |---
             Status|RESOLVED                    |REOPENED

--- Comment #2 from Mark Wielaard <mark at klomp dot org> ---
So this makes most uses for the user "root" correct, but still contains a race
condition:

+        /* 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 problem isn't when WE try to download and/or (re)set the 000 file. The
problem is if someone other client races past us after the unlink and puts a
000 file back (because the server still doesn't have it). Then the open will
again succeed for us, but the target is 000.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug debuginfod/28240] debuginfod client cache falsely sticky for root user

[mark at klomp dot org]

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

--- Comment #3 from Mark Wielaard <mark at klomp dot org> ---
We got rid of the zero-permission 000 files with:

commit 8b568fdea8e1baea3d68cc38dec587e4c9219276
Author: Aaron Merey <amerey@redhat.com>
Date:   Fri Apr 8 19:37:11 2022 -0400

    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 for negative caching instead.

    Also use each empty file's mtime to determine the time since
    last download attempt instead of the cache_miss_s file's mtime.

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

    Tested-by: Milian Wolff <mail@milianw.de>
    Signed-off-by: Aaron Merey <amerey@redhat.com>

And I think that also got rid of this race issue.

Any other client that races past us now will either create a new empty file
with   
open (target_cache_path, O_CREAT|O_EXCL, DEFFILEMODE) or puts in a new
non-empty file using rename (target_cache_tmppath, target_cache_path). Both of
which are atomic.

So I think this is resolved now. But would like someone else to double check.
These races are tricky.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug debuginfod/28240] debuginfod client cache falsely sticky for root user

[mark at klomp dot org]

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

--- Comment #4 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Mark Wielaard from comment #3)
> So I think this is resolved now. But would like someone else to double
> check. These races are tricky.

That was 3 months ago. Can we assume this is fixed?

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug debuginfod/28240] debuginfod client cache falsely sticky for root user

[mark at klomp dot org]

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

Mark Wielaard <mark at klomp dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #5 from Mark Wielaard <mark at klomp dot org> ---
(In reply to Mark Wielaard from comment #4)
> (In reply to Mark Wielaard from comment #3)
> > So I think this is resolved now. But would like someone else to double
> > check. These races are tricky.
> 
> That was 3 months ago. Can we assume this is fixed?

Lets assume so.

-- 
You are receiving this mail because:
You are on the CC list for the bug.