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:
Show 73 quoted lines
> 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: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712aDaniel Martí <mvdan@mvdan.cc>