Gentle nudge on this, anything I can do to assist in getting it reviewed?

I still run into the error a few times per week :)

On 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:
> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
>
> secret_service_search_sync() can return an item whose secret is not
> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search
> silently discards secret-loading failures, and the GNOME keyring
> daemon silently omits from its GetSecrets reply any item that is
> locked or that was deleted after the search matched it, e.g. by a
> concurrent "credential erase" from another git process.
>
> secret_item_get_secret() then returns NULL, which we pass unchecked
> to secret_value_get_text() and secret_value_unref(), producing
>
>      secret_value_get_text: assertion 'value' failed
>      secret_value_unref: assertion 'value != NULL' failed
>
> and losing the password even when the secret is still retrievable.
>
> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the
> one item we use with secret_item_load_secret_sync(), which does
> report errors. A secret the search would have silently dropped is
> now retrieved normally, and a genuinely inaccessible item produces
> a useful message instead of assertion spew, with git falling back
> to prompting either way. Merely guarding against NULL would avoid
> the assertions, but would forfeit a secret that is still available.
> The cost is unchanged: the search no longer batch-fetches the
> secrets of all matching items, and the explicit load fetches the
> one we use.
>
> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
> ---
>      credential/libsecret: load secrets explicitly
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1
> Pull-Request: https://github.com/git/git/pull/2372
>
>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
> index 941b2afd5e..6bbdf2bd45 100644
> --- a/contrib/credential/libsecret/git-credential-libsecret.c
> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)
>   	items = secret_service_search_sync(service,
>   					   &schema,
>   					   attributes,
> -					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> +					   SECRET_SEARCH_UNLOCK,
>   					   NULL,
>   					   &error);
>   	g_hash_table_unref(attributes);
> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)
>   		gchar **parts;
>   >   		item = items->data;
> +
> +		/*
> +		 * Load the secret explicitly rather than via
> +		 * SECRET_SEARCH_LOAD_SECRETS, which silently discards load
> +		 * failures and returns items whose secret is NULL.
> +		 */
> +		if (!secret_item_load_secret_sync(item, NULL, &error)) {
> +			g_critical("could not load secret: %s", error->message);
> +			g_error_free(error);
> +			g_list_free_full(items, g_object_unref);
> +			return EXIT_FAILURE;
> +		}
>   		secret = secret_item_get_secret(item);
>   		attributes = secret_item_get_attributes(item);
>   >
> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a

Daniel Martí <mvdan@mvdan.cc> writes:

> Gentle nudge on this, anything I can do to assist in getting it reviewed?
>
> I still run into the error a few times per week :)
>
> On 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:
>> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
>>
>> secret_service_search_sync() can return an item whose secret is not
>> loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search
>> silently discards secret-loading failures, and the GNOME keyring
>> daemon silently omits from its GetSecrets reply any item that is
>> locked or that was deleted after the search matched it, e.g. by a
>> concurrent "credential erase" from another git process.
>>
>> secret_item_get_secret() then returns NULL, which we pass unchecked
>> to secret_value_get_text() and secret_value_unref(), producing

I do not program Gnome so I am not exactly qualified to review this,
but anyway.

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?  Rather,
would it be more straight-forward to do

	item = items->data;

	secret = secret_item_get_secret(item);
	if (!secret &&
            !secret_item_load_secret_sync(item, NULL, &error)) {
		... your error handling here ...
		return EXIT_FAILURE;
	} 
        if (!secret)
		secret = secret_item_get_secret(item);

instead?  I am assuming that it is rare (like, only a few times a
week) to race with other activities that unloads the secret and most
of the time the first secret_service_search_sync() gets everything
needed in a single call.

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).

Thanks.


>>
>>      secret_value_get_text: assertion 'value' failed
>>      secret_value_unref: assertion 'value != NULL' failed
>>
>> and losing the password even when the secret is still retrievable.
>>
>> Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the
>> one item we use with secret_item_load_secret_sync(), which does
>> report errors. A secret the search would have silently dropped is
>> now retrieved normally, and a genuinely inaccessible item produces
>> a useful message instead of assertion spew, with git falling back
>> to prompting either way. Merely guarding against NULL would avoid
>> the assertions, but would forfeit a secret that is still available.
>> The cost is unchanged: the search no longer batch-fetches the
>> secrets of all matching items, and the explicit load fetches the
>> one we use.
>>
>> Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
>> ---
>>      credential/libsecret: load secrets explicitly
>>
>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1
>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1
>> Pull-Request: https://github.com/git/git/pull/2372
>>
>>   .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
>> index 941b2afd5e..6bbdf2bd45 100644
>> --- a/contrib/credential/libsecret/git-credential-libsecret.c
>> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
>> @@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)
>>   	items = secret_service_search_sync(service,
>>   					   &schema,
>>   					   attributes,
>> -					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
>> +					   SECRET_SEARCH_UNLOCK,
>>   					   NULL,
>>   					   &error);
>>   	g_hash_table_unref(attributes);
>> @@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)
>>   		gchar **parts;
>>   
>>   		item = items->data;
>> +
>> +		/*
>> +		 * Load the secret explicitly rather than via
>> +		 * SECRET_SEARCH_LOAD_SECRETS, which silently discards load
>> +		 * failures and returns items whose secret is NULL.
>> +		 */
>> +		if (!secret_item_load_secret_sync(item, NULL, &error)) {
>> +			g_critical("could not load secret: %s", error->message);
>> +			g_error_free(error);
>> +			g_list_free_full(items, g_object_unref);
>> +			return EXIT_FAILURE;
>> +		}
>>   		secret = secret_item_get_secret(item);
>>   		attributes = secret_item_get_attributes(item);
>>   
>>
>> base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a

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!