From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: fix use of X509_get_ext_count()
To: tech@openbsd.org
Date: Sat, 3 Feb 2024 17:54:27 +0100

> I wonder if it would make sense to split 'i' into two variables. One as
> int and another size_t one. Then the typecast in the second for loop is
> not needed.

Yes.

> Is the compiler smart enough to not constantly evaluate
> X509_get_ext_count() in the for loop? It should I guess.

I was thinking it would be since x is only passed to functions with a
const argument, but now that I inspected the disassembly, it did call it
every time. I suppose the constness of the functions we pass x to is not
enough to infer that, and they are in a shared library. There could
be global side effects, so it needs to call it in every iteration.

The call is super cheap, so I'm not sure it's worth micro-optimizing
this, but on the other hand, it's not intrusive and gives us another
explicit error.

Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
diff -u -p -r1.124 cert.c
--- cert.c	3 Feb 2024 14:43:15 -0000	1.124
+++ cert.c	3 Feb 2024 16:48:34 -0000
@@ -737,7 +737,8 @@ struct cert *
 cert_parse_pre(const char *fn, const unsigned char *der, size_t len)
 {
 	const unsigned char	*oder;
-	int			 i;
+	size_t			 j;
+	int			 i, extsz;
 	X509			*x = NULL;
 	X509_EXTENSION		*ext = NULL;
 	const X509_ALGOR	*palg;
@@ -808,8 +809,12 @@ cert_parse_pre(const char *fn, const uns
 		goto out;
 
 	/* Look for X509v3 extensions. */
+	if ((extsz = X509_get_ext_count(x)) <= 0) {
+		warnx("%s: certificate without X.509v3 extensions", fn);
+		goto out;
+	}
 
-	for (i = 0; i < X509_get_ext_count(x); i++) {
+	for (i = 0; i < extsz; i++) {
 		ext = X509_get_ext(x, i);
 		assert(ext != NULL);
 		obj = X509_EXTENSION_get_object(ext);
@@ -938,7 +943,7 @@ cert_parse_pre(const char *fn, const uns
 			    p.fn);
 			goto out;
 		}
-		for (i = 0; (size_t)i < p.res->asz; i++) {
+		for (j = 0; j < p.res->asz; j++) {
 			if (p.res->as[i].type == CERT_AS_INHERIT) {
 				warnx("%s: inherit elements not allowed in EE"
 				    " cert", p.fn);