Re: [PATCH] credential/libsecret: load secrets explicitly

["Daniel Martí" <mvdan@mvdan.cc>]

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 <gitster@pobox.com> 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!