Unexport various unused X509_OBJECT and X509_LOOKUP functions.

Some things of note:

- Anyone calling X509_OBJECT_up_ref_count is breaking X509_OBJECT's
  internal invariants, or relying on someone else handing back an
  X509_OBJECT with broken invariants.

- X509_LOOKUP_by_subject hands back an X509_OBJECT with broken internal
  invariants. Fortunately, it is never called, so unexport it as a the
  first step to cleaning this up.

Change-Id: Ia67693f802671cf857bf51aec6e20f27d1525212
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64130
Auto-Submit: David Benjamin <[email protected]>
Reviewed-by: Bob Beck <[email protected]>
Commit-Queue: David Benjamin <[email protected]>
(cherry picked from commit d9b81bb43a24b3adb6e8a616a4828e1bad89c486)

Author: davidben

crypto/x509/x509_lu.c

@@ -65,10 +65,22 @@
 #include "../internal.h"
 #include "internal.h"
 
-X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
-  X509_LOOKUP *ret;
 
-  ret = (X509_LOOKUP *)OPENSSL_zalloc(sizeof(X509_LOOKUP));
+static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
+                                      X509_NAME *name);
+static X509_OBJECT *X509_OBJECT_retrieve_by_subject(
+      STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name);
+static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
+                                               X509_OBJECT *x);
+static int X509_OBJECT_up_ref_count(X509_OBJECT *a);
+
+static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method);
+static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
+                                  X509_OBJECT *ret);
+static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx);
+
+static X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method) {
+  X509_LOOKUP *ret = OPENSSL_zalloc(sizeof(X509_LOOKUP));
   if (ret == NULL) {
     return NULL;
   }
@@ -91,18 +103,7 @@ void X509_LOOKUP_free(X509_LOOKUP *ctx) {
   OPENSSL_free(ctx);
 }
 
-int X509_LOOKUP_init(X509_LOOKUP *ctx) {
-  if (ctx->method == NULL) {
-    return 0;
-  }
-  if (ctx->method->init != NULL) {
-    return ctx->method->init(ctx);
-  } else {
-    return 1;
-  }
-}
-
-int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
+static int X509_LOOKUP_shutdown(X509_LOOKUP *ctx) {
   if (ctx->method == NULL) {
     return 0;
   }
@@ -125,14 +126,18 @@ int X509_LOOKUP_ctrl(X509_LOOKUP *ctx, int cmd, const char *argc, long argl,
   }
 }
 
-int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
-                           X509_OBJECT *ret) {
+static int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type, X509_NAME *name,
+                                  X509_OBJECT *ret) {
   if ((ctx->method == NULL) || (ctx->method->get_by_subject == NULL)) {
     return 0;
   }
   if (ctx->skip) {
     return 0;
   }
+  // Note |get_by_subject| leaves |ret| in an inconsistent state. It has
+  // pointers to an |X509| or |X509_CRL|, but has not bumped the refcount yet.
+  // For now, the caller is expected to fix this, but ideally we'd fix the
+  // |X509_LOOKUP| convention itself.
   return ctx->method->get_by_subject(ctx, type, name, ret) > 0;
 }
 
@@ -365,7 +370,7 @@ void X509_OBJECT_free(X509_OBJECT *obj) {
   OPENSSL_free(obj);
 }
 
-int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
+static int X509_OBJECT_up_ref_count(X509_OBJECT *a) {
   switch (a->type) {
     case X509_LU_X509:
       X509_up_ref(a->data.x509);
@@ -473,13 +478,13 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type,
   return (int)idx;
 }
 
-int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
-                               X509_NAME *name) {
+static int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
+                                      X509_NAME *name) {
   return x509_object_idx_cnt(h, type, name, NULL);
 }
 
-X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, int type,
-                                             X509_NAME *name) {
+X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h,
+                                                    int type, X509_NAME *name) {
   int idx;
   idx = X509_OBJECT_idx_by_subject(h, type, name);
   if (idx == -1) {
@@ -574,8 +579,8 @@ STACK_OF(X509_CRL) *X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm) {
   return sk;
 }
 
-X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
-                                        X509_OBJECT *x) {
+static X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
+                                               X509_OBJECT *x) {
   sk_X509_OBJECT_sort(h);
   size_t idx;
   if (!sk_X509_OBJECT_find_awslc(h, &idx, x)) {

crypto/x509/x509_test.cc

@@ -1851,7 +1851,7 @@ TEST(X509Test, MatchFoundSetsPeername) {
   char *peername = nullptr;
   EXPECT_NE(1, X509_check_host(leaf.get(), kWrongHostname, strlen(kWrongHostname), 0, &peername));
   ASSERT_EQ(nullptr, peername);
-  
+
   EXPECT_EQ(1, X509_check_host(leaf.get(), kHostname, strlen(kHostname), 0, &peername));
   EXPECT_STREQ(peername, kHostname);
   OPENSSL_free(peername);
@@ -5086,15 +5086,8 @@ TEST(X509Test, AddDuplicates) {
       ASSERT_TRUE(X509_STORE_lock(store.get()));
       // Sleep after taking the lock to cause contention. Sleep longer than the
       // adder half of threads to ensure we hold the lock while they contend
-      // for it. |X509_OBJECT_retrieve_by_subject| is called because it doesn't
-      // take a lock on the store, thus avoiding deadlock.
+      // for it.
       std::this_thread::sleep_for(std::chrono::microseconds(11 + (sleep_buf[0] % 5)));
-      EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
-        store->objs, X509_LU_X509, X509_get_subject_name(a.get())
-      ));
-      EXPECT_TRUE(X509_OBJECT_retrieve_by_subject(
-        store->objs, X509_LU_X509, X509_get_subject_name(b.get())
-      ));
       ASSERT_TRUE(X509_STORE_unlock(store.get()));
     });
   }

