Re: PR28240 patch: debuginfod-client negative-hit caching for root user

[Mark Wielaard <mark@klomp.org>]

Hi Frank,

On Wed, Aug 18, 2021 at 06:39:07PM -0400, Frank Ch. Eigler via Elfutils-devel wrote:
> commit d2cbc610a9e6552f663e29136d19597b8fdcf832 (HEAD -> master)
> 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.

Yeah, technically access isn't really checking the file permissions.

>     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.  Fixed related testsuite inconsistencies to assert
>     subprocess errors (rc != 0), where
>        ! CMD
>     is right and
>        CMD && false || true
>     and similar were wrong.

Thanks. Even though I think people shouldn't run the build/testsuite
as root. But people will... sigh.

> diff --git a/debuginfod/debuginfod-client.c b/debuginfod/debuginfod-client.c
> index 7d4b220f30dc..be15736b3ebd 100644
> --- a/debuginfod/debuginfod-client.c
> +++ b/debuginfod/debuginfod-client.c
> @@ -726,47 +726,49 @@ debuginfod_query_server (debuginfod_client *c,
>  
>    rc = debuginfod_init_cache(cache_path, interval_path, maxage_path);
>    if (rc != 0)
>      goto out;
>    rc = debuginfod_clean_cache(c, cache_path, interval_path, maxage_path);
>    if (rc != 0)
>      goto out;
>  
> -  /* If the target is already in the cache 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;
> -    }
> -
>    struct stat st;
> -  time_t cache_miss;
> -  /* Check if the file exists and it's of permission 000*/
> -  if (errno == EACCES
> -      && stat(target_cache_path, &st) == 0
> +  /* 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)
>      {
> +      time_t cache_miss;
> +
>        rc = debuginfod_config_cache(cache_miss_path, cache_miss_default_s, &st);
>        if (rc < 0)
>          goto out;
>  
>        cache_miss = (time_t)rc;
>        if (time(NULL) - st.st_mtime <= cache_miss)
>          {
>           rc = -ENOENT;
>           goto out;
>         }
>        else
>          unlink(target_cache_path);
>      }
> +  
> +  /* If the target is already in the cache, and not a 000 file (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;
> +    }

Yes, this seems the right sequence, but don't we still have a little race
condition (if someone runs as root)?

If the timeout triggers, the file gets unlinked, then some other
process comes along, races past this one, does the server call, and
puts back a 000 file, then this process wakes up again and does the
open (as root) getting the empty file...

Cheers,

Mark