Re: [PATCH] PR29022: 000-permissions files cause problems for backups

[Mark Wielaard <mark@klomp.org>]

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