From: Daniel Martí Date: Mon, 21 Sep 2026 21:54:15 GMT Subject: Re: [PATCH] credential/libsecret: load secrets explicitly Message-ID: In-Reply-To: Friendly nudge :) Happy to look into any tweaks or questions. On 8/22/26 9:47 PM, Daniel Martí wrote: > Hi Junio, thanks very much for your review. I also do not program Gnome, > so this is just based on my best effort research :) > > Junio C Hamano writes: > > The above makes it sound like we can just request with > > secret_service_search_sync() exactly the same way as before (i.e., > > with LOAD_SECRETS), and then check with secret_item_get_secret() > > to see if it has secret value in it.  The problem with the current > > code is that we do not validate what that secrete value is (iow, we > > do not say "ah, NULL, we should not assume we do have secret already > > obtained here"). > > > > So does the first hunk to drop _LOAD_SECRETS really needed? > > Not strictly; your version would avoid the assertion failures too. > > I dropped the flag because it isn't the single call it appears to be: > libsecret implements SECRET_SEARCH_LOAD_SECRETS as the SearchItems > D-Bus call followed by a separate GetSecrets call covering the matched > items, with any error from that second step discarded. It also skips > locked items on the client side without even asking the daemon, so > SECRET_SEARCH_UNLOCK having silently failed is one way to end up with > a NULL secret. The patch does SearchItems plus one GetSecret for the > one item we use, so the common case is the same two round trips either > way; it is your fallback version that would add a third in the rare > case. That is what the "cost is unchanged" paragraph in the commit > message is trying to say. > > Keeping the flag with a fallback load would also mean the explicit > load only runs during the rare race, so that path would go essentially > unexercised, and load errors on the common path would still be > silently discarded. With the unconditional load there is one code > path, taken on every run, and any failure produces a real error > message. > > > I am also assuming that this is a race condition that is not very > > easy to reliably reproduce in the test, so I wouldn't expect it to > > come with a test to ensure that the fix will not regress in the > > future (in other words, lack of patch to t/ directory is fine). > > Right, it needs a concurrently-racing process (or a locked keyring) > to trigger, so I don't see a way to test it reliably. > > Thanks!