Re: rpki-client X509_free XXX fix

2021-11-04 Thread Sebastian Benoit 
Claudio Jeker(cje...@diehard.n-r-g.com) on 2021.11.04 18:31:54 +0100:
> There is this bit in parser.c
>   X509_free(x509); // needed? XXX
> 
> As tb@ properly noted this X509_free() is needed because the cert_parse()
> returns an up referenced x509 pointer back.
> 
> I moved the X509_free() so the error cases become simpler and we no longer
> leak a reference on success. At least this is how I read the code.
> 
> OK?

ok

> -- 
> :wq Claudio
> 
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 parser.c
> --- parser.c  4 Nov 2021 11:32:55 -   1.27
> +++ parser.c  4 Nov 2021 17:25:18 -
> @@ -232,12 +232,12 @@ proc_parser_cert(const struct entity *en
>   X509_STORE_CTX_cleanup(ctx);
>   sk_X509_free(chain);
>   sk_X509_CRL_free(crls);
> + X509_free(x509);
>  
>   cert->talid = a->cert->talid;
>  
>   /* Validate the cert to get the parent */
>   if (!valid_cert(entp->file, &auths, cert)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
> @@ -247,7 +247,6 @@ proc_parser_cert(const struct entity *en
>*/
>   if (cert->purpose == CERT_PURPOSE_CA) {
>   if (!auth_insert(&auths, cert, a)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
> @@ -318,20 +317,22 @@ proc_parser_root_cert(const struct entit
>   goto badcert;
>   }
>  
> + X509_free(x509);
> +
>   cert->talid = entp->talid;
>  
>   /*
>* Add valid roots to the RPKI auth tree.
>*/
>   if (!auth_insert(&auths, cert, NULL)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
>  
>   return cert;
> +
>   badcert:
> - X509_free(x509); // needed? XXX
> + X509_free(x509);
>   cert_free(cert);
>   return NULL;
>  }
>

Re: rpki-client X509_free XXX fix

2021-11-04 Thread Theo Buehler 
On Thu, Nov 04, 2021 at 06:31:54PM +0100, Claudio Jeker wrote:
> There is this bit in parser.c
>   X509_free(x509); // needed? XXX
> 
> As tb@ properly noted this X509_free() is needed because the cert_parse()
> returns an up referenced x509 pointer back.
>
> I moved the X509_free() so the error cases become simpler and we no longer
> leak a reference on success. At least this is how I read the code.

I agree with your reading.

I pondered your suggestion to stop taking the second reference. That
would also be an option, but it would break the symmetry with the parse
functions that wrap cms_parse_validate(), so I think your diff is the
right approach.

ok

> -- 
> :wq Claudio
> 
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 parser.c
> --- parser.c  4 Nov 2021 11:32:55 -   1.27
> +++ parser.c  4 Nov 2021 17:25:18 -
> @@ -232,12 +232,12 @@ proc_parser_cert(const struct entity *en
>   X509_STORE_CTX_cleanup(ctx);
>   sk_X509_free(chain);
>   sk_X509_CRL_free(crls);
> + X509_free(x509);
>  
>   cert->talid = a->cert->talid;
>  
>   /* Validate the cert to get the parent */
>   if (!valid_cert(entp->file, &auths, cert)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
> @@ -247,7 +247,6 @@ proc_parser_cert(const struct entity *en
>*/
>   if (cert->purpose == CERT_PURPOSE_CA) {
>   if (!auth_insert(&auths, cert, a)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
> @@ -318,20 +317,22 @@ proc_parser_root_cert(const struct entit
>   goto badcert;
>   }
>  
> + X509_free(x509);
> +
>   cert->talid = entp->talid;
>  
>   /*
>* Add valid roots to the RPKI auth tree.
>*/
>   if (!auth_insert(&auths, cert, NULL)) {
> - X509_free(x509); // needed? XXX
>   cert_free(cert);
>   return NULL;
>   }
>  
>   return cert;
> +
>   badcert:
> - X509_free(x509); // needed? XXX
> + X509_free(x509);
>   cert_free(cert);
>   return NULL;
>  }
>

rpki-client X509_free XXX fix

2021-11-04 Thread Claudio Jeker 
There is this bit in parser.c
X509_free(x509); // needed? XXX

As tb@ properly noted this X509_free() is needed because the cert_parse()
returns an up referenced x509 pointer back.

I moved the X509_free() so the error cases become simpler and we no longer
leak a reference on success. At least this is how I read the code.

OK?
-- 
:wq Claudio

Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.27
diff -u -p -r1.27 parser.c
--- parser.c4 Nov 2021 11:32:55 -   1.27
+++ parser.c4 Nov 2021 17:25:18 -
@@ -232,12 +232,12 @@ proc_parser_cert(const struct entity *en
X509_STORE_CTX_cleanup(ctx);
sk_X509_free(chain);
sk_X509_CRL_free(crls);
+   X509_free(x509);
 
cert->talid = a->cert->talid;
 
/* Validate the cert to get the parent */
if (!valid_cert(entp->file, &auths, cert)) {
-   X509_free(x509); // needed? XXX
cert_free(cert);
return NULL;
}
@@ -247,7 +247,6 @@ proc_parser_cert(const struct entity *en
 */
if (cert->purpose == CERT_PURPOSE_CA) {
if (!auth_insert(&auths, cert, a)) {
-   X509_free(x509); // needed? XXX
cert_free(cert);
return NULL;
}
@@ -318,20 +317,22 @@ proc_parser_root_cert(const struct entit
goto badcert;
}
 
+   X509_free(x509);
+
cert->talid = entp->talid;
 
/*
 * Add valid roots to the RPKI auth tree.
 */
if (!auth_insert(&auths, cert, NULL)) {
-   X509_free(x509); // needed? XXX
cert_free(cert);
return NULL;
}
 
return cert;
+
  badcert:
-   X509_free(x509); // needed? XXX
+   X509_free(x509);
cert_free(cert);
return NULL;
 }