include/openssl/x509.h

@@ -2658,6 +2658,13 @@ OPENSSL_EXPORT int X509_NAME_get_text_by_NID(const X509_NAME *name, int nid,
 OPENSSL_EXPORT X509_STORE_CTX *X509_STORE_CTX_get0_parent_ctx(
     X509_STORE_CTX *ctx);
 
+// X509_LOOKUP_free releases memory associated with |ctx|. This function should
+// never be used outside the library. No function in the public API hands
+// ownership of an |X509_LOOKUP| to the caller.
+//
+// TODO(davidben): Unexport this function after rust-openssl is fixed to no
+// longer call it.
+OPENSSL_EXPORT void X509_LOOKUP_free(X509_LOOKUP *ctx);
 
 // Private structures.
 
@@ -3016,13 +3023,6 @@ OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_new(void);
 // X509_OBJECT_free frees an |X509_OBJECT| from the heap.
 OPENSSL_EXPORT void X509_OBJECT_free(X509_OBJECT *a);
 
-OPENSSL_EXPORT int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h,
-                                              int type, X509_NAME *name);
-OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_retrieve_by_subject(
-    STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name);
-OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h,
-                                                       X509_OBJECT *x);
-
 // X509_OBJECT_new returns a newly-allocated, empty |X509_OBJECT| or NULL on
 // error.
 OPENSSL_EXPORT X509_OBJECT *X509_OBJECT_new(void);
@@ -3038,7 +3038,6 @@ OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *obj);
 // a certificate.
 OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *obj);
 
-OPENSSL_EXPORT int X509_OBJECT_up_ref_count(X509_OBJECT *a);
 OPENSSL_EXPORT int X509_OBJECT_get_type(const X509_OBJECT *a);
 OPENSSL_EXPORT X509 *X509_OBJECT_get0_X509(const X509_OBJECT *a);
 // X509_OBJECT_get0_X509_CRL returns the |X509_CRL| associated with |a|
@@ -3182,13 +3181,6 @@ OPENSSL_EXPORT int X509_load_crl_file(X509_LOOKUP *ctx, const char *file,
 OPENSSL_EXPORT int X509_load_cert_crl_file(X509_LOOKUP *ctx, const char *file,
                                            int type);
 
-OPENSSL_EXPORT X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method);
-OPENSSL_EXPORT void X509_LOOKUP_free(X509_LOOKUP *ctx);
-OPENSSL_EXPORT int X509_LOOKUP_init(X509_LOOKUP *ctx);
-OPENSSL_EXPORT int X509_LOOKUP_by_subject(X509_LOOKUP *ctx, int type,
-                                          X509_NAME *name, X509_OBJECT *ret);
-OPENSSL_EXPORT int X509_LOOKUP_shutdown(X509_LOOKUP *ctx);
-
 OPENSSL_EXPORT int X509_STORE_load_locations(X509_STORE *ctx, const char *file,
                                              const char *dir);
 OPENSSL_EXPORT int X509_STORE_set_default_paths(X509_STORE *ctx);
@@ -3379,7 +3371,6 @@ BORINGSSL_MAKE_DELETER(X509_ATTRIBUTE, X509_ATTRIBUTE_free)
 BORINGSSL_MAKE_DELETER(X509_CRL, X509_CRL_free)
 BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref)
 BORINGSSL_MAKE_DELETER(X509_EXTENSION, X509_EXTENSION_free)
-BORINGSSL_MAKE_DELETER(X509_LOOKUP, X509_LOOKUP_free)
 BORINGSSL_MAKE_DELETER(X509_OBJECT, X509_OBJECT_free)
 BORINGSSL_MAKE_DELETER(X509_NAME, X509_NAME_free)
 BORINGSSL_MAKE_DELETER(X509_NAME_ENTRY, X509_NAME_ENTRY_free)