Switch easy fields in X509 to UniquePtr Embedded fields still need some work. Also extensions is tied up in some pointer-to-pointer calling convention in the awful config bits. Change-Id: I198403f0baf3a4453999a2677145a97d3f653ae1 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97153 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Adam Langley <agl@google.com>

diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index 2d5febc..ccdf687 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h

@@ -142,8 +142,8 @@
   ASN1_TIME notAfter;
   X509Name subject;
   X509Pubkey key;
-  ASN1_BIT_STRING *issuerUID = nullptr;            // [ 1 ] optional in v2
-  ASN1_BIT_STRING *subjectUID = nullptr;           // [ 2 ] optional in v2
+  UniquePtr<ASN1_BIT_STRING> issuerUID;            // [ 1 ] optional in v2
+  UniquePtr<ASN1_BIT_STRING> subjectUID;           // [ 2 ] optional in v2
   STACK_OF(X509_EXTENSION) *extensions = nullptr;  // [ 3 ] optional in v3
   // Certificate fields:
   X509_ALGOR sig_alg;
@@ -152,18 +152,18 @@
   // buf, if not nullptr, contains a copy of the serialized Certificate.
   // TODO(davidben): Now every parsed `X509` has an underlying `CRYPTO_BUFFER`,
   // but `X509`s created peacemeal do not. Can we make this more uniform?
-  CRYPTO_BUFFER *buf = nullptr;
+  UniquePtr<CRYPTO_BUFFER> buf;
   CRYPTO_EX_DATA ex_data;
   // These contain copies of various extension values
   long ex_pathlen = -1;
   uint32_t ex_flags = 0;
   uint32_t ex_kusage = 0;
   uint32_t ex_xkusage = 0;
-  ASN1_OCTET_STRING *skid = nullptr;
-  AUTHORITY_KEYID *akid = nullptr;
-  STACK_OF(DIST_POINT) *crldp = nullptr;
-  STACK_OF(GENERAL_NAME) *altname = nullptr;
-  NAME_CONSTRAINTS *nc = nullptr;
+  UniquePtr<ASN1_OCTET_STRING> skid;
+  UniquePtr<AUTHORITY_KEYID> akid;
+  UniquePtr<STACK_OF(DIST_POINT)> crldp;
+  UniquePtr<STACK_OF(GENERAL_NAME)> altname;
+  UniquePtr<NAME_CONSTRAINTS> nc;
   unsigned char cert_hash[SHA256_DIGEST_LENGTH] = {};
   bssl::X509_CERT_AUX *aux = nullptr;
   Mutex lock;

diff --git a/crypto/x509/t_x509.cc b/crypto/x509/t_x509.cc
index 1d7de41..a68a7a8 100644
--- a/crypto/x509/t_x509.cc
+++ b/crypto/x509/t_x509.cc

@@ -187,7 +187,7 @@
       if (BIO_printf(bp, "%8sIssuer Unique ID: ", "") <= 0) {
         return 0;
       }
-      if (!X509_signature_dump(bp, impl->issuerUID, 12)) {
+      if (!X509_signature_dump(bp, impl->issuerUID.get(), 12)) {
         return 0;
       }
     }
@@ -195,7 +195,7 @@
       if (BIO_printf(bp, "%8sSubject Unique ID: ", "") <= 0) {
         return 0;
       }
-      if (!X509_signature_dump(bp, impl->subjectUID, 12)) {
+      if (!X509_signature_dump(bp, impl->subjectUID.get(), 12)) {
         return 0;
       }
     }

diff --git a/crypto/x509/v3_ncons.cc b/crypto/x509/v3_ncons.cc
index f439395..d65febf 100644
--- a/crypto/x509/v3_ncons.cc
+++ b/crypto/x509/v3_ncons.cc

@@ -734,7 +734,7 @@
   // check.
   const auto *x_impl = FromOpaque(x);
   size_t name_count =
-      X509_NAME_entry_count(nm) + sk_GENERAL_NAME_num(x_impl->altname);
+      X509_NAME_entry_count(nm) + sk_GENERAL_NAME_num(x_impl->altname.get());
   size_t constraint_count = sk_GENERAL_SUBTREE_num(nc->permittedSubtrees) +
                             sk_GENERAL_SUBTREE_num(nc->excludedSubtrees);
   size_t check_count = constraint_count * name_count;
@@ -787,7 +787,7 @@
     }
   }
 
