Fallback to a read lock on fork preparation

.gitignore

@@ -13,6 +13,7 @@ tests/*.log
 tests/*.trs
 tests/pincache
 tests/tfork
+tests/tfork_deadlock
 tests/tlsctx
 tests/tcmpkeys
 

src/encoder.c

@@ -470,37 +470,38 @@ static P11PROV_PK11_URI *p11prov_encoder_private_key_to_asn1(P11PROV_CTX *pctx,
                                                              P11PROV_OBJ *key)
 {
     P11PROV_PK11_URI *out = NULL;
+    char *uri = NULL;
+    size_t uri_len;
+    int ret = RET_OSSL_ERR;
 
-    char *uri = p11prov_key_to_uri(pctx, key);
+    uri = p11prov_key_to_uri(pctx, key);
     if (!uri) {
-        goto error;
+        goto done;
     }
 
-    size_t uri_len = strlen(uri);
+    uri_len = strlen(uri);
     P11PROV_debug("uri=%s", uri);
 
     out = P11PROV_PK11_URI_new();
     if (!out) {
-        goto error;
+        goto done;
     }
 
     if (!ASN1_STRING_set(out->desc, P11PROV_DESCS_URI_FILE,
                          sizeof(P11PROV_DESCS_URI_FILE) - 1)) {
-        goto error;
+        goto done;
     }
     if (!ASN1_STRING_set(out->uri, uri, uri_len)) {
-        goto error;
+        goto done;
     }
 
-    goto done;
-
-error:
-    P11PROV_PK11_URI_free(out);
-    out = NULL;
+    ret = RET_OSSL_OK;
 
 done:
-    if (uri) {
-        free(uri);
+    OPENSSL_free(uri);
+    if (ret != RET_OSSL_OK) {
+        P11PROV_PK11_URI_free(out);
+        out = NULL;
     }
     return out;
 }

src/slot.c

@@ -246,11 +246,27 @@ void p11prov_slot_fork_prepare(P11PROV_SLOTS_CTX *sctx)
 {
     int err;
 
-    err = pthread_rwlock_wrlock(&sctx->rwlock);
+    /* attempt to get a write lock if possible, but fall back to a mere
+     * read lock if not possible (for example because it would cause
+     * a deadlock).
+     * Holding a write lock here is slightly preferable in case the
+     * application decides to create threads immediately after the fork
+     * within an atfork handler that runs before ours.
+     * Holding a write lock will prevent other threads from grabbing a
+     * read lock before we can reset the locks. However we assume this
+     * scenario to be mostly hypothetical and exceedingly rare given most
+     * forks result in a exec(), and atfork() is also a rarely used
+     * function, so falling back to a read lock to avoid deadlocks is ok
+     * in the vast majority of use cases.
+     */
+    err = pthread_rwlock_trywrlock(&sctx->rwlock);
     if (err != 0) {
-        err = errno;
-        P11PROV_debug("Failed to get slots lock (errno:%d)", err);
-        return;
+        err = pthread_rwlock_rdlock(&sctx->rwlock);
+        if (err != 0) {
+            err = errno;
+            P11PROV_debug("Failed to get slots lock (errno:%d)", err);
+            return;
+        }
     }
 }
 

tests/Makefile.am

@@ -10,7 +10,8 @@ testssrcdir=@abs_srcdir@
 #VALGRIND_SUPPRESSIONS_FILES = $(top_srcdir)/tests/pkcs11-provider.supp
 VALGRIND_FLAGS = --num-callers=30 -q --keep-debuginfo=yes
 
-check_PROGRAMS = tsession tgenkey tlsctx tdigests tdigest_dupctx treadkeys tcmpkeys tfork pincache
+check_PROGRAMS = tsession tgenkey tlsctx tdigests tdigest_dupctx treadkeys \
+		 tcmpkeys tfork pincache tfork_deadlock
 
 tsession_SOURCES = tsession.c
 tsession_CFLAGS = $(STD_CFLAGS) $(OPENSSL_CFLAGS)
@@ -48,6 +49,10 @@ pincache_SOURCES = pincache.c
 pincache_CFLAGS = $(STD_CFLAGS) $(OPENSSL_CFLAGS)
 pincache_LDADD = $(OPENSSL_LIBS)
 
+tfork_deadlock_SOURCES = tfork_deadlock.c
+tfork_deadlock_CFLAGS = $(STD_CFLAGS) $(OPENSSL_CFLAGS)
+tfork_deadlock_LDADD = $(OPENSSL_LIBS)
+
 tmp.softokn:
 	LIBSPATH=$(libspath) \
 	TESTSSRCDIR=$(testssrcdir) \
@@ -66,7 +71,7 @@ tmp.softhsm:
 dist_check_SCRIPTS = \
 	helpers.sh setup-softhsm.sh setup-softokn.sh softhsm-proxy.sh \
 	test-wrapper tbasic tcerts tecc tecdh tedwards tdemoca thkdf \
