commit 59906b3aa8d9f48ad7303edc540912bd588a8e46
author David Benjamin <davidben@google.com> Sat Nov 25 13:27:08 2023 -0500
committer Boringssl LUCI CQ <boringssl-scoped@luci-project-accounts.iam.gserviceaccount.com> Tue Dec 05 17:55:06 2023 +0000
tree 9b5ea3e0aea1f871fd51c774e08ae8cf1aef817c
parent 33a5e94645787d74593efa0e63200a31372325a2

Consistently call CRYPTO_free_ex_data first

(Or second, if there's a method table.)

CRYPTO_EX_free is passed the parent object, so the caller could, in
principle, inspect the object. We should pass in an object in a
self-consistent state. Also fix up the documentation. I think some bits
of a since removed CRYPTO_EX_new got jumbled up in there.

Change-Id: I316d00aee61bf544f59d4dac2efcd825e4bfa9b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64255
Reviewed-by: Bob Beck <bbe@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/fipsmodule/ec/ec_key.c

diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c
index a48671a..d42566a 100644
--- a/crypto/fipsmodule/ec/ec_key.c
+++ b/crypto/fipsmodule/ec/ec_key.c
@@ -163,12 +163,12 @@
     METHOD_unref(r->ecdsa_meth);
   }
 
+  CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), r, &r->ex_data);
+
   EC_GROUP_free(r->group);
   EC_POINT_free(r->pub_key);
   ec_wrapped_scalar_free(r->priv_key);
 
-  CRYPTO_free_ex_data(g_ec_ex_data_class_bss_get(), r, &r->ex_data);
-
   OPENSSL_free(r);
 }
 

crypto/x509/x509_vfy.c

diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index c56e174..09394cc 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -1717,11 +1717,9 @@
 }
 
 void X509_STORE_CTX_cleanup(X509_STORE_CTX *ctx) {
-  X509_VERIFY_PARAM_free(ctx->param);
-  ctx->param = NULL;
-  sk_X509_pop_free(ctx->chain, X509_free);
-  ctx->chain = NULL;
   CRYPTO_free_ex_data(&g_ex_data_class, ctx, &(ctx->ex_data));
+  X509_VERIFY_PARAM_free(ctx->param);
+  sk_X509_pop_free(ctx->chain, X509_free);
   OPENSSL_memset(ctx, 0, sizeof(X509_STORE_CTX));
 }
 

include/openssl/ex_data.h

diff --git a/include/openssl/ex_data.h b/include/openssl/ex_data.h
index 8f2f98b..dab5383 100644
--- a/include/openssl/ex_data.h
+++ b/include/openssl/ex_data.h
@@ -163,10 +163,11 @@
 // callback has been passed to |SSL_get_ex_new_index| then it may be called each
 // time an |SSL*| is destroyed.
 //
-// The callback is passed the new object (i.e. the |SSL*|) in |parent|. The
-// arguments |argl| and |argp| contain opaque values that were given to
-// |CRYPTO_get_ex_new_index|. The callback should return one on success, but
-// the value is ignored.
+// The callback is passed the to-be-destroyed object (i.e. the |SSL*|) in
+// |parent|. As |parent| will shortly be destroyed, callers must not perform
+// operations that would increment its reference count, pass ownership, or
+// assume the object outlives the function call. The arguments |argl| and |argp|
+// contain opaque values that were given to |CRYPTO_get_ex_new_index|.
 //
 // This callback may be called with a NULL value for |ptr| if |parent| has no
 // value set for this index. However, the callbacks may also be skipped entirely