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

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.

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

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


-- Jacob



_______________________________________________
Gnupg-devel mailing list
[email protected]
https://lists.gnupg.org/mailman/listinfo/gnupg-devel