Skip to content

Commit

Permalink
Fix #272: run c_obj_create under a lock.
Browse files Browse the repository at this point in the history
See #272.

This commit adds a lock to run `c_obj_create` and `OBJ_create` thread-safely in
`OQS_PROVIDER_ENTRYPOINT_NAME`,
  • Loading branch information
thb-sb committed Nov 2, 2023
1 parent 4dac252 commit 8d3d336
Show file tree
Hide file tree
Showing 4 changed files with 129 additions and 8 deletions.
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ jobs:
name: Run tests
command: |
if << parameters.OQS_PROVIDER_BUILD_STATIC >> ; then
ctest --test-dir _build/
ctest --output-on-failure --test-dir _build/
else
./scripts/runtests.sh -V
fi
Expand Down
24 changes: 17 additions & 7 deletions oqsprov/oqsprov.c
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <openssl/core_names.h>
#include <openssl/err.h>
#include <openssl/objects.h>
#include <openssl/objectserr.h>
#include <openssl/params.h>
#include <openssl/provider.h>
#include <stdio.h>
Expand Down Expand Up @@ -917,6 +918,7 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
OSSL_PARAM version_request[] = {{"openssl-version", OSSL_PARAM_UTF8_PTR,
&opensslv, sizeof(&opensslv), 0},
{NULL, 0, NULL, 0, 0}};
unsigned long last_error = 0;

OQS_init();

Expand Down Expand Up @@ -968,10 +970,14 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
for (i = 0; i < OQS_OID_CNT; i += 2) {
if (!c_obj_create(handle, oqs_oid_alg_list[i], oqs_oid_alg_list[i + 1],
oqs_oid_alg_list[i + 1])) {
ERR_raise(ERR_LIB_USER, OQSPROV_R_OBJ_CREATE_ERR);
fprintf(stderr, "error registering NID for %s\n",
oqs_oid_alg_list[i + 1]);
goto end_init;
last_error = ERR_peek_last_error();
if ((last_error != ERR_PACK(ERR_LIB_OBJ, 0, OBJ_R_OID_EXISTS))
|| (OBJ_txt2nid(oqs_oid_alg_list[i]) == NID_undef)) {
ERR_raise(ERR_LIB_USER, OQSPROV_R_OBJ_CREATE_ERR);
fprintf(stderr, "error registering NID for %s\n",
oqs_oid_alg_list[i + 1]);
goto end_init;
}
}

/* create object (NID) again to avoid setup corner case problems
Expand Down Expand Up @@ -1016,9 +1022,13 @@ int OQS_PROVIDER_ENTRYPOINT_NAME(const OSSL_CORE_HANDLE *handle,
|| ((libctx = OSSL_LIB_CTX_new_child(handle, orig_in)) == NULL)
|| ((*provctx = oqsx_newprovctx(libctx, handle, corebiometh))
== NULL)) {
OQS_PROV_PRINTF("OQS PROV: error creating new provider context\n");
ERR_raise(ERR_LIB_USER, OQSPROV_R_LIB_CREATE_ERR);
goto end_init;
last_error = ERR_peek_last_error();
if ((last_error != ERR_PACK(ERR_LIB_OBJ, 0, OBJ_R_OID_EXISTS))
|| (OBJ_txt2nid(oqs_oid_alg_list[i]) == NID_undef)) {
OQS_PROV_PRINTF("OQS PROV: error creating new provider context\n");
ERR_raise(ERR_LIB_USER, OQSPROV_R_LIB_CREATE_ERR);
goto end_init;
}
}

*out = oqsprovider_dispatch_table;
Expand Down
12 changes: 12 additions & 0 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,18 @@ set_tests_properties(oqs_endecode
)
endif()

add_executable(oqs_test_multi_thread oqs_test_multi_thread.c)
target_link_libraries(oqs_test_multi_thread PRIVATE ${OPENSSL_CRYPTO_LIBRARY})
add_test(
NAME oqs_multi_thread
COMMAND oqs_test_multi_thread
"oqsprovider"
"${CMAKE_CURRENT_SOURCE_DIR}/openssl-ca.cnf"
)
set_tests_properties(oqs_multi_thread
PROPERTIES ENVIRONMENT "OPENSSL_MODULES=${OQS_PROV_BINARY_DIR}"
)

if (OQS_PROVIDER_BUILD_STATIC)
targets_set_static_provider(oqs_test_signatures
oqs_test_kems
Expand Down
99 changes: 99 additions & 0 deletions test/oqs_test_multi_thread.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// SPDX-License-Identifier: Apache-2.0 AND MIT

#include <pthread.h>

#include <openssl/crypto.h>
#include <openssl/provider.h>

#include "test_common.h"

static const size_t N_THREADS = 32;

static const char *kModuleName = NULL;
static const char *kConfigFile = NULL;

/** \brief Loads the oqs-provider in an `OSSL_LIB_CTX` object. */
static int load_oqs_provider_thread(OSSL_LIB_CTX *lib_ctx)
{
OSSL_PROVIDER *default_provider = NULL;
OSSL_PROVIDER *oqs_provider = NULL;
int ret = -1;

#ifdef OQS_PROVIDER_STATIC

if ((default_provider = OSSL_PROVIDER_load(lib_ctx, "default")) == NULL) {
goto end;
}
if (OSSL_PROVIDER_add_builtin(lib_ctx, "oqsprovider",
OQS_PROVIDER_ENTRYPOINT_NAME)
!= 1) {
putchar('-');
goto unload_default_provider;
}
if ((oqs_provider = OSSL_PROVIDER_load(lib_ctx, "oqsprovider")) == NULL) {
putchar('/');
goto unload_default_provider;
}
ret = 0;
OSS_PROVIDER_unload(oqs_provider);

#else

if (OSSL_LIB_CTX_load_config(lib_ctx, kConfigFile) == 1
&& OSSL_PROVIDER_available(lib_ctx, kModuleName)) {
putchar('>');
ret = 0;
}
goto end;

#endif // ifdef OQS_PROVIDER_STATIC

unload_default_provider:
OSSL_PROVIDER_unload(default_provider);

end:
return ret;
}

/** \brief Creates an OSSL_LIB_CTX object and loads oqs-provider. */
static void *thread_create_ossl_lib_ctx(void *arg)
{
OSSL_LIB_CTX *lib_ctx = NULL;
int ret = -1;

(void)arg;

if ((lib_ctx = OSSL_LIB_CTX_new()) == NULL) {
goto end;
}
ret = load_oqs_provider_thread(lib_ctx);
OSSL_LIB_CTX_free(lib_ctx);

end:
return (void *)(size_t)ret;
}

int main(int argc, char **argv)
{
size_t i;
pthread_t threads[N_THREADS];
void *result;
int ret = 0;

T(argc == 3);

kModuleName = argv[1];
kConfigFile = argv[2];

for (i = 0; i < N_THREADS; ++i) {
pthread_create(threads + i, NULL, thread_create_ossl_lib_ctx, NULL);
}

for (i = 0; i < N_THREADS; ++i) {
result = NULL;
pthread_join(threads[i], &result);
ret |= (int)(size_t)result;
}

return ret;
}

0 comments on commit 8d3d336

Please sign in to comment.