Fold x509_name_{init,cleanup} into the classs
Those date to when we were writing this like C.
Change-Id: Id0fe506df65a7f2a5aea657c299a70d9b2bea24d
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/97151
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h
index d97396c..05ccd69 100644
--- a/crypto/x509/internal.h
+++ b/crypto/x509/internal.h
@@ -66,7 +66,7 @@
// (RFC 5280) and C type is `X509_NAME_ENTRY*`.
DECLARE_ASN1_ITEM(X509_NAME_ENTRY)
-struct X509_NAME_CACHE {
+struct X509NameCache {
static constexpr bool kAllowUniquePtr = true;
// canon contains the DER-encoded canonicalized X.509 Name, not including the
// outermost TLV.
@@ -77,8 +77,14 @@
class X509Name : public X509_name_st {
public:
- STACK_OF(X509_NAME_ENTRY) *entries = nullptr;
- mutable bssl::Atomic<bssl::X509_NAME_CACHE *> cache;
+ ~X509Name();
+
+ // TODO(crbug.com/42290036): Switch to `Vector<UniquePtr<X509_NAME_ENTRY>>`,
+ // which would save an allocation. Potentially `Vector<X509_NAME_ENTRY>` if we
+ // are willing to break pointer stability of entries after
+ // `X509_NAME_add_entry` or `X509_NAME_delete_entry`.
+ UniquePtr<STACK_OF(X509_NAME_ENTRY)> entries;
+ mutable Atomic<X509NameCache *> cache = nullptr;
} /* X509_NAME */;
BSSL_NAMESPACE_END
@@ -611,9 +617,6 @@
// TODO(https://crbug.com/boringssl/695): Remove this.
int DIST_POINT_set_dpname(DIST_POINT_NAME *dpn, X509_NAME *iname);
-void x509_name_init(X509_NAME *name);
-void x509_name_cleanup(X509_NAME *name);
-
// x509_parse_name parses a DER-encoded, X.509 Name from `cbs` and writes the
// result to `*out`. It returns one on success and zero on error.
int x509_parse_name(CBS *cbs, X509_NAME *out);
@@ -622,7 +625,7 @@
// result to `out`. It returns one on success and zero on error.
int x509_marshal_name(CBB *out, const X509_NAME *in);
-const X509_NAME_CACHE *x509_name_get_cache(const X509_NAME *name);
+const X509NameCache *x509_name_get_cache(const X509_NAME *name);
void x509_name_invalidate_cache(X509_NAME *name);
int x509_name_copy(X509_NAME *dst, const X509_NAME *src);
diff --git a/crypto/x509/v3_crld.cc b/crypto/x509/v3_crld.cc
index 372cb9c..86c048b 100644
--- a/crypto/x509/v3_crld.cc
+++ b/crypto/x509/v3_crld.cc
@@ -116,7 +116,7 @@
return -1;
}
int ret = X509V3_NAME_from_section(nm.get(), dnsect, MBSTRING_ASC);
- rnm.reset(std::exchange(FromOpaque(nm.get())->entries, nullptr));
+ rnm = std::move(FromOpaque(nm.get())->entries);
if (!ret || sk_X509_NAME_ENTRY_num(rnm.get()) <= 0) {
return -1;
}
@@ -424,7 +424,11 @@
print_gens(out, dpn->name.fullname, indent);
} else {
X509Name ntmp;
- ntmp.entries = dpn->name.relativename;
+ ntmp.entries.reset(sk_X509_NAME_ENTRY_deep_copy(
+ dpn->name.relativename, X509_NAME_ENTRY_dup, X509_NAME_ENTRY_free));
+ if (ntmp.entries == nullptr) {
+ return 0;
+ }
BIO_printf(out, "%*sRelative Name:\n%*s", indent, "", indent + 2, "");
X509_NAME_print_ex(out, &ntmp, 0, XN_FLAG_ONELINE);
BIO_puts(out, "\n");
diff --git a/crypto/x509/v3_ncons.cc b/crypto/x509/v3_ncons.cc
index 2a04719..f439395 100644
--- a/crypto/x509/v3_ncons.cc
+++ b/crypto/x509/v3_ncons.cc
@@ -296,11 +296,11 @@
// makes this comparison easy. It is matched if the constraint is a prefix of
// the name.
int nc_dn(const X509_NAME *nm, const X509_NAME *base) {
- const X509_NAME_CACHE *nm_cache = x509_name_get_cache(nm);
+ const X509NameCache *nm_cache = x509_name_get_cache(nm);
if (nm_cache == nullptr) {
return X509_V_ERR_OUT_OF_MEM;
}
- const X509_NAME_CACHE *base_cache = x509_name_get_cache(base);
+ const X509NameCache *base_cache = x509_name_get_cache(base);
if (base_cache == nullptr) {
return X509_V_ERR_OUT_OF_MEM;
}
diff --git a/crypto/x509/x509_cmp.cc b/crypto/x509/x509_cmp.cc
index b544351..f30f030 100644
--- a/crypto/x509/x509_cmp.cc
+++ b/crypto/x509/x509_cmp.cc
@@ -117,11 +117,11 @@
}
int X509_NAME_cmp(const X509_NAME *a, const X509_NAME *b) {
- const X509_NAME_CACHE *a_cache = x509_name_get_cache(a);
+ const X509NameCache *a_cache = x509_name_get_cache(a);
if (a_cache == nullptr) {
return -2;
}
- const X509_NAME_CACHE *b_cache = x509_name_get_cache(b);
+ const X509NameCache *b_cache = x509_name_get_cache(b);
if (b_cache == nullptr) {
return -2;
}
@@ -145,7 +145,7 @@
}
uint32_t X509_NAME_hash(const X509_NAME *x) {
- const X509_NAME_CACHE *cache = x509_name_get_cache(x);
+ const X509NameCache *cache = x509_name_get_cache(x);
if (cache == nullptr) {
return 0;
}
@@ -158,7 +158,7 @@
// this is reasonably efficient.
uint32_t X509_NAME_hash_old(const X509_NAME *x) {
- const X509_NAME_CACHE *cache = x509_name_get_cache(x);
+ const X509NameCache *cache = x509_name_get_cache(x);
if (cache == nullptr) {
return 0;
}
diff --git a/crypto/x509/x509_obj.cc b/crypto/x509/x509_obj.cc
index 667456f..f36676b 100644
--- a/crypto/x509/x509_obj.cc
+++ b/crypto/x509/x509_obj.cc
@@ -68,8 +68,8 @@
len--; // space for '\0'
l = 0;
- for (i = 0; i < sk_X509_NAME_ENTRY_num(name->entries); i++) {
- ne = sk_X509_NAME_ENTRY_value(name->entries, i);
+ for (i = 0; i < sk_X509_NAME_ENTRY_num(name->entries.get()); i++) {
+ ne = sk_X509_NAME_ENTRY_value(name->entries.get(), i);
n = OBJ_obj2nid(ne->object);
if ((n == NID_undef) || ((s = OBJ_nid2sn(n)) == nullptr)) {
i2t_ASN1_OBJECT(tmp_buf, sizeof(tmp_buf), ne->object);
diff --git a/crypto/x509/x509name.cc b/crypto/x509/x509name.cc
index 273d6b4..32c35ce 100644
--- a/crypto/x509/x509name.cc
+++ b/crypto/x509/x509name.cc
@@ -82,7 +82,7 @@
return 0;
}
const auto *impl = FromOpaque(name);
- return (int)sk_X509_NAME_ENTRY_num(impl->entries);
+ return (int)sk_X509_NAME_ENTRY_num(impl->entries.get());
}
int X509_NAME_get_index_by_NID(const X509_NAME *name, int nid, int lastpos) {
@@ -105,7 +105,7 @@
if (lastpos < 0) {
lastpos = -1;
}
- const STACK_OF(X509_NAME_ENTRY) *sk = impl->entries;
+ const STACK_OF(X509_NAME_ENTRY) *sk = impl->entries.get();
int n = (int)sk_X509_NAME_ENTRY_num(sk);
for (lastpos++; lastpos < n; lastpos++) {
const X509_NAME_ENTRY *ne = sk_X509_NAME_ENTRY_value(sk, lastpos);
@@ -121,10 +121,10 @@
return nullptr;
}
const auto *impl = FromOpaque(name);
- if (loc < 0 || sk_X509_NAME_ENTRY_num(impl->entries) <= (size_t)loc) {
+ if (loc < 0 || sk_X509_NAME_ENTRY_num(impl->entries.get()) <= (size_t)loc) {
return nullptr;
} else {
- return (sk_X509_NAME_ENTRY_value(impl->entries, loc));
+ return (sk_X509_NAME_ENTRY_value(impl->entries.get(), loc));
}
}
@@ -133,11 +133,11 @@
return nullptr;
}
const auto *impl = FromOpaque(name);
- if (loc < 0 || sk_X509_NAME_ENTRY_num(impl->entries) <= (size_t)loc) {
+ if (loc < 0 || sk_X509_NAME_ENTRY_num(impl->entries.get()) <= (size_t)loc) {
return nullptr;
}
- STACK_OF(X509_NAME_ENTRY) *sk = impl->entries;
+ STACK_OF(X509_NAME_ENTRY) *sk = impl->entries.get();
X509_NAME_ENTRY *ret = sk_X509_NAME_ENTRY_delete(sk, loc);
size_t n = sk_X509_NAME_ENTRY_num(sk);
x509_name_invalidate_cache(name);
@@ -211,13 +211,13 @@
}
auto *impl = FromOpaque(name);
if (impl->entries == nullptr) {
- impl->entries = sk_X509_NAME_ENTRY_new_null();
+ impl->entries.reset(sk_X509_NAME_ENTRY_new_null());
if (impl->entries == nullptr) {
return 0;
}
}
- STACK_OF(X509_NAME_ENTRY) *sk = impl->entries;
+ STACK_OF(X509_NAME_ENTRY) *sk = impl->entries.get();
int n = (int)sk_X509_NAME_ENTRY_num(sk);
if (loc > n) {
loc = n;
diff --git a/crypto/x509/x_name.cc b/crypto/x509/x_name.cc
index 2a60d33..5b5eb22 100644
--- a/crypto/x509/x_name.cc
+++ b/crypto/x509/x_name.cc
@@ -122,33 +122,18 @@
return copy.release();
}
-void bssl::x509_name_init(X509_NAME *name) {
- auto *impl = FromOpaque(name);
- OPENSSL_memset(impl, 0, sizeof(*impl));
-}
-
-void bssl::x509_name_cleanup(X509_NAME *name) {
- auto *impl = FromOpaque(name);
- sk_X509_NAME_ENTRY_pop_free(impl->entries, X509_NAME_ENTRY_free);
- Delete(impl->cache.exchange(nullptr));
-}
+bssl::X509Name::~X509Name() { Delete(cache.exchange(nullptr)); }
X509_NAME *X509_NAME_new() { return New<X509Name>(); }
-void X509_NAME_free(X509_NAME *name) {
- if (name != nullptr) {
- x509_name_cleanup(name);
- Delete(FromOpaque(name));
- }
-}
+void X509_NAME_free(X509_NAME *name) { Delete(FromOpaque(name)); }
int bssl::x509_parse_name(CBS *cbs, X509_NAME *out) {
auto *impl = FromOpaque(out);
- // Reset the old state.
- x509_name_cleanup(impl);
- x509_name_init(impl);
+ impl->entries = nullptr;
+ x509_name_invalidate_cache(impl);
- impl->entries = sk_X509_NAME_ENTRY_new_null();
+ impl->entries.reset(sk_X509_NAME_ENTRY_new_null());
if (impl->entries == nullptr) {
return 0;
}
@@ -172,7 +157,7 @@
return 0;
}
entry->set = set;
- if (!PushToStack(impl->entries, std::move(entry))) {
+ if (!PushToStack(impl->entries.get(), std::move(entry))) {
return 0;
}
}
@@ -185,18 +170,18 @@
static int x509_marshal_name_entries(CBB *out, const X509_NAME *name,
int canonicalize) {
auto *impl = FromOpaque(name);
- if (sk_X509_NAME_ENTRY_num(impl->entries) == 0) {
+ if (sk_X509_NAME_ENTRY_num(impl->entries.get()) == 0) {
return 1;
}
// Bootstrap the first RDN.
- int set = sk_X509_NAME_ENTRY_value(impl->entries, 0)->set;
+ int set = sk_X509_NAME_ENTRY_value(impl->entries.get(), 0)->set;
CBB rdn;
if (!CBB_add_asn1(out, &rdn, CBS_ASN1_SET)) {
return 0;
}
- for (const X509_NAME_ENTRY *entry : impl->entries) {
+ for (const X509_NAME_ENTRY *entry : impl->entries.get()) {
if (entry->set != set) {
// Flush the previous RDN and start a new one.
if (!CBB_flush_asn1_set_of(&rdn) ||
@@ -213,14 +198,14 @@
return CBB_flush_asn1_set_of(&rdn) && CBB_flush(out);
}
-const X509_NAME_CACHE *bssl::x509_name_get_cache(const X509_NAME *name) {
+const X509NameCache *bssl::x509_name_get_cache(const X509_NAME *name) {
auto *impl = FromOpaque(name);
- const X509_NAME_CACHE *cache = impl->cache.load();
+ const X509NameCache *cache = impl->cache.load();
if (cache != nullptr) {
return cache;
}
- UniquePtr<X509_NAME_CACHE> new_cache = MakeUnique<X509_NAME_CACHE>();
+ UniquePtr<X509NameCache> new_cache = MakeUnique<X509NameCache>();
// Cache the DER encoding, including the outer TLV.
ScopedCBB cbb;
CBB seq;
@@ -237,7 +222,7 @@
return nullptr;
}
- X509_NAME_CACHE *expected = nullptr;
+ X509NameCache *expected = nullptr;
if (impl->cache.compare_exchange_strong(expected, new_cache.get())) {
// We won the race. `impl` now owns `new_cache`.
return new_cache.release();
@@ -255,7 +240,7 @@
}
int bssl::x509_marshal_name(CBB *out, const X509_NAME *in) {
- const X509_NAME_CACHE *cache = x509_name_get_cache(in);
+ const X509NameCache *cache = x509_name_get_cache(in);
if (cache == nullptr) {
return 0;
}
@@ -263,7 +248,7 @@
}
int bssl::x509_name_copy(X509_NAME *dst, const X509_NAME *src) {
- const X509_NAME_CACHE *cache = x509_name_get_cache(src);
+ const X509NameCache *cache = x509_name_get_cache(src);
if (cache == nullptr) {
return 0;
}
@@ -305,7 +290,7 @@
OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER);
return -1;
}
- const X509_NAME_CACHE *cache = x509_name_get_cache(in);
+ const X509NameCache *cache = x509_name_get_cache(in);
if (cache == nullptr) {
return -1;
}
@@ -415,7 +400,7 @@
int X509_NAME_get0_der(const X509_NAME *nm, const unsigned char **out_der,
size_t *out_der_len) {
- const X509_NAME_CACHE *cache = x509_name_get_cache(nm);
+ const X509NameCache *cache = x509_name_get_cache(nm);
if (cache == nullptr) {
return 0;
}
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc
index b0ad3f1..088a2ce 100644
--- a/crypto/x509/x_x509.cc
+++ b/crypto/x509/x_x509.cc
@@ -47,10 +47,8 @@
X509Impl::X509Impl() : RefCounted(CheckSubClass()) {
asn1_string_init(&serialNumber, V_ASN1_INTEGER);
x509_algor_init(&tbs_sig_alg);
- x509_name_init(&issuer);
asn1_string_init(¬Before, -1);
asn1_string_init(¬After, -1);
- x509_name_init(&subject);
x509_pubkey_init(&key);
x509_algor_init(&sig_alg);
asn1_string_init(&signature, V_ASN1_BIT_STRING);
@@ -64,10 +62,8 @@
asn1_string_cleanup(&serialNumber);
x509_algor_cleanup(&tbs_sig_alg);
- x509_name_cleanup(&issuer);
asn1_string_cleanup(¬Before);
asn1_string_cleanup(¬After);
- x509_name_cleanup(&subject);
x509_pubkey_cleanup(&key);
ASN1_BIT_STRING_free(issuerUID);
ASN1_BIT_STRING_free(subjectUID);
