From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: fix use of X509_get_ext_count()
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Sat, 3 Feb 2024 22:21:20 +0100

On Sat, Feb 03, 2024 at 05:57:18PM +0100, Theo Buehler wrote:
> On Sat, Feb 03, 2024 at 05:54:27PM +0100, Theo Buehler wrote:
> > > 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.
> 
> And here without indexing mistake.

This version is OK claudio@. Thanks
 
> 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:56:56 -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,8 +943,8 @@ cert_parse_pre(const char *fn, const uns
>  			    p.fn);
>  			goto out;
>  		}
> -		for (i = 0; (size_t)i < p.res->asz; i++) {
> -			if (p.res->as[i].type == CERT_AS_INHERIT) {
> +		for (j = 0; j < p.res->asz; j++) {
> +			if (p.res->as[j].type == CERT_AS_INHERIT) {
>  				warnx("%s: inherit elements not allowed in EE"
>  				    " cert", p.fn);
>  				goto out;
> 

-- 
:wq Claudio