From patchwork Wed Apr 6 00:34:07 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aaron Merey X-Patchwork-Id: 52656 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9717E3858025 for ; Wed, 6 Apr 2022 00:34:31 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 9717E3858025 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1649205271; bh=XmURwx2e0E3R60fHUN8qIKjIF+G+CdsmuF6RIgFbi0o=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:From; b=hO7SphOsGg/f0hfrmh7+zUkifDzwUPwsr2bUel4HwBQ8eWWn3weFodsMr0rC4kVcJ OpoWKm3vWZNMoWe0SCsUPfXvgXqMoMjBvSqVzEYfliRYM6KktccCGiCQaDpW6DPLbJ Uyci42vrUl2aY2cOFUuDtREwh5bJIAj1RNB74M9I= X-Original-To: elfutils-devel@sourceware.org Delivered-To: elfutils-devel@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 05DC23858D37 for ; Wed, 6 Apr 2022 00:34:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 05DC23858D37 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-121-z97xmes5PaiovvaUBAoUeQ-1; Tue, 05 Apr 2022 20:34:17 -0400 X-MC-Unique: z97xmes5PaiovvaUBAoUeQ-1 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CDA5C80005D for ; Wed, 6 Apr 2022 00:34:16 +0000 (UTC) Received: from fedora.redhat.com (unknown [10.22.34.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8ADC02026D64; Wed, 6 Apr 2022 00:34:15 +0000 (UTC) To: elfutils-devel@sourceware.org Subject: [PATCH] PR29022: 000-permissions files cause problems for backups Date: Tue, 5 Apr 2022 20:34:07 -0400 Message-Id: <20220406003407.292374-1-amerey@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-10.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_BADIPHTTP, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: elfutils-devel@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Elfutils-devel mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-Patchwork-Original-From: Aaron Merey via Elfutils-devel From: Aaron Merey Reply-To: Aaron Merey Errors-To: elfutils-devel-bounces+patchwork=sourceware.org@sourceware.org Sender: "Elfutils-devel" 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 Signed-off-by: Aaron Merey Tested-by: Milian Wolff Tested-by: Milian Wolff added. Milian, thanks again --- 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%) 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 + + * debuginfod-client.c: Represent negative caching with empty files + instead of 000-permission files. + 2022-04-03 Frank Ch. Eigler * 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 + + * run-debuginfod-000-permissions.sh: Delete. + * run-debuginfod-negative-cache.sh: New test. + 2022-03-20 Mark Wielaard * 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