-	toaepsha2 trsapss tdigest ttls tpubkey tfork turi trand tecxc \
+	toaepsha2 trsapss tdigest ttls tpubkey tforking turi trand tecxc \
 	tcms top_state tpem_encoder
 
 test_LIST = \
@@ -78,7 +83,7 @@ test_LIST = \
 	ecdh-softokn.t \
 	democa-softokn.t democa-softhsm.t \
 	digest-softokn.t digest-softhsm.t \
-	fork-softokn.t fork-softhsm.t \
+	forking-softokn.t forking-softhsm.t \
 	oaepsha2-softokn.t \
 	hkdf-softokn.t \
 	rsapss-softokn.t \

tests/tfork_deadlock.c

@@ -0,0 +1,232 @@
+/* Copyright (C) 2022 Simo Sorce <[email protected]>
+   SPDX-License-Identifier: Apache-2.0 */
+
+#define _GNU_SOURCE
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <openssl/err.h>
+#include <openssl/evp.h>
+#include <openssl/store.h>
+#include <openssl/ui.h>
+#include <sys/wait.h>
+
+#define PRINTERR(...) \
+    do { \
+        fprintf(stderr, __VA_ARGS__); \
+        fflush(stderr); \
+    } while (0)
+
+#define PRINTERROSSL(...) \
+    do { \
+        fprintf(stderr, __VA_ARGS__); \
+        ERR_print_errors_fp(stderr); \
+        fflush(stderr); \
+    } while (0)
+
+static void sign_op(EVP_PKEY *key)
+{
+    size_t size = EVP_PKEY_get_size(key);
+    unsigned char sig[size];
+    const char *data = "Sign Me!";
+    EVP_MD_CTX *sign_md;
+    int ret;
+
+    sign_md = EVP_MD_CTX_new();
+    ret = EVP_DigestSignInit_ex(sign_md, NULL, "SHA256", NULL, NULL, key, NULL);
+    if (ret != 1) {
+        PRINTERROSSL("Failed to init EVP_DigestSign\n");
+        exit(EXIT_FAILURE);
+    }
+
+    ret = EVP_DigestSignUpdate(sign_md, data, sizeof(data));
+    if (ret != 1) {
+        PRINTERROSSL("Failed to EVP_DigestSignUpdate\n");
+        exit(EXIT_FAILURE);
+    }
+
+    ret = EVP_DigestSignFinal(sign_md, sig, &size);
+    if (ret != 1) {
+        PRINTERROSSL("Failed to EVP_DigestSignFinal-ize\n");
+        exit(EXIT_FAILURE);
+    }
+    EVP_MD_CTX_free(sign_md);
+}
+
+static int ui_fork_in_callback(UI *ui, UI_STRING *uis)
+{
+    const char *pinvalue;
+    enum UI_string_types type;
+    pid_t pid;
+    int status;
+
+    pinvalue = getenv("PINVALUE");
+    if (!pinvalue) {
+        fprintf(stderr, "PINVALUE not defined\n");
+        exit(EXIT_FAILURE);
+    }
+
+    /* now fork while in the callback to check if we deadlock */
+    pid = fork();
+    if (pid == -1) {
+        PRINTERR("Fork failed\n");
+        exit(EXIT_FAILURE);
+    }
+
+    if (pid == 0) {
+        PRINTERR("Child Done");
+        exit(EXIT_SUCCESS);
+    }
+
+    waitpid(pid, &status, 0);
+    if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+        PRINTERR("Child failure\n");
+        exit(EXIT_FAILURE);
+    }
+
+    type = UI_get_string_type(uis);
+    switch (type) {
+    case UIT_PROMPT:
+        fprintf(stderr, "Prompt: \"%s\"\n", UI_get0_output_string(uis));
+        fprintf(stderr, "Returning: %s\n", pinvalue);
+        UI_set_result(ui, uis, pinvalue);
+        return 1;
+    default:
+        fprintf(stderr, "Unexpected UI type: %d\n", (int)type);
+        exit(EXIT_FAILURE);
+    }
+
+    return 0;
+}
+
+#ifdef __APPLE__
+/* No sigtimedwait on MacOS, so let's just fail this test */
+static int sigtimedwait(const sigset_t *set, siginfo_t *info,
+                        const struct timespec *timeout)
+{
+    errno = EPERM;
+    return -1;
+}
+#endif
+
+int main(int argc, char *argv[])
+{
+    const char *keyuri = NULL;
+    UI_METHOD *ui_method = NULL;
+    OSSL_STORE_CTX *store;
+    OSSL_STORE_INFO *info;
+    EVP_PKEY *key = NULL;
+    sigset_t mask;
+    sigset_t orig_mask;
+    struct timespec timeout;
+    pid_t pid;
+    int status;
+
+    keyuri = getenv("PRIURI");
+    /* optional first argument is a PKCS#11 uri of the key to test.
+     * Default is provided by environment variable BASEURI */
+    if (argc > 1) {
+        keyuri = argv[1];
+    }
+    if (!keyuri) {
+        fprintf(stderr, "PRIURI not defined\n");
+        exit(EXIT_FAILURE);
+    }
+
+    ui_method = UI_create_method("Pin Fork Test");
+    if (!ui_method) {
+        fprintf(stderr, "Failed to set up UI_METHOD\n");
+        exit(EXIT_FAILURE);
+    }
+    (void)UI_method_set_reader(ui_method, ui_fork_in_callback);
+
+    /* prepare timeout handler */
+    sigemptyset(&mask);
+    sigaddset(&mask, SIGCHLD);
+
+    status = sigprocmask(SIG_BLOCK, &mask, &orig_mask);
+    if (status == -1) {
+        PRINTERR("Sigprocmask failed\n");
+        exit(EXIT_FAILURE);
+    }
+
+    /* now fork so we can monitor the "parent" process */
+    pid = fork();
+    if (pid == -1) {
+        PRINTERR("Fork failed\n");
+        exit(EXIT_FAILURE);
+    }
+
+    /* controller parent */
+    if (pid != 0) {
+        /* Wait  a max of 5 seconds. Should be enough even on slow
+         * containers */
+        timeout.tv_sec = 5;
+        timeout.tv_nsec = 0;
+
+    retry:
+        status = sigtimedwait(&mask, NULL, &timeout);
+        if (status == -1) {
+            if (errno == EINTR) {
+                /* not the right signal */
+                goto retry;
+            }
+            if (errno == EAGAIN) {
+                PRINTERR("Timeout, Child likely deadlocked!\n");
+                kill(pid, SIGKILL);
+                exit(EXIT_FAILURE);
+            }
+            PRINTERR("Unknown Error: %d", errno);
+            kill(pid, SIGKILL);
+            exit(EXIT_FAILURE);
+        }
+
+        waitpid(pid, &status, 0);
+        if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
+            PRINTERR("Parent's failure\n");
+            exit(EXIT_FAILURE);
+        }
+
+        PRINTERR("ALL A-OK!\n");
+        UI_destroy_method(ui_method);
+        exit(EXIT_SUCCESS);
+    }
+
+    /* regular "parent" process (child of controlling parent) */
+
+    store = OSSL_STORE_open_ex(keyuri, NULL, "provider=pkcs11", ui_method, NULL,
+                               NULL, NULL, NULL);
+    if (store == NULL) {
+        fprintf(stderr, "Failed to open pkcs11 store\n");
+        exit(EXIT_FAILURE);
+    }
+
+    for (info = OSSL_STORE_load(store); info != NULL;
+         info = OSSL_STORE_load(store)) {
+        int type = OSSL_STORE_INFO_get_type(info);
+
+        switch (type) {
+        case OSSL_STORE_INFO_PKEY:
+            key = OSSL_STORE_INFO_get1_PKEY(info);
+            break;
+        default:
+            fprintf(stderr, "Invalid data from store\n");
+            exit(EXIT_FAILURE);
+        }
+        OSSL_STORE_INFO_free(info);
+    }
+
+    OSSL_STORE_close(store);
+
+    if (!key) {
+        fprintf(stderr, "Failed to find key\n");
+        exit(EXIT_FAILURE);
+    }
+
+    sign_op(key);
+    EVP_PKEY_free(key);
+    UI_destroy_method(ui_method);
+    PRINTERR("Parent Done");
+    exit(EXIT_SUCCESS);
+}

tests/tforking

@@ -0,0 +1,21 @@
+#!/bin/bash -e
+# Copyright (C) 2024 Simo Sorce <[email protected]>
+# SPDX-License-Identifier: Apache-2.0
+
+source "${TESTSSRCDIR}/helpers.sh"
+
+title PARA "Regular forking test"
+$CHECKER "${TESTBLDDIR}/tfork"
+
+# MacOS can't compile the fork_deadlock.c test because it lacks
+# some POSIX functions ... so we completely disable the test
+if [ "$(uname)" == "Darwin" ]; then
+    exit 0
+fi
+
+title PARA "Pinless config file to cause prompting callback in fork deadlock test"
+ORIG_OPENSSL_CONF=${OPENSSL_CONF}
+sed "s/^pkcs11-module-token-pin.*$/##nopin/" "${OPENSSL_CONF}" > "${OPENSSL_CONF}.nopin"
+OPENSSL_CONF=${OPENSSL_CONF}.nopin
+$CHECKER "${TESTBLDDIR}/tfork_deadlock"
+OPENSSL_CONF=${ORIG_OPENSSL_CONF}