From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gnu.wildebeest.org (wildebeest.demon.nl [212.238.236.112]) by sourceware.org (Postfix) with ESMTPS id 5457E3858D3C for ; Thu, 26 Aug 2021 21:59:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5457E3858D3C Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=klomp.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=klomp.org Received: from reform (deer0x0b.wildebeest.org [172.31.17.141]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by gnu.wildebeest.org (Postfix) with ESMTPSA id 1411E3000ADC; Thu, 26 Aug 2021 23:59:24 +0200 (CEST) Received: by reform (Postfix, from userid 1000) id 597B62E80D92; Thu, 26 Aug 2021 23:59:24 +0200 (CEST) Date: Thu, 26 Aug 2021 23:59:24 +0200 From: Mark Wielaard To: "Frank Ch. Eigler" Cc: elfutils-devel@sourceware.org Subject: Re: PR28240 patch: debuginfod-client negative-hit caching for root user Message-ID: References: <20210818223907.GD6064@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210818223907.GD6064@redhat.com> X-Spam-Status: No, score=-10.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP 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-List-Received-Date: Thu, 26 Aug 2021 21:59:28 -0000 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 > 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