[Swan-dev] Coverity Scan finds issue with programs/pluto/keys.c

[D. Hugh Redelmeier]

| From: Andrew Cagney <andrew.cagney at gmail.com>

| On Tue, 4 May 2021 at 01:15, D. Hugh Redelmeier <hugh at mimosa.com> wrote:
| 
| > I'm suspicious about the case where both calls to try_all_keys return
| > true.  I'll call this "double stop".

I should have called this double not-stop.

| /*
|  * Try all keys from PUBKEY_DB.
|  *
|  * Return true when searching should stop (not when it succeeded);
|  * return false when searching can continue.
|  *
|  *   Returns  FATAL_DIAG  KEY     tried_cnt
|  *    false                                 can try again
|  *    true    <valid>   <valid>     N/A     fatal error caused by KEY
|  *    true      NULL     NULL        0      no key found
|  *    true      NULL     NULL       >0      no key worked
|  *    true      NULL    <valid>     N/A     KEY worked
|  */

This table says that nothing else is meaningful when false is
returned.

| FATAIL_DIAG => KEY

According to the table, only if true is returned.

	returns && FATAL_DIAG => KEY

Nothing tests the return value of the second call.

Something is wrong.  Perhaps the table.

================


        bool stop = try_all_keys("peer", ike->sa.st_remote_certs.pubkey_db, &s);
	if (!stop) {
		stop = try_all_keys("preloaded", pluto_pubkeys, &s);
	}

Would be simpler as

        if (!try_all_keys("peer", ike->sa.st_remote_certs.pubkey_db, &s) {
		(void) try_all_keys("preloaded", pluto_pubkeys, &s);
	}

No variable "stop".  No dangling possibility that "stop" will be used
later.  No re-assignment.  No suggestion that the return value of the
second call might actually be used.