Implement PR feedback

crypto/x509/x509_lu.c

@@ -197,11 +197,17 @@ X509_STORE *X509_STORE_new(void) {
 }
 
 int X509_STORE_lock(X509_STORE *v) {
+    if (v == NULL) {
+      return 0;
+    }
     CRYPTO_MUTEX_lock_write(&v->objs_lock);
     return 1;
 }
 
 int X509_STORE_unlock(X509_STORE *v) {
+    if (v == NULL) {
+      return 0;
+    }
     CRYPTO_MUTEX_unlock_write(&v->objs_lock);
     return 1;
 }

crypto/x509/x509_test.cc

@@ -1955,8 +1955,8 @@ TEST(X509Test, TestX509GettersSetters) {
   ASSERT_TRUE(x509);
   ASSERT_TRUE(crl);
 
-  EXPECT_EQ(nullptr, X509_OBJECT_get0_X509(obj.get()));
-  EXPECT_EQ(nullptr, X509_OBJECT_get0_X509_CRL(obj.get()));
+  EXPECT_EQ(0, X509_OBJECT_get0_X509(obj.get()));
+  EXPECT_EQ(0, X509_OBJECT_get0_X509_CRL(obj.get()));
   EXPECT_EQ(0, X509_OBJECT_set1_X509(nullptr, x509.get()));
   EXPECT_EQ(0, X509_OBJECT_set1_X509_CRL(nullptr, crl.get()));
 
@@ -5056,16 +5056,18 @@ TEST(X509Test, AddDuplicates) {
       // Sleep with some jitter to offset thread execution
       uint8_t sleep_buf[1];
       ASSERT_TRUE(RAND_bytes(sleep_buf, sizeof(sleep_buf)));
-      std::this_thread::sleep_for(std::chrono::milliseconds(sleep_buf[0] % 10));
+      std::this_thread::sleep_for(std::chrono::microseconds(1 + (sleep_buf[0] % 5)));
       EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get()));
       EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get()));
     });
     threads.emplace_back([&] {
       uint8_t sleep_buf[1];
       ASSERT_TRUE(RAND_bytes(sleep_buf, sizeof(sleep_buf)));
       ASSERT_TRUE(X509_STORE_lock(store.get()));
-      // Sleep after taking the lock to cause contention
-      std::this_thread::sleep_for(std::chrono::milliseconds(sleep_buf[0] % 10));
+      // 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.
+      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())
       ));

include/openssl/x509.h

@@ -2800,10 +2800,21 @@ 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|
 OPENSSL_EXPORT X509_CRL *X509_OBJECT_get0_X509_CRL(const X509_OBJECT *a);
+
+// X509_OBJECT_set1_X509 sets |obj| on |a| and uprefs |obj|. As with other set1
+// methods, |a| does not take ownership of |obj|; the caller is responsible for
+// managing freeing |obj| when appropriate.
 OPENSSL_EXPORT int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj);
+
+// X509_OBJECT_set1_X509_CRL sets CRL |obj| on |a| and uprefs |obj|. As with
+// other set1 methods, |a| does not take ownership of |obj|; the caller is
+// responsible for managing freeing |obj| when appropriate.
 OPENSSL_EXPORT int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj);
+
 OPENSSL_EXPORT X509_STORE *X509_STORE_new(void);
+// X509_STORE_lock takes a write lock on |v|. return 1 on success, 0 on failure
 OPENSSL_EXPORT int X509_STORE_lock(X509_STORE *v);
+// X509_STORE_lock releases a lock on |v|. return 1 on success, 0 on failure
 OPENSSL_EXPORT int X509_STORE_unlock(X509_STORE *v);
 OPENSSL_EXPORT int X509_STORE_up_ref(X509_STORE *store);
 OPENSSL_EXPORT void X509_STORE_free(X509_STORE *v);