Lines suspicious of being a memory leak in smime.c about neomutt HOT 2 CLOSED

I don't see ... that it would require the caller to make a copy

I think we're OK. The jobs of the dialogs is to select a key, but as you've spotted they return differently.

By contrast, in dlg_gpgme(), the user selection causes the key to be copied:

neomutt/ncrypt/gpgme_functions.c

Lines 728 to 785 in 07e12e7

	static int op_generic_select_entry(struct GpgmeData *gd, int op)
	{
	const int index = menu_get_index(gd->menu);
	struct CryptKeyInfo *cur_key = gd->key_table[index];
	/* FIXME make error reporting more verbose - this should be
	* easy because GPGME provides more information */
	if (OptPgpCheckTrust)
	{
	if (!crypt_key_is_valid(cur_key))
	{
	mutt_error(_("This key can't be used: expired/disabled/revoked"));
	return FR_ERROR;
	}
	}
	if (OptPgpCheckTrust && (!crypt_id_is_valid(cur_key) || !crypt_id_is_strong(cur_key)))
	{
	const char *warn_s = NULL;
	char buf2[1024];
	if (cur_key->flags & KEYFLAG_CANTUSE)
	{
	warn_s = _("ID is expired/disabled/revoked. Do you really want to use the key?");
	}
	else
	{
	warn_s = "??";
	switch (cur_key->validity)
	{
	case GPGME_VALIDITY_NEVER:
	warn_s = _("ID is not valid. Do you really want to use the key?");
	break;
	case GPGME_VALIDITY_MARGINAL:
	warn_s = _("ID is only marginally valid. Do you really want to use the key?");
	break;
	case GPGME_VALIDITY_FULL:
	case GPGME_VALIDITY_ULTIMATE:
	break;
	case GPGME_VALIDITY_UNKNOWN:
	case GPGME_VALIDITY_UNDEFINED:
	warn_s = _("ID has undefined validity. Do you really want to use the key?");
	break;
	}
	}
	snprintf(buf2, sizeof(buf2), "%s", warn_s);
	if (query_yesorno(buf2, MUTT_NO) != MUTT_YES)
	{
	mutt_clear_error();
	return FR_NO_ACTION;
	}
	}
	gd->key = crypt_copy_key(cur_key);
	gd->done = true;
	return FR_SUCCESS;
	}

Ahh, this is what I was looking for but didn't find.

Both methods work and I don't think either leaks. My preference of the two styles is for dlg_smime() -- the dialog selects a key from a list, but nothing else.

Agree.

from neomutt.

SMIME code, probably a couple of memory leaks:

Entirely possible, the code's seldom been looked at.

I don't see ... that it would require the caller to make a copy

I think we're OK.
The jobs of the dialogs is to select a key, but as you've spotted they return differently.

In smime_get_key_by_addr(), we get all the possible keys:

and filter it down into matches, then have one of several paths:

In the Dialog path, the user selects a key -- this is one of the keys in matches:

which is returned:

All the paths in smime_get_key_by_addr() now call return_key = smime_copy_key() on the selection and then free matches:

Finally, smime_get_key_by_addr() returns the single selected key:

smime_get_key_by_str() follows the same pattern, but more simply.

It is almost identical to dlg_gpgme(), which doesn't require it.

By contrast, in dlg_gpgme(), the user selection causes the key to be copied:

then crypt_getkeybyaddr() keeps track of the copied key, frees matches and returns the selected key:

crypt_getkeybystr() works the same way.

Both methods work and I don't think either leaks.
My preference of the two styles is for dlg_smime() -- the dialog selects a key from a list, but nothing else.

Looking through the code, there's room for improvement.
We could use TAILQ or ARRAY, rather than a homegrown List implementation,
but those upgrades might require a lot of changes.

from neomutt.