+++ /dev/null
-commit b97226815030be274f13a114cf8ebf0063899c41
-Author: Michael Paquier <michael@paquier.xyz>
-Date: Mon Nov 27 09:40:50 2023 +0900
-
- Fix race condition with BIO methods initialization in libpq with threads
-
- The libpq code in charge of creating per-connection SSL objects was
- prone to a race condition when loading the custom BIO methods needed by
- my_SSL_set_fd(). As BIO methods are stored as a static variable, the
- initialization of a connection could fail because it could be possible
- to have one thread refer to my_bio_methods while it is being manipulated
- by a second concurrent thread.
-
- This error has been introduced by 8bb14cdd33de, that has removed
- ssl_config_mutex around the call of my_SSL_set_fd(), that itself sets
- the custom BIO methods used in libpq. Like previously, the BIO method
- initialization is now protected by the existing ssl_config_mutex, itself
- initialized earlier for WIN32.
-
- While on it, document that my_bio_methods is protected by
- ssl_config_mutex, as this can be easy to miss.
-
- Reported-by: Willi Mann
- Author: Willi Mann, Michael Paquier
- Discussion: https://postgr.es/m/e77abc4c-4d03-4058-a9d7-ef0035657e04@celonis.com
- Backpatch-through: 12
-
-diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
-index af59ff49f7..62f813df68 100644
---- a/src/interfaces/libpq/fe-secure-openssl.c
-+++ b/src/interfaces/libpq/fe-secure-openssl.c
-@@ -1805,6 +1805,7 @@ PQsslAttribute(PGconn *conn, const char *attribute_name)
- #define BIO_set_data(bio, data) (bio->ptr = data)
- #endif
-
-+/* protected by ssl_config_mutex */
- static BIO_METHOD *my_bio_methods;
-
- static int
-@@ -1870,6 +1871,15 @@ my_sock_write(BIO *h, const char *buf, int size)
- static BIO_METHOD *
- my_BIO_s_socket(void)
- {
-+ BIO_METHOD *res;
-+
-+#ifdef ENABLE_THREAD_SAFETY
-+ if (pthread_mutex_lock(&ssl_config_mutex))
-+ return NULL;
-+#endif
-+
-+ res = my_bio_methods;
-+
- if (!my_bio_methods)
- {
- BIO_METHOD *biom = (BIO_METHOD *) BIO_s_socket();
-@@ -1878,39 +1888,58 @@ 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, biom, sizeof(BIO_METHOD));
-+ res->bread = my_sock_read;
-+ res->bwrite = my_sock_write;
- #endif
- }
-- return my_bio_methods;
-+
-+ my_bio_methods = res;
-+
-+#ifdef ENABLE_THREAD_SAFETY
-+ pthread_mutex_unlock(&ssl_config_mutex);
-+#endif
-+
-+ return res;
-+
-+err:
-+#ifdef HAVE_BIO_METH_NEW
-+ if (res)
-+ BIO_meth_free(res);
-+#else
-+ if (res)
-+ free(res);
-+#endif
-+
-+#ifdef ENABLE_THREAD_SAFETY
-+ pthread_mutex_unlock(&ssl_config_mutex);
-+#endif
-+ return NULL;
- }
-
- /* This should exactly match OpenSSL's SSL_set_fd except for using my BIO */
-commit 5dd30bb54bfec14e5677aff3e306edec79c204ef
-Author: Tom Lane <tgl@sss.pgh.pa.us>
-Date: Tue Nov 28 12:34:03 2023 -0500
-
- Use BIO_{get,set}_app_data instead of BIO_{get,set}_data.
-
- We should have done it this way all along, but we accidentally got
- away with using the wrong BIO field up until OpenSSL 3.2. There,
- the library's BIO routines that we rely on use the "data" field
- for their own purposes, and our conflicting use causes assorted
- weird behaviors up to and including core dumps when SSL connections
- are attempted. Switch to using the approved field for the purpose,
- i.e. app_data.
-
- While at it, remove our configure probes for BIO_get_data as well
- as the fallback implementation. BIO_{get,set}_app_data have been
- there since long before any OpenSSL version that we still support,
- even in the back branches.
-
- Also, update src/test/ssl/t/001_ssltests.pl to allow for a minor
- change in an error message spelling that evidently came in with 3.2.
-
- Tristan Partin and Bo Andreson. Back-patch to all supported branches.
-
- Discussion: https://postgr.es/m/CAN55FZ1eDDYsYaL7mv+oSLUij2h_u6hvD4Qmv-7PK7jkji0uyQ@mail.gmail.com
-
-diff --git a/configure b/configure
-index d83a402ea1..d55440cd6a 100755
---- a/configure
-+++ b/configure
-@@ -13239,7 +13239,7 @@ done
- # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
- # doesn't have these OpenSSL 1.1.0 functions. So check for individual
- # functions.
-- for ac_func in OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free
-+ for ac_func in OPENSSL_init_ssl BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free
- do :
- as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
- ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
-diff --git a/configure.ac b/configure.ac
-index 570daced81..2bc752ca1a 100644
---- a/configure.ac
-+++ b/configure.ac
-@@ -1347,7 +1347,7 @@ if test "$with_ssl" = openssl ; then
- # defines OPENSSL_VERSION_NUMBER to claim version 2.0.0, even though it
- # doesn't have these OpenSSL 1.1.0 functions. So check for individual
- # functions.
-- AC_CHECK_FUNCS([OPENSSL_init_ssl BIO_get_data BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free])
-+ AC_CHECK_FUNCS([OPENSSL_init_ssl BIO_meth_new ASN1_STRING_get0_data HMAC_CTX_new HMAC_CTX_free])
- # OpenSSL versions before 1.1.0 required setting callback functions, for
- # thread-safety. In 1.1.0, it's no longer required, and CRYPTO_lock()
- # function was removed.
-diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
-index f5c5ed210e..aed8a75345 100644
---- a/src/backend/libpq/be-secure-openssl.c
-+++ b/src/backend/libpq/be-secure-openssl.c
-@@ -839,11 +839,6 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
- * to retry; do we need to adopt their logic for that?
- */
-
--#ifndef HAVE_BIO_GET_DATA
--#define BIO_get_data(bio) (bio->ptr)
--#define BIO_set_data(bio, data) (bio->ptr = data)
--#endif
--
- static BIO_METHOD *my_bio_methods = NULL;
-
- static int
-@@ -853,7 +848,7 @@ my_sock_read(BIO *h, char *buf, int size)
-
- if (buf != NULL)
- {
-- res = secure_raw_read(((Port *) BIO_get_data(h)), buf, size);
-+ res = secure_raw_read(((Port *) BIO_get_app_data(h)), buf, size);
- BIO_clear_retry_flags(h);
- if (res <= 0)
- {
-@@ -873,7 +868,7 @@ my_sock_write(BIO *h, const char *buf, int size)
- {
- int res = 0;
-
-- res = secure_raw_write(((Port *) BIO_get_data(h)), buf, size);
-+ res = secure_raw_write(((Port *) BIO_get_app_data(h)), buf, size);
- BIO_clear_retry_flags(h);
- if (res <= 0)
- {
-@@ -949,7 +944,7 @@ my_SSL_set_fd(Port *port, int fd)
- SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
- goto err;
- }
-- BIO_set_data(bio, port);
-+ BIO_set_app_data(bio, port);
-
- BIO_set_fd(bio, fd, BIO_NOCLOSE);
- SSL_set_bio(port->ssl, bio, bio);
-diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
-index d09e9f9a1c..768e3d719c 100644
---- a/src/include/pg_config.h.in
-+++ b/src/include/pg_config.h.in
-@@ -77,9 +77,6 @@
- /* Define to 1 if you have the `backtrace_symbols' function. */
- #undef HAVE_BACKTRACE_SYMBOLS
-
--/* Define to 1 if you have the `BIO_get_data' function. */
--#undef HAVE_BIO_GET_DATA
--
- /* Define to 1 if you have the `BIO_meth_new' function. */
- #undef HAVE_BIO_METH_NEW
-
-diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
-index 62f813df68..d863d279a0 100644
---- a/src/interfaces/libpq/fe-secure-openssl.c
-+++ b/src/interfaces/libpq/fe-secure-openssl.c
-@@ -1800,11 +1800,6 @@ PQsslAttribute(PGconn *conn, const char *attribute_name)
- * to retry; do we need to adopt their logic for that?
- */
-
--#ifndef HAVE_BIO_GET_DATA
--#define BIO_get_data(bio) (bio->ptr)
--#define BIO_set_data(bio, data) (bio->ptr = data)
--#endif
--
- /* protected by ssl_config_mutex */
- static BIO_METHOD *my_bio_methods;
-
-@@ -1813,7 +1808,7 @@ my_sock_read(BIO *h, char *buf, int size)
- {
- int res;
-
-- res = pqsecure_raw_read((PGconn *) BIO_get_data(h), buf, size);
-+ res = pqsecure_raw_read((PGconn *) BIO_get_app_data(h), buf, size);
- BIO_clear_retry_flags(h);
- if (res < 0)
- {
-@@ -1843,7 +1838,7 @@ my_sock_write(BIO *h, const char *buf, int size)
- {
- int res;
-
-- res = pqsecure_raw_write((PGconn *) BIO_get_data(h), buf, size);
-+ res = pqsecure_raw_write((PGconn *) BIO_get_app_data(h), buf, size);
- BIO_clear_retry_flags(h);
- if (res < 0)
- {
-@@ -1962,7 +1957,7 @@ my_SSL_set_fd(PGconn *conn, int fd)
- SSLerr(SSL_F_SSL_SET_FD, ERR_R_BUF_LIB);
- goto err;
- }
-- BIO_set_data(bio, conn);
-+ BIO_set_app_data(bio, conn);
-
- SSL_set_bio(conn->ssl, bio, bio);
- BIO_set_fd(bio, fd, BIO_NOCLOSE);
-diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
-index 707f4005af..c570b48a1b 100644
---- a/src/test/ssl/t/001_ssltests.pl
-+++ b/src/test/ssl/t/001_ssltests.pl
-@@ -682,7 +682,7 @@ $node->connect_fails(
- "$common_connstr user=ssltestuser sslcert=ssl/client-revoked.crt "
- . sslkey('client-revoked.key'),
- "certificate authorization fails with revoked client cert",
-- expected_stderr => qr/SSL error: sslv3 alert certificate revoked/,
-+ expected_stderr => qr|SSL error: ssl[a-z0-9/]* alert certificate revoked|,
- # revoked certificates should not authenticate the user
- log_unlike => [qr/connection authenticated:/],);
-
-@@ -743,6 +743,6 @@ $node->connect_fails(
- "$common_connstr user=ssltestuser sslcert=ssl/client-revoked.crt "
- . sslkey('client-revoked.key'),
- "certificate authorization fails with revoked client cert with server-side CRL directory",
-- expected_stderr => qr/SSL error: sslv3 alert certificate revoked/);
-+ expected_stderr => qr|SSL error: ssl[a-z0-9/]* alert certificate revoked|);
-
- done_testing();
-diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
-index 790f03b05e..a53239fa28 100644
---- a/src/tools/msvc/Solution.pm
-+++ b/src/tools/msvc/Solution.pm
-@@ -226,7 +226,6 @@ sub GenerateFiles
- HAVE_ATOMICS => 1,
- HAVE_ATOMIC_H => undef,
- HAVE_BACKTRACE_SYMBOLS => undef,
-- HAVE_BIO_GET_DATA => undef,
- HAVE_BIO_METH_NEW => undef,
- HAVE_CLOCK_GETTIME => undef,
- HAVE_COMPUTED_GOTO => undef,
-@@ -566,7 +565,6 @@ sub GenerateFiles
- || ($digit1 >= '1' && $digit2 >= '1' && $digit3 >= '0'))
- {
- $define{HAVE_ASN1_STRING_GET0_DATA} = 1;
-- $define{HAVE_BIO_GET_DATA} = 1;
- $define{HAVE_BIO_METH_NEW} = 1;
- $define{HAVE_HMAC_CTX_FREE} = 1;
- $define{HAVE_HMAC_CTX_NEW} = 1;
-commit 551d4b28e4453fce5890c0df7121bae44b417ec2
-Author: Tom Lane <tgl@sss.pgh.pa.us>
-Date: Mon Dec 11 11:51:56 2023 -0500
-
- Be more wary about OpenSSL not setting errno on error.
-
- OpenSSL will sometimes return SSL_ERROR_SYSCALL without having set
- errno; this is apparently a reflection of recv(2)'s habit of not
- setting errno when reporting EOF. Ensure that we treat such cases
- the same as read EOF. Previously, we'd frequently report them like
- "could not accept SSL connection: Success" which is confusing, or
- worse report them with an unrelated errno left over from some
- previous syscall.
-
- To fix, ensure that errno is zeroed immediately before the call,
- and report its value only when it's not zero afterwards; otherwise
- report EOF.
-
- For consistency, I've applied the same coding pattern in libpq's
- pqsecure_raw_read(). Bare recv(2) shouldn't really return -1 without
- setting errno, but in case it does we might as well cope.
-
- Per report from Andres Freund. Back-patch to all supported versions.
-
- Discussion: https://postgr.es/m/20231208181451.deqnflwxqoehhxpe@awork3.anarazel.de
-
-diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
-index aed8a75345..ed13e8b06d 100644
---- a/src/backend/libpq/be-secure-openssl.c
-+++ b/src/backend/libpq/be-secure-openssl.c
-@@ -457,6 +457,7 @@ aloop:
- * per-thread error queue following another call to an OpenSSL I/O
- * routine.
- */
-+ errno = 0;
- ERR_clear_error();
- r = SSL_accept(port->ssl);
- if (r <= 0)
-@@ -493,7 +494,7 @@ aloop:
- WAIT_EVENT_SSL_OPEN_SERVER);
- goto aloop;
- case SSL_ERROR_SYSCALL:
-- if (r < 0)
-+ if (r < 0 && errno != 0)
- ereport(COMMERROR,
- (errcode_for_socket_access(),
- errmsg("could not accept SSL connection: %m")));
-@@ -727,7 +728,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
- break;
- case SSL_ERROR_SYSCALL:
- /* leave it to caller to ereport the value of errno */
-- if (n != -1)
-+ if (n != -1 || errno == 0)
- {
- errno = ECONNRESET;
- n = -1;
-@@ -785,8 +786,14 @@ be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
- n = -1;
- break;
- case SSL_ERROR_SYSCALL:
-- /* leave it to caller to ereport the value of errno */
-- if (n != -1)
-+
-+ /*
-+ * Leave it to caller to ereport the value of errno. However, if
-+ * errno is still zero then assume it's a read EOF situation, and
-+ * report ECONNRESET. (This seems possible because SSL_write can
-+ * also do reads.)
-+ */
-+ if (n != -1 || errno == 0)
- {
- errno = ECONNRESET;
- n = -1;
-diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
-index 75392a8bb7..bb9fa77cd4 100644
---- a/src/backend/libpq/pqcomm.c
-+++ b/src/backend/libpq/pqcomm.c
-@@ -954,6 +954,8 @@ pq_recvbuf(void)
- {
- int r;
-
-+ errno = 0;
-+
- r = secure_read(MyProcPort, PqRecvBuffer + PqRecvLength,
- PQ_RECV_BUFFER_SIZE - PqRecvLength);
-
-@@ -966,10 +968,13 @@ pq_recvbuf(void)
- * Careful: an ereport() that tries to write to the client would
- * cause recursion to here, leading to stack overflow and core
- * dump! This message must go *only* to the postmaster log.
-+ *
-+ * If errno is zero, assume it's EOF and let the caller complain.
- */
-- ereport(COMMERROR,
-- (errcode_for_socket_access(),
-- errmsg("could not receive data from client: %m")));
-+ if (errno != 0)
-+ ereport(COMMERROR,
-+ (errcode_for_socket_access(),
-+ errmsg("could not receive data from client: %m")));
- return EOF;
- }
- if (r == 0)
-@@ -1046,6 +1051,8 @@ pq_getbyte_if_available(unsigned char *c)
- /* Put the socket into non-blocking mode */
- socket_set_nonblocking(true);
-
-+ errno = 0;
-+
- r = secure_read(MyProcPort, c, 1);
- if (r < 0)
- {
-@@ -1062,10 +1069,13 @@ pq_getbyte_if_available(unsigned char *c)
- * Careful: an ereport() that tries to write to the client would
- * cause recursion to here, leading to stack overflow and core
- * dump! This message must go *only* to the postmaster log.
-+ *
-+ * If errno is zero, assume it's EOF and let the caller complain.
- */
-- ereport(COMMERROR,
-- (errcode_for_socket_access(),
-- errmsg("could not receive data from client: %m")));
-+ if (errno != 0)
-+ ereport(COMMERROR,
-+ (errcode_for_socket_access(),
-+ errmsg("could not receive data from client: %m")));
- r = EOF;
- }
- }
-diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
-index d863d279a0..61f37678a6 100644
---- a/src/interfaces/libpq/fe-secure-openssl.c
-+++ b/src/interfaces/libpq/fe-secure-openssl.c
-@@ -209,7 +209,7 @@ rloop:
- */
- goto rloop;
- case SSL_ERROR_SYSCALL:
-- if (n < 0)
-+ if (n < 0 && SOCK_ERRNO != 0)
- {
- result_errno = SOCK_ERRNO;
- if (result_errno == EPIPE ||
-@@ -317,7 +317,13 @@ pgtls_write(PGconn *conn, const void *ptr, size_t len)
- n = 0;
- break;
- case SSL_ERROR_SYSCALL:
-- if (n < 0)
-+
-+ /*
-+ * If errno is still zero then assume it's a read EOF situation,
-+ * and report EOF. (This seems possible because SSL_write can
-+ * also do reads.)
-+ */
-+ if (n < 0 && SOCK_ERRNO != 0)
- {
- result_errno = SOCK_ERRNO;
- if (result_errno == EPIPE || result_errno == ECONNRESET)
-@@ -1479,10 +1485,12 @@ open_client_SSL(PGconn *conn)
- {
- int r;
-
-+ SOCK_ERRNO_SET(0);
- ERR_clear_error();
- r = SSL_connect(conn->ssl);
- if (r <= 0)
- {
-+ int save_errno = SOCK_ERRNO;
- int err = SSL_get_error(conn->ssl, r);
- unsigned long ecode;
-
-@@ -1499,10 +1507,10 @@ open_client_SSL(PGconn *conn)
- {
- char sebuf[PG_STRERROR_R_BUFLEN];
-
-- if (r == -1)
-+ if (r == -1 && save_errno != 0)
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("SSL SYSCALL error: %s\n"),
-- SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
-+ SOCK_STRERROR(save_errno, sebuf, sizeof(sebuf)));
- else
- appendPQExpBufferStr(&conn->errorMessage,
- libpq_gettext("SSL SYSCALL error: EOF detected\n"));
-diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
-index a1dc7b796d..4c85b7299c 100644
---- a/src/interfaces/libpq/fe-secure.c
-+++ b/src/interfaces/libpq/fe-secure.c
-@@ -235,6 +235,8 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
- int result_errno = 0;
- char sebuf[PG_STRERROR_R_BUFLEN];
-
-+ SOCK_ERRNO_SET(0);
-+
- n = recv(conn->sock, ptr, len, 0);
-
- if (n < 0)
-@@ -262,6 +264,11 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
- "\tbefore or while processing the request.\n"));
- break;
-
-+ case 0:
-+ /* If errno didn't get set, treat it as regular EOF */
-+ n = 0;
-+ break;
-+
- default:
- appendPQExpBuffer(&conn->errorMessage,
- libpq_gettext("could not receive data from server: %s\n"),
projects / packages / postgresql.git / commitdiff