Re: [PATCH GnuPG 5/7] g10: check null in assert

Sam James via Gnupg-devel <[email protected]> Thu, 29 Jan 2026 04:11:16 +0000
Newsgroups	gmane.comp.encryption.gpg.devel
Organization	Gentoo
Message-ID	<[email protected]>
Jacob Bachmeyer <[email protected]> writes:

> On 1/28/26 06:35, Sam James via Gnupg-devel wrote:
>> * g10/keyedit.c (keyedit_quick_revsig): Check 'keyblock' in log_assert.
>>
>> --
>>
>> Found by GCC's -fanalyzer.
>>
>> Signed-off-by: Sam James <[email protected]>
>> ---
>>   g10/keyedit.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/g10/keyedit.c b/g10/keyedit.c
>> index 970a72027..567de4efd 100644
>> --- a/g10/keyedit.c
>> +++ b/g10/keyedit.c
>> @@ -3306,8 +3306,8 @@ keyedit_quick_revsig (ctrl_t ctrl, const char *username, const char *sigtorev,
>>     err = quick_find_keyblock (ctrl, username, 0, &kdbhd, &keyblock);
>>     if (err)
>>       goto leave;
>> -  log_assert (keyblock->pkt->pkttype == PKT_PUBLIC_KEY
>> -              || keyblock->pkt->pkttype == PKT_SECRET_KEY);
>> +  log_assert (keyblock && (keyblock->pkt->pkttype == PKT_PUBLIC_KEY
>> +              || keyblock->pkt->pkttype == PKT_SECRET_KEY));
>>     primarypk = keyblock->pkt->pkt.public_key;
>>     primarykid = pk_keyid (primarypk);
>
> Minor nit:  this expression should either be split across three lines
> or the second line indented to line up after the opening paren.  As it
> stands, it is easy for a human to misread.
>

ack

> Alternately, is it possible for this code to be reached if keyblock is
> NULL?  Perhaps a better solution would be to change the "if (err)" to
> "if (err || !keyblock)" or the more verbose "if (err != 0 || keyblock
> == NULL)"?  (I assume that quick_find_keyblock always returns an error
> if keyblock is NULL upon return, but the analyzer does not know that.)
>

Yes, this is a pattern I saw a few times where it doesn't understand
error-return relations, unfortunately.

It does look into q_f_k, it just isn't able to deduce that they're tied,
even though when looking at it, it seems to be fine.

In this case, adding the check inside of an asset seemed reasonable to
me to make it clear and allow further analysis, but I can understand not
agreeing with that. I wouldn't want to litter the codebase with
suppressions either.
 
> Some future version of GCC can optimize the "|| !keyblock" away when
> it is able to prove that "!err" implies "keyblock".  Current GCC
> clearly cannot do this.
>
> Now that I look at this a bit more, I think we may have found another
> shortcoming in the analyzer: 

I plan on filing some GCC bugs for any analyzer shortcomings we end up
finding so the discussion is interesting, thanks.

> does log_assert crash the program on an
> assertion failure?  Does the analyzer know that?  If not, a NULL
> keyblock would be dereferenced on the very next line, but the analyzer
> sees that it was checked and is happy... oops.

It should do, because it's defined as:

# ifdef GPGRT_HAVE_MACRO_FUNCTION
#  define log_assert(expr)                                      \
  ((expr)                                                       \
   ? (void) 0                                                   \
   : _gpgrt_log_assert (#expr, __FILE__, __LINE__, __FUNCTION__))
# else /*!GPGRT_HAVE_MACRO_FUNCTION*/
#  define log_assert(expr)                                      \
  ((expr)                                                       \
   ? (void) 0                                                   \
   : _gpgrt_log_assert (#expr, __FILE__, __LINE__, NULL))
# endif /*!GPGRT_HAVE_MACRO_FUNCTION*/

and _gpgrt_log_assert is noreturn.

>
>
> -- Jacob

_______________________________________________
Gnupg-devel mailing list
[email protected]
https://lists.gnupg.org/mailman/listinfo/gnupg-devel
signature.asc (application/pgp-signature, 418 B)