[Swan-dev] making coverity happy

[D. Hugh Redelmeier]

I just noticed this code in 0f69bb7d2d33c64739e83388e8a9e4d914a8971a

+                               struct payload_digest *p;
+                               for (p = md->chain[ISAKMP_NEXT_v2N]; p != NULL; p = p->next) {
+                                       if (p->payload.v2n.isan_type == v2N_SIGNATURE_HASH_ALGORITHMS)
+                                               break;
+                               }
+                               passert(p != NULL); /* Make Coverity happy */
+                               if (!negotiate_hash_algo_from_notification(p,st))
                                        return STF_FATAL;

The assertion may have made Coverity happy but it makes me nervous.
Based on purely local analysis (I haven't actually tried to understand
the code).

Is it OK for p to be NULL at this point?

- if so, the assertion is scary

- if not, the FOR loop condition is scary.

And another thing: are multiple v2N_SIGNATURE_HASH_ALGORITHMS
notification payloads legal?  If so, what should be done with them?
If not, what should be done with them?

This structure makes more sense to me:

+                               for (struct payload_digest *p = md->chain[ISAKMP_NEXT_v2N]; p != NULL; p = p->next) {
+                                       if (p->payload.v2n.isan_type == v2N_SIGNATURE_HASH_ALGORITHMS) {
+                                               if (!negotiate_hash_algo_from_notification(p,st))
+                                                       return STF_FATAL;
+                                               break;   /* are multiple legal ??? */
+                                       }
+                               }

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

There's another slight puzzle with this code

		if (seen_ntfy_hash) {
			if (!DBGP(IMPAIR_IGNORE_HASH_NOTIFY_REQUEST)) {
				st->st_seen_hashnotify = TRUE;
				...
			} else {
				st->st_seen_hashnotify = FALSE;
				...
			}
		}

What is st->st_seen_hashnotify set to when !seen_ntfy_hash?

Perhaps it defaults to FALSE.  If so, the last assignment to it is
redundant and thus confusing (at least to me).  If not, it feels like
a bug.

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

Along the same lines, could

		if (seen_ntfy_frag)
			st->st_seen_fragvid = TRUE;

be simplified to this?

		st->st_seen_fragvid = seen_ntfy_frag;

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

Perhaps, at an inconsequential cost in CPU time, the seen_ntfy_hash
variable could be eliminated, resulting in simpler code.  The loop is
moved up two levels in the control structure: one IF is eliminated and
one is moved inside the loop.

		for (struct payload_digest *p = md->chain[ISAKMP_NEXT_v2N]; p != NULL; p = p->next) {
			if (p->payload.v2n.isan_type == v2N_SIGNATURE_HASH_ALGORITHMS) {
				if (DBGP(IMPAIR_IGNORE_HASH_NOTIFY_REQUEST)) {
					libreswan_log("Impair: Ignoring the Signature hash notify in IKE_SA_INIT Request");
				} else {
					st->st_seen_hashnotify = TRUE;
					if (!negotiate_hash_algo_from_notification(p,st))
						return STF_FATAL;
				}
				break;	/* ??? what about multiple payloads */
			}
		}

(The declaration and setting of the local variable seen_ntfy_frag can 
then be deleted.)