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
