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

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!