Hi Frank,
On Thu, 2021-09-30 at 14:36 -0400, Frank Ch. Eigler via Elfutils-devel
wrote:
> @@ -766,24 +766,14 @@ debuginfod_query_server (debuginfod_client *c,
> 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;
> @@ -797,6 +787,18 @@ debuginfod_query_server (debuginfod_client *c,
> 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;
> + }
Isn't this a time-of-check to time-of-use (TOCTOU) race now?
At least for the root user?
> long timeout = default_timeout;
> const char* timeout_envvar = getenv(DEBUGINFOD_TIMEOUT_ENV_VAR);
> diff --git a/tests/ChangeLog b/tests/ChangeLog
> index 578e8cb15502..e5aca513f1e8 100644
> --- a/tests/ChangeLog
> +++ b/tests/ChangeLog
> @@ -1,3 +1,9 @@
> +2021-09-30 Frank Ch. Eigler <f...@redhat.com>
> +
> + PR28240
> + * run-debuginfod-000-permission.sh, -writable.sh:
> + Correct negative-cache file permission checking.
> +
> 2021-09-30 Frank Ch. Eigler <f...@redhat.com>
>
> * run-debuginfod-*.sh: Use ! CMD to assert expected failure.
> diff --git a/tests/run-debuginfod-000-permission.sh b/tests/run-
> debuginfod-000-permission.sh
> index 8480ad486886..301ce7e7d9d3 100755
> --- a/tests/run-debuginfod-000-permission.sh
> +++ b/tests/run-debuginfod-000-permission.sh
> @@ -56,7 +56,7 @@ if [ ! -f $DEBUGINFOD_CACHE_PATH/01234567/debuginfo
> ]; then
> err
> fi
>
> -if [ -r $DEBUGINFOD_CACHE_PATH/01234567/debuginfo ]; then
> +if [ `stat -c "%A" $DEBUGINFOD_CACHE_PATH/01234567/debuginfo` != "
> ----------" ]; then
> echo "The cache $DEBUGINFOD_CACHE_PATH/01234567/debuginfo is
> readable"
> err
> fi
> diff --git a/tests/run-debuginfod-artifact-running.sh b/tests/run-
> debuginfod-artifact-running.sh
> index 51fa9c0a40bd..b94444267ade 100755
> --- a/tests/run-debuginfod-artifact-running.sh
> +++ b/tests/run-debuginfod-artifact-running.sh
> @@ -90,10 +90,6 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
> rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> debuginfo $BUILDID`
> cmp $filename F/prog.debug
> -if [ -w $filename ]; then
> - echo "cache file writable, boo"
> - err
> -fi
I assume you are just removing the test here, instead of replacing,
because it is already tested in the next one?
> filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> executable F/prog`
> cmp $filename F/prog
> diff --git a/tests/run-debuginfod-writable.sh b/tests/run-debuginfod-
> writable.sh
> index 69ececb39454..9cc4ea1db92c 100755
> --- a/tests/run-debuginfod-writable.sh
> +++ b/tests/run-debuginfod-writable.sh
> @@ -79,7 +79,7 @@ wait_ready $PORT1 'thread_busy{role="scan"}' 0
> rm -rf $DEBUGINFOD_CACHE_PATH # clean it from previous tests
> filename=`testrun ${abs_top_builddir}/debuginfod/debuginfod-find
> debuginfo $BUILDID`
> cmp $filename F/p+r%o\$g.debug
> -if [ -w $filename ]; then
> +if [ `stat -c "%A" $filename` != "-r--------" ]; then
> echo "cache file writable, boo"
> err
> fi
Cheers,
Mark
