From d8ab30be9cc4d4e77008d4037e696bc41ce293f8 Mon Sep 17 00:00:00 2001 From: "Dr. David von Oheimb" Date: Fri, 8 Jan 2021 23:18:19 +0100 Subject: [PATCH] X509v3_get_ext_by_NID.pod: Add warning on counter-intuitive behavior of X509v3_delete_ext() etc. Also simplify two uses of these functions. Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/13711) --- crypto/ct/ct_sct_ctx.c | 5 +---- crypto/x509/v3_conf.c | 8 ++------ doc/man3/X509v3_get_ext_by_NID.pod | 11 ++++++++--- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/crypto/ct/ct_sct_ctx.c b/crypto/ct/ct_sct_ctx.c index a84c476caf8..353b5a7f0ed 100644 --- a/crypto/ct/ct_sct_ctx.c +++ b/crypto/ct/ct_sct_ctx.c @@ -168,15 +168,12 @@ int SCT_CTX_set1_cert(SCT_CTX *sctx, X509 *cert, X509 *presigner) * SCT. */ if (idx >= 0) { - X509_EXTENSION *ext; - /* Take a copy of certificate so we don't modify passed version */ pretmp = X509_dup(cert); if (pretmp == NULL) goto err; - ext = X509_delete_ext(pretmp, idx); - X509_EXTENSION_free(ext); + X509_EXTENSION_free(X509_delete_ext(pretmp, idx)); if (!ct_x509_cert_fixup(pretmp, presigner)) goto err; diff --git a/crypto/x509/v3_conf.c b/crypto/x509/v3_conf.c index 740108fefd9..9eda71348c5 100644 --- a/crypto/x509/v3_conf.c +++ b/crypto/x509/v3_conf.c @@ -295,12 +295,8 @@ static void delete_ext(STACK_OF(X509_EXTENSION) *sk, X509_EXTENSION *dext) ASN1_OBJECT *obj; obj = X509_EXTENSION_get_object(dext); - while ((idx = X509v3_get_ext_by_OBJ(sk, obj, -1)) >= 0) { - X509_EXTENSION *tmpext = X509v3_get_ext(sk, idx); - - X509v3_delete_ext(sk, idx); - X509_EXTENSION_free(tmpext); - } + while ((idx = X509v3_get_ext_by_OBJ(sk, obj, -1)) >= 0) + X509_EXTENSION_free(X509v3_delete_ext(sk, idx)); } /* diff --git a/doc/man3/X509v3_get_ext_by_NID.pod b/doc/man3/X509v3_get_ext_by_NID.pod index f77474ca804..79c68e1478b 100644 --- a/doc/man3/X509v3_get_ext_by_NID.pod +++ b/doc/man3/X509v3_get_ext_by_NID.pod @@ -74,9 +74,9 @@ looks for an extension of criticality B. A zero value for B looks for a non-critical extension a nonzero value looks for a critical extension. -X509v3_delete_ext() deletes the extension with index B from B. The -deleted extension is returned and must be freed by the caller. If B -is in invalid index value B is returned. +X509v3_delete_ext() deletes the extension with index B from B. +The deleted extension is returned and must be freed by the caller. +If B is in invalid index value B is returned. X509v3_add_ext() adds extension B to stack B<*x> at position B. If B is B<-1> the new extension is added to the end. If B<*x> is B @@ -111,6 +111,11 @@ error. These search functions start from the extension B the B parameter so it should initially be set to B<-1>, if it is set to zero the initial extension will not be checked. +=head1 BUGS + +X509v3_delete_ext() and its variants are a bit counter-intuitive +because these functions do not free the extension they delete. + =head1 RETURN VALUES X509v3_get_ext_count() returns the extension count. -- 2.47.2