On Wed, Nov 22, 2023 at 10:43:32AM +0900, Michael Paquier wrote: > I am not really convinced that we require a second mutex here, as > there is always a concern with inter-locking changes. I may be > missing something, of course, but BIO_s_socket() is just a pointer to > a set of callbacks hidden in bss_sock.c with BIO_meth_new() and > BIO_get_new_index() assigning some centralized data to handle the > methods in a controlled way in OpenSSL.
I was looking at the origin of this one, and this is an issue coming down to 8bb14cdd33de that has removed the ssl_config_mutex taken in pgtls_open_client() when we called my_SSL_set_fd(). The commit has accidentally missed that path with the static BIO method where the mutex mattered. > We only case about > initializing once for the sake of libpq's threads, so wouldn't it be > better to move my_BIO_s_socket() in pgtls_init() where the > initialization of the BIO methods would be protected by > ssl_config_mutex? That's basically what Willi means in his first > message, no? I've looked at this idea, and finished by being unhappy with the error handling that we are currently assuming in my_SSL_set_fd() in the event of an error in the bio method setup, which would be most likely an OOM, so let's use ssl_config_mutex in my_BIO_s_socket(). Another thing is that I have minimized the manipulation of my_bio_methods in the setup routine. I've been also testing the risks of collusion, and it takes me quite a a few tries with hundred threads to reproduce the failure even without any forced sleep, so that seems really hard to reach. Please find attached a patch (still need to do more checks with older versions of OpenSSL). Any thoughts or comments? -- Michael
From dc1bc40271311868ec4840be2c246efa705dc7a5 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Fri, 24 Nov 2023 16:47:03 +0900
Subject: [PATCH v2] Fix race condition in initialization of my_bio_methods
static variable
---
src/interfaces/libpq/fe-secure-openssl.c | 61 ++++++++++++++++--------
1 file changed, 40 insertions(+), 21 deletions(-)
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index f1192d28f2..30a83fbd01 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -1885,6 +1885,13 @@ my_sock_write(BIO *h, const char *buf, int size)
static BIO_METHOD *
my_BIO_s_socket(void)
{
+ BIO_METHOD *res;
+
+ if (pthread_mutex_lock(&ssl_config_mutex))
+ return NULL;
+
+ res = my_bio_methods;
+
if (!my_bio_methods)
{
BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
@@ -1893,39 +1900,51 @@ my_BIO_s_socket(void)
my_bio_index = BIO_get_new_index();
if (my_bio_index == -1)
- return NULL;
+ goto err;
my_bio_index |= (BIO_TYPE_DESCRIPTOR | BIO_TYPE_SOURCE_SINK);
- my_bio_methods = BIO_meth_new(my_bio_index, "libpq socket");
- if (!my_bio_methods)
- return NULL;
+ res = BIO_meth_new(my_bio_index, "libpq socket");
+ if (!res)
+ goto err;
/*
* As of this writing, these functions never fail. But check anyway,
* like OpenSSL's own examples do.
*/
- if (!BIO_meth_set_write(my_bio_methods, my_sock_write) ||
- !BIO_meth_set_read(my_bio_methods, my_sock_read) ||
- !BIO_meth_set_gets(my_bio_methods, BIO_meth_get_gets(biom)) ||
- !BIO_meth_set_puts(my_bio_methods, BIO_meth_get_puts(biom)) ||
- !BIO_meth_set_ctrl(my_bio_methods, BIO_meth_get_ctrl(biom)) ||
- !BIO_meth_set_create(my_bio_methods, BIO_meth_get_create(biom)) ||
- !BIO_meth_set_destroy(my_bio_methods, BIO_meth_get_destroy(biom)) ||
- !BIO_meth_set_callback_ctrl(my_bio_methods, BIO_meth_get_callback_ctrl(biom)))
+ if (!BIO_meth_set_write(res, my_sock_write) ||
+ !BIO_meth_set_read(res, my_sock_read) ||
+ !BIO_meth_set_gets(res, BIO_meth_get_gets(biom)) ||
+ !BIO_meth_set_puts(res, BIO_meth_get_puts(biom)) ||
+ !BIO_meth_set_ctrl(res, BIO_meth_get_ctrl(biom)) ||
+ !BIO_meth_set_create(res, BIO_meth_get_create(biom)) ||
+ !BIO_meth_set_destroy(res, BIO_meth_get_destroy(biom)) ||
+ !BIO_meth_set_callback_ctrl(res, BIO_meth_get_callback_ctrl(biom)))
{
- BIO_meth_free(my_bio_methods);
- my_bio_methods = NULL;
- return NULL;
+ goto err;
}
#else
- my_bio_methods = malloc(sizeof(BIO_METHOD));
- if (!my_bio_methods)
- return NULL;
- memcpy(my_bio_methods, biom, sizeof(BIO_METHOD));
- my_bio_methods->bread = my_sock_read;
- my_bio_methods->bwrite = my_sock_write;
+ res = malloc(sizeof(BIO_METHOD));
+ if (!res)
+ goto err;
+ memcpy(res, res, sizeof(BIO_METHOD));
+ res->bread = my_sock_read;
+ res->bwrite = my_sock_write;
#endif
}
+
+ my_bio_methods = res;
+ pthread_mutex_unlock(&ssl_config_mutex);
return my_bio_methods;
+
+err:
+#ifdef HAVE_BIO_METH_NEW
+ if (res)
+ BIO_meth_free(res);
+#else
+ if (res)
+ free(res);
+#endif
+ pthread_mutex_unlock(&ssl_config_mutex);
+ return NULL;
}
/* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */
--
2.42.0
signature.asc
Description: PGP signature