-  for (const GENERAL_NAME *gen : x_impl->altname) {
+  for (const GENERAL_NAME *gen : x_impl->altname.get()) {
     int r = nc_match(gen, nc, /*case_insensitive_exclude_localpart=*/false);
     if (r != X509_V_OK) {
       return r;

diff --git a/crypto/x509/v3_purp.cc b/crypto/x509/v3_purp.cc
index ec5aa37..c330eb8 100644
--- a/crypto/x509/v3_purp.cc
+++ b/crypto/x509/v3_purp.cc

@@ -167,13 +167,13 @@
 static int setup_crldp(X509 *x) {
   int j;
   auto *impl = FromOpaque(x);
-  impl->crldp = reinterpret_cast<STACK_OF(DIST_POINT) *>(
-      X509_get_ext_d2i(x, NID_crl_distribution_points, &j, nullptr));
+  impl->crldp.reset(reinterpret_cast<STACK_OF(DIST_POINT) *>(
+      X509_get_ext_d2i(x, NID_crl_distribution_points, &j, nullptr)));
   if (impl->crldp == nullptr && j != -1) {
     return 0;
   }
-  for (size_t i = 0; i < sk_DIST_POINT_num(impl->crldp); i++) {
-    if (!setup_dp(x, sk_DIST_POINT_value(impl->crldp, i))) {
+  for (size_t i = 0; i < sk_DIST_POINT_num(impl->crldp.get()); i++) {
+    if (!setup_dp(x, sk_DIST_POINT_value(impl->crldp.get(), i))) {
       return 0;
     }
   }
@@ -299,13 +299,13 @@
     impl->ex_flags |= EXFLAG_INVALID;
   }
 
-  impl->skid = reinterpret_cast<ASN1_OCTET_STRING *>(
-      X509_get_ext_d2i(x, NID_subject_key_identifier, &j, nullptr));
+  impl->skid.reset(reinterpret_cast<ASN1_OCTET_STRING *>(
+      X509_get_ext_d2i(x, NID_subject_key_identifier, &j, nullptr)));
   if (impl->skid == nullptr && j != -1) {
     impl->ex_flags |= EXFLAG_INVALID;
   }
-  impl->akid = reinterpret_cast<AUTHORITY_KEYID *>(
-      X509_get_ext_d2i(x, NID_authority_key_identifier, &j, nullptr));
+  impl->akid.reset(reinterpret_cast<AUTHORITY_KEYID *>(
+      X509_get_ext_d2i(x, NID_authority_key_identifier, &j, nullptr)));
   if (impl->akid == nullptr && j != -1) {
     impl->ex_flags |= EXFLAG_INVALID;
   }
@@ -313,18 +313,18 @@
   if (!X509_NAME_cmp(X509_get_subject_name(x), X509_get_issuer_name(x))) {
     impl->ex_flags |= EXFLAG_SI;
     // If SKID matches AKID also indicate self signed
-    if (X509_check_akid(x, impl->akid) == X509_V_OK &&
+    if (X509_check_akid(x, impl->akid.get()) == X509_V_OK &&
         !ku_reject(x, X509v3_KU_KEY_CERT_SIGN)) {
       impl->ex_flags |= EXFLAG_SS;
     }
   }
-  impl->altname = reinterpret_cast<STACK_OF(GENERAL_NAME) *>(
-      X509_get_ext_d2i(x, NID_subject_alt_name, &j, nullptr));
+  impl->altname.reset(reinterpret_cast<STACK_OF(GENERAL_NAME) *>(
+      X509_get_ext_d2i(x, NID_subject_alt_name, &j, nullptr)));
   if (impl->altname == nullptr && j != -1) {
     impl->ex_flags |= EXFLAG_INVALID;
   }
-  impl->nc = reinterpret_cast<NAME_CONSTRAINTS *>(
-      X509_get_ext_d2i(x, NID_name_constraints, &j, nullptr));
+  impl->nc.reset(reinterpret_cast<NAME_CONSTRAINTS *>(
+      X509_get_ext_d2i(x, NID_name_constraints, &j, nullptr)));
   if (impl->nc == nullptr && j != -1) {
     impl->ex_flags |= EXFLAG_INVALID;
   }
@@ -480,7 +480,7 @@
 
   const auto *subject_impl = FromOpaque(subject);
   if (subject_impl->akid) {
-    int ret = X509_check_akid(issuer, subject_impl->akid);
+    int ret = X509_check_akid(issuer, subject_impl->akid.get());
     if (ret != X509_V_OK) {
       return ret;
     }
@@ -500,7 +500,7 @@
   // Check key ids (if present)
   auto *issuer_impl = FromOpaque(issuer);
   if (akid->keyid && issuer_impl->skid &&
-      ASN1_OCTET_STRING_cmp(akid->keyid, issuer_impl->skid)) {
+      ASN1_OCTET_STRING_cmp(akid->keyid, issuer_impl->skid.get())) {
     return X509_V_ERR_AKID_SKID_MISMATCH;
   }
   // Check serial number
@@ -567,7 +567,7 @@
     return nullptr;
   }
   auto *impl = FromOpaque(x509);
-  return impl->skid;
+  return impl->skid.get();
 }
 
 const ASN1_OCTET_STRING *X509_get0_authority_key_id(X509 *x509) {

diff --git a/crypto/x509/x509_set.cc b/crypto/x509/x509_set.cc
index e55829f..970d2e6 100644
--- a/crypto/x509/x509_set.cc
+++ b/crypto/x509/x509_set.cc

@@ -138,10 +138,10 @@
                     const ASN1_BIT_STRING **out_subject_uid) {
   const auto *impl = FromOpaque(x509);
   if (out_issuer_uid != nullptr) {
-    *out_issuer_uid = impl->issuerUID;
+    *out_issuer_uid = impl->issuerUID.get();
   }
   if (out_subject_uid != nullptr) {
-    *out_subject_uid = impl->subjectUID;
+    *out_subject_uid = impl->subjectUID.get();
   }
 }
 

diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index 4e04c77..9e1f60c 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc

@@ -3268,12 +3268,12 @@
   UniquePtr<X509> root(X509_parse_from_buffer(buf.get()));
   ASSERT_TRUE(root);
 
-  EXPECT_EQ(buf.get(), FromOpaque(root.get())->buf);
+  EXPECT_EQ(buf.get(), FromOpaque(root.get())->buf.get());
   buf.reset();
 
   // This ensures the X509 took a reference to `buf`, otherwise this will be a
   // reference to free memory and ASAN should notice.
-  CRYPTO_BUFFER_len(FromOpaque(root.get())->buf);
+  CRYPTO_BUFFER_len(FromOpaque(root.get())->buf.get());
 }
 
 TEST(X509Test, TestFromBufferWithTrailingData) {
@@ -3332,7 +3332,7 @@
   size_t data2_len;
   UniquePtr<uint8_t> data2;
   ASSERT_TRUE(PEMToDER(&data2, &data2_len, kLeafPEM));
-  EXPECT_EQ(FromOpaque(root.get())->buf, buf.get());
+  EXPECT_EQ(FromOpaque(root.get())->buf.get(), buf.get());
 
   // Historically, this function tested the interaction between
   // `X509_parse_from_buffer` and object reuse. We no longer support object
@@ -3345,7 +3345,7 @@
   root.reset(raw);
 
   ASSERT_EQ(root.get(), ret);
-  ASSERT_NE(buf.get(), FromOpaque(root.get())->buf);
+  ASSERT_NE(buf.get(), FromOpaque(root.get())->buf.get());
 
   // Free `data2` and ensure that `root` took its own copy. Otherwise
   // serializing `root`, below, will trigger a use-after-free.

diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc
index 16625ba..ce54dd6 100644
--- a/crypto/x509/x509_vfy.cc
+++ b/crypto/x509/x509_vfy.cc

@@ -569,7 +569,7 @@
     // but if it includes constraints it is to be assumed it expects them
     // to be obeyed.
     for (j = (int)sk_X509_num(ctx->chain) - 1; j > i; j--) {
-      NAME_CONSTRAINTS *nc = FromOpaque(sk_X509_value(ctx->chain, j))->nc;
+      NAME_CONSTRAINTS *nc = FromOpaque(sk_X509_value(ctx->chain, j))->nc.get();
       if (nc) {
         has_name_constraints = 1;
         rv = NAME_CONSTRAINTS_check(x, nc);
@@ -1048,8 +1048,8 @@
       return 0;
     }
   }
-  for (size_t i = 0; i < sk_DIST_POINT_num(impl->crldp); i++) {
-    DIST_POINT *dp = sk_DIST_POINT_value(impl->crldp, i);
+  for (size_t i = 0; i < sk_DIST_POINT_num(impl->crldp.get()); i++) {
+    DIST_POINT *dp = sk_DIST_POINT_value(impl->crldp.get(), i);
     // Skip distribution points with a reasons field or a CRL issuer:
     //
     // We do not support CRLs partitioned by reason code. RFC 5280 requires CAs

diff --git a/crypto/x509/x_all.cc b/crypto/x509/x_all.cc
index 118c7b0..415d0fc 100644
--- a/crypto/x509/x_all.cc
+++ b/crypto/x509/x_all.cc

@@ -78,7 +78,6 @@
   }
 
   // Discard the cached encoding. (We just modified it.)
-  CRYPTO_BUFFER_free(impl->buf);
   impl->buf = nullptr;
 
   ScopedCBB cbb;

diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc
index 54c3a8c..740124b 100644
--- a/crypto/x509/x_x509.cc
+++ b/crypto/x509/x_x509.cc

@@ -63,17 +63,9 @@
   x509_algor_cleanup(&tbs_sig_alg);
   asn1_string_cleanup(&notBefore);
   asn1_string_cleanup(&notAfter);
-  ASN1_BIT_STRING_free(issuerUID);
-  ASN1_BIT_STRING_free(subjectUID);
   sk_X509_EXTENSION_pop_free(extensions, X509_EXTENSION_free);
   x509_algor_cleanup(&sig_alg);
   asn1_string_cleanup(&signature);
-  CRYPTO_BUFFER_free(buf);
-  ASN1_OCTET_STRING_free(skid);
-  AUTHORITY_KEYID_free(akid);
-  CRL_DIST_POINTS_free(crldp);
-  GENERAL_NAMES_free(altname);
-  NAME_CONSTRAINTS_free(nc);
   X509_CERT_AUX_free(aux);
 }
 
@@ -94,7 +86,7 @@
   }
 
   // Save the buffer to cache the original encoding.
-  ret->buf = UpRef(buf).release();
+  ret->buf = UpRef(buf);
 
   // Parse the Certificate.
   CBS cbs, cert, tbs;
@@ -157,17 +149,17 @@
   // Per RFC 5280, section 4.1.2.8, these fields require v2 or v3:
   if (ret->version >= X509_VERSION_2 &&
       CBS_peek_asn1_tag(&tbs, kIssuerUIDTag)) {
-    ret->issuerUID = ASN1_BIT_STRING_new();
+    ret->issuerUID.reset(ASN1_BIT_STRING_new());
     if (ret->issuerUID == nullptr ||
-        !asn1_parse_bit_string(&tbs, ret->issuerUID, kIssuerUIDTag)) {
+        !asn1_parse_bit_string(&tbs, ret->issuerUID.get(), kIssuerUIDTag)) {
       return nullptr;
     }
   }
   if (ret->version >= X509_VERSION_2 &&
       CBS_peek_asn1_tag(&tbs, kSubjectUIDTag)) {
-    ret->subjectUID = ASN1_BIT_STRING_new();
+    ret->subjectUID.reset(ASN1_BIT_STRING_new());
     if (ret->subjectUID == nullptr ||
-        !asn1_parse_bit_string(&tbs, ret->subjectUID, kSubjectUIDTag)) {
+        !asn1_parse_bit_string(&tbs, ret->subjectUID.get(), kSubjectUIDTag)) {
       return nullptr;
     }
   }
@@ -225,7 +217,7 @@
     // exactly what we parsed. The `CRYPTO_BUFFER` contains the full
     // Certificate, so we need to find the TBSCertificate portion.
     CBS cbs, cert, tbs;
-    CRYPTO_BUFFER_init_CBS(impl->buf, &cbs);
+    CRYPTO_BUFFER_init_CBS(impl->buf.get(), &cbs);
     if (!CBS_get_asn1(&cbs, &cert, CBS_ASN1_SEQUENCE) ||
         !CBS_get_asn1_element(&cert, &tbs, CBS_ASN1_SEQUENCE)) {
       // This should be impossible.
@@ -255,9 +247,10 @@
       !x509_marshal_name(&tbs, &impl->subject) ||
       !x509_marshal_public_key(&tbs, &impl->key) ||
       (impl->issuerUID != nullptr &&
-       !asn1_marshal_bit_string(&tbs, impl->issuerUID, kIssuerUIDTag)) ||
+       !asn1_marshal_bit_string(&tbs, impl->issuerUID.get(), kIssuerUIDTag)) ||
       (impl->subjectUID != nullptr &&
-       !asn1_marshal_bit_string(&tbs, impl->subjectUID, kSubjectUIDTag))) {
+       !asn1_marshal_bit_string(&tbs, impl->subjectUID.get(),
+                                kSubjectUIDTag))) {
     return 0;
   }
   if (impl->extensions != nullptr) {
@@ -451,7 +444,6 @@
 
 int i2d_re_X509_tbs(X509 *x509, uint8_t **outp) {
   auto *impl = FromOpaque(x509);
-  CRYPTO_BUFFER_free(impl->buf);
   impl->buf = nullptr;
   return i2d_X509_tbs(x509, outp);
 }