From 21185a7e94a5b6549f7d3d084fcbbe8c1ca8bd03 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 21 Feb 2024 22:31:00 +0000 Subject: [PATCH 01/12] Add misc. x509 un/lock and set1 functions The set1 functions are cribbed from [OpenSSL's implementation][1], but notably differ in that we don't free the `X509_OBJECT`. This is pursuant to OpenSSL's [own convention][2] around "set1" functions: > In the 1 version the ownership of the object is not passed to or > retained by the parent object. Instead a copy or "up ref" of the > object is performed. OpenSSL [frees][3] the input `X509_OBJECT`; we do not. [1]: https://github.com/openssl/openssl/blob/4a6f70c03182b421d326831532edca32bcdb3fb1/crypto/x509/x509_lu.c#L506 [2]: https://www.openssl.org/docs/man3.2/man7/ossl-guide-libraries-introduction.html [3]: https://github.com/openssl/openssl/blob/4a6f70c03182b421d326831532edca32bcdb3fb1/crypto/x509/x509_lu.c#L511 --- crypto/x509/x509_lu.c | 30 ++++++++++++++++++++++ crypto/x509/x509_test.cc | 55 +++++++++++++++++++++++++++++++++++++--- include/openssl/x509.h | 5 ++++ 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 364aa8a515..d407136a37 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -196,6 +196,16 @@ X509_STORE *X509_STORE_new(void) { return NULL; } +int X509_STORE_lock(X509_STORE *v) { + CRYPTO_MUTEX_lock_write(&v->objs_lock); + return 1; +} + +int X509_STORE_unlock(X509_STORE *v) { + CRYPTO_MUTEX_unlock_write(&v->objs_lock); + return 1; +} + int X509_STORE_up_ref(X509_STORE *store) { CRYPTO_refcount_inc(&store->references); return 1; @@ -405,6 +415,26 @@ X509_CRL *X509_OBJECT_get0_X509_CRL(const X509_OBJECT *a) { return a->data.crl; } +int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj) { + if (a == NULL || !X509_up_ref(obj)) { + return 0; + } + + a->type = X509_LU_X509; + a->data.x509 = obj; + return 1; +} + +int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj) { + if (a == NULL || !X509_CRL_up_ref(obj)) { + return 0; + } + + a->type = X509_LU_CRL; + a->data.crl = obj; + return 1; +} + static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, int type, X509_NAME *name, int *pnmatch) { X509_OBJECT stmp; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index ae2385c39e..7d40c24abe 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -1945,6 +1946,24 @@ TEST(X509Test, TestCRL) { ASSERT_EQ(nullptr, X509_OBJECT_get0_X509_CRL(&invalidCRL)); } +TEST(X509Test, TestX509GettersSetters) { + bssl::UniquePtr obj(X509_OBJECT_new()); + bssl::UniquePtr x509(CertFromPEM(kCRLTestRoot)); + bssl::UniquePtr crl(CRLFromPEM(kBasicCRL)); + + ASSERT_TRUE(obj); + ASSERT_TRUE(x509); + ASSERT_TRUE(crl); + + EXPECT_EQ(0, X509_OBJECT_get0_X509(obj.get())); + EXPECT_EQ(0, X509_OBJECT_get0_X509_CRL(obj.get())); + + EXPECT_EQ(1, X509_OBJECT_set1_X509(obj.get(), x509.get())); + EXPECT_EQ(x509.get(), X509_OBJECT_get0_X509(obj.get())); + EXPECT_EQ(1, X509_OBJECT_set1_X509_CRL(obj.get(), crl.get())); + EXPECT_EQ(crl.get(), X509_OBJECT_get0_X509_CRL(obj.get())); +} + TEST(X509Test, ManyNamesAndConstraints) { bssl::UniquePtr many_constraints(CertFromPEM( GetTestData("crypto/x509/test/many_constraints.pem").c_str())); @@ -5022,12 +5041,40 @@ TEST(X509Test, AddDuplicates) { ASSERT_TRUE(a); ASSERT_TRUE(b); + // To begin, add the certs to the store. Subsequent adds will be duplicative. EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + + const size_t kNumThreads = 50; + std::vector threads; + for (size_t i = 0; i < kNumThreads; i++) { + threads.emplace_back([&] { + // Firstly, save off |i| in the thread's context. + const size_t idx = i; + // 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] % 100)); + // Half the threads add duplicate certs, the other half take a lock and + // look them up to exercise un/locking functions. + if (idx % 2 == 0) { + EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); + EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); + } else { + ASSERT_TRUE(X509_STORE_lock(store.get())); + 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())); + } + }); + } + for (auto &thread : threads) { + thread.join(); + } EXPECT_EQ(sk_X509_OBJECT_num(X509_STORE_get0_objects(store.get())), 2u); } diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 6e09f02a1b..e73b9489ac 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2802,7 +2802,11 @@ 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); +OPENSSL_EXPORT int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj); +OPENSSL_EXPORT int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj); OPENSSL_EXPORT X509_STORE *X509_STORE_new(void); +OPENSSL_EXPORT int X509_STORE_lock(X509_STORE *v); +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); @@ -3124,6 +3128,7 @@ BORINGSSL_MAKE_UP_REF(X509_CRL, X509_CRL_up_ref) BORINGSSL_MAKE_DELETER(X509_EXTENSION, X509_EXTENSION_free) BORINGSSL_MAKE_DELETER(X509_INFO, X509_INFO_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) BORINGSSL_MAKE_DELETER(X509_PKEY, X509_PKEY_free) From a78d8d621775cbabb1ae8b9e89d4c986649c3fde Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 23 Feb 2024 03:22:38 +0000 Subject: [PATCH 02/12] Remove random jitter to appease MacOS in CI This commit was made because the MacOS CI jobs [seem to have taken issue][1] with how we were doing things previously. [1]: https://github.com/aws/aws-lc/actions/runs/8014000000/job/21891957419?pr=1449 --- crypto/x509/x509_test.cc | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7d40c24abe..f4a7249ef9 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -30,7 +30,6 @@ #include #include #include -#include #include #include @@ -5051,12 +5050,9 @@ TEST(X509Test, AddDuplicates) { threads.emplace_back([&] { // Firstly, save off |i| in the thread's context. const size_t idx = i; - // 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] % 100)); // Half the threads add duplicate certs, the other half take a lock and - // look them up to exercise un/locking functions. + // look them up to exercise un/locking functions. No sleep, let them + // contend as quickly as possible. if (idx % 2 == 0) { EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); From 066ef057d588a54c417e6a4f7fbfc968978ad27f Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 23 Feb 2024 15:49:22 +0000 Subject: [PATCH 03/12] Reinstate jitter, fix stack-use-after-scope ASAN error --- crypto/x509/x509_test.cc | 45 ++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index f4a7249ef9..e4c148c7eb 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -30,6 +30,7 @@ #include #include #include +#include #include #include @@ -5044,28 +5045,32 @@ TEST(X509Test, AddDuplicates) { EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - const size_t kNumThreads = 50; + // Half the threads add duplicate certs, the other half take a lock and + // look them up to exercise un/locking functions. + const size_t kNumThreads = 10; std::vector threads; - for (size_t i = 0; i < kNumThreads; i++) { + for (size_t i = 0; i < kNumThreads/2; i++) { threads.emplace_back([&] { - // Firstly, save off |i| in the thread's context. - const size_t idx = i; - // Half the threads add duplicate certs, the other half take a lock and - // look them up to exercise un/locking functions. No sleep, let them - // contend as quickly as possible. - if (idx % 2 == 0) { - EXPECT_TRUE(X509_STORE_add_cert(store.get(), a.get())); - EXPECT_TRUE(X509_STORE_add_cert(store.get(), b.get())); - } else { - ASSERT_TRUE(X509_STORE_lock(store.get())); - 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())); - } + // 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)); + 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)); + 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())); }); } for (auto &thread : threads) { From d31763aee927b512df5855142c9cf7fae2165a09 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 23 Feb 2024 15:56:03 +0000 Subject: [PATCH 04/12] Add more set1 coverage --- crypto/x509/x509_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index e4c148c7eb..7d7c16108e 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1957,6 +1957,8 @@ TEST(X509Test, TestX509GettersSetters) { 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())); EXPECT_EQ(1, X509_OBJECT_set1_X509(obj.get(), x509.get())); EXPECT_EQ(x509.get(), X509_OBJECT_get0_X509(obj.get())); From 6f23ceacf489a0804daf363b881338fe63c71194 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 23 Feb 2024 16:41:58 +0000 Subject: [PATCH 05/12] Free preexisting obj contents in setters --- crypto/x509/x509_lu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index d407136a37..216eea3d78 100644 --- a/crypto/x509/x509_lu.c +++ b/crypto/x509/x509_lu.c @@ -420,6 +420,7 @@ int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj) { return 0; } + X509_OBJECT_free_contents(a); a->type = X509_LU_X509; a->data.x509 = obj; return 1; @@ -430,6 +431,7 @@ int X509_OBJECT_set1_X509_CRL(X509_OBJECT *a, X509_CRL *obj) { return 0; } + X509_OBJECT_free_contents(a); a->type = X509_LU_CRL; a->data.crl = obj; return 1; From 2da663d43b4f61aaa3fa99d7ff097772a4691281 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Mon, 26 Feb 2024 19:54:49 +0000 Subject: [PATCH 06/12] Separate tip-of-main python CI job from releases --- .github/workflows/integrations.yml | 15 +++++++++++++-- tests/ci/integration/run_python_integration.sh | 8 ++++++-- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/.github/workflows/integrations.yml b/.github/workflows/integrations.yml index 87bd013f99..b1465fa801 100644 --- a/.github/workflows/integrations.yml +++ b/.github/workflows/integrations.yml @@ -91,7 +91,7 @@ jobs: - name: Run integration build run: | ./tests/ci/integration/run_socat_integration.sh - python: + python-main: runs-on: ubuntu-latest steps: - name: Install OS Dependencies @@ -101,7 +101,18 @@ jobs: - uses: actions/checkout@v3 - name: Build AWS-LC, build python, run tests run: | - ./tests/ci/integration/run_python_integration.sh + ./tests/ci/integration/run_python_integration.sh main + python-releases: + runs-on: ubuntu-latest + steps: + - name: Install OS Dependencies + run: | + sudo apt-get update + sudo apt-get -y --no-install-recommends install cmake gcc ninja-build golang make + - uses: actions/checkout@v3 + - name: Build AWS-LC, build python, run tests + run: | + ./tests/ci/integration/run_python_integration.sh 3.10 3.11 3.12 bind9: runs-on: ubuntu-latest steps: diff --git a/tests/ci/integration/run_python_integration.sh b/tests/ci/integration/run_python_integration.sh index 7bdc3245df..09bc983251 100755 --- a/tests/ci/integration/run_python_integration.sh +++ b/tests/ci/integration/run_python_integration.sh @@ -106,6 +106,11 @@ function python_patch() { done } +if [[ "$#" -eq "0" ]]; then + echo "No python branches provided for testing" + exit 1 +fi + mkdir -p ${SCRATCH_FOLDER} rm -rf ${SCRATCH_FOLDER}/* cd ${SCRATCH_FOLDER} @@ -126,9 +131,8 @@ pushd ${PYTHON_SRC_FOLDER} which sysctl && ( sysctl -w net.ipv6.conf.all.disable_ipv6=0 || /bin/true ) echo 0 >/proc/sys/net/ipv6/conf/all/disable_ipv6 || /bin/true -# NOTE: cpython keeps a unique branch per version, add version branches here # NOTE: As we add more versions to support, we may want to parallelize here -for branch in 3.10 3.11 3.12 main; do +for branch in "$@"; do python_patch ${branch} python_build ${branch} python_run_tests ${branch} From ac55b548304546d623e4a81a17f37fbace286b57 Mon Sep 17 00:00:00 2001 From: Will Childs-Klein Date: Wed, 28 Feb 2024 08:40:34 -0600 Subject: [PATCH 07/12] Update crypto/x509/x509_test.cc Co-authored-by: Andrew Hopkins --- crypto/x509/x509_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7d7c16108e..7806e8c098 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -1955,8 +1955,8 @@ TEST(X509Test, TestX509GettersSetters) { ASSERT_TRUE(x509); ASSERT_TRUE(crl); - EXPECT_EQ(0, X509_OBJECT_get0_X509(obj.get())); - EXPECT_EQ(0, X509_OBJECT_get0_X509_CRL(obj.get())); + EXPECT_EQ(nullptr, X509_OBJECT_get0_X509(obj.get())); + EXPECT_EQ(nullptr, 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())); From 269f97b06b91f21a42462fe666f73fa18af8ff38 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 28 Feb 2024 15:24:16 +0000 Subject: [PATCH 08/12] Implement PR feedback --- crypto/x509/x509_lu.c | 6 ++++++ crypto/x509/x509_test.cc | 12 +++++++----- include/openssl/x509.h | 11 +++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c index 216eea3d78..bcad1043d8 100644 --- a/crypto/x509/x509_lu.c +++ b/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; } diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7806e8c098..f27a1aabf5 100644 --- a/crypto/x509/x509_test.cc +++ b/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,7 +5056,7 @@ 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())); }); @@ -5064,8 +5064,10 @@ TEST(X509Test, AddDuplicates) { 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()) )); diff --git a/include/openssl/x509.h b/include/openssl/x509.h index e73b9489ac..2a022826b7 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2802,10 +2802,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); From c259e0d9eeeacd428aa580e64ede3de923a57f65 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 28 Feb 2024 15:35:20 +0000 Subject: [PATCH 09/12] Error out if we dont have patch for a version --- tests/ci/integration/run_python_integration.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/ci/integration/run_python_integration.sh b/tests/ci/integration/run_python_integration.sh index 09bc983251..3abd849b8f 100755 --- a/tests/ci/integration/run_python_integration.sh +++ b/tests/ci/integration/run_python_integration.sh @@ -95,6 +95,10 @@ function python_patch() { local branch=${1} local src_dir="${PYTHON_SRC_FOLDER}/${branch}" local patch_dir="${PYTHON_PATCH_FOLDER}/${branch}" + if [[ ! $(find -L ${patch_dir} -type f -name '*.patch') ]]; then + echo "No patch for ${branch}!" + exit 1 + fi git clone https://github.com/python/cpython.git ${src_dir} \ --depth 1 \ --branch ${branch} From 65683119934dbe915bccc4ce50680bdabbd13535 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 29 Feb 2024 15:03:39 +0000 Subject: [PATCH 10/12] Implement PR feedback --- crypto/x509/x509_test.cc | 3 ++- crypto/x509/x509cset.c | 3 +++ crypto/x509/x_x509.c | 3 +++ include/openssl/x509.h | 5 ++++- 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index f27a1aabf5..e0283dc70f 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -5066,7 +5066,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. + // for it. |X509_OBJECT_retrieve_by_subject| is called because it doesn't + // take a lock on the store, thus avoiding deadlock. 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()) diff --git a/crypto/x509/x509cset.c b/crypto/x509/x509cset.c index 6dd1fc0826..a35a086a19 100644 --- a/crypto/x509/x509cset.c +++ b/crypto/x509/x509cset.c @@ -138,6 +138,9 @@ int X509_CRL_sort(X509_CRL *c) { } int X509_CRL_up_ref(X509_CRL *crl) { + if (crl == NULL) { + return 0; + } CRYPTO_refcount_inc(&crl->references); return 1; } diff --git a/crypto/x509/x_x509.c b/crypto/x509/x_x509.c index 4f23fd68bb..963d9afd6f 100644 --- a/crypto/x509/x_x509.c +++ b/crypto/x509/x_x509.c @@ -198,6 +198,9 @@ X509 *X509_parse_from_buffer(CRYPTO_BUFFER *buf) { } int X509_up_ref(X509 *x) { + if (x == NULL) { + return 0; + } CRYPTO_refcount_inc(&x->references); return 1; } diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 2a022826b7..577d8b7893 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2814,7 +2814,10 @@ OPENSSL_EXPORT int X509_OBJECT_set1_X509(X509_OBJECT *a, X509 *obj); 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 +// X509_STORE_lock takes a write lock on |v|. return 1 on success, 0 on failure. +// +// Avoid using the store while it is locked; many functions take a lock +// internally, so operating on the store may cause a deadlock. 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); From 34f7a30f992a2ffdce37204d89760448bafd5ed1 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 29 Feb 2024 15:13:45 +0000 Subject: [PATCH 11/12] Clarify doc comment --- include/openssl/x509.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 577d8b7893..5d37df865b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2816,8 +2816,9 @@ 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. // -// Avoid using the store while it is locked; many functions take a lock -// internally, so operating on the store may cause a deadlock. +// Avoid operations on the X509_STORE or a X509_STORE_CTX containing it while +// it is locked; many |X509_STORE_*| functions take this lock internally which +// will cause a deadlock when called on a locked store. 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); From 0a4161a109010a100b65319acce786f0603869b9 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 29 Feb 2024 17:02:21 +0000 Subject: [PATCH 12/12] Fix doc comment --- include/openssl/x509.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 5d37df865b..efa160214b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2820,7 +2820,7 @@ OPENSSL_EXPORT X509_STORE *X509_STORE_new(void); // it is locked; many |X509_STORE_*| functions take this lock internally which // will cause a deadlock when called on a locked store. OPENSSL_EXPORT int X509_STORE_lock(X509_STORE *v); -// X509_STORE_lock releases a lock on |v|. return 1 on success, 0 on failure +// X509_STORE_unlock 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);