From 55788a90967e82a9ea05b45c06a293b46ec53d72 Mon Sep 17 00:00:00 2001 From: Peter Bierma Date: Sun, 10 Aug 2025 10:47:11 -0400 Subject: [PATCH] gh-137583: Only lock the SSL context, not the SSL socket (GH-137588) Fixes a deadlock in 3.13.6. --- Lib/test/test_ssl.py | 36 ++++++++++++++++++ ...-08-09-08-53-32.gh-issue-137583.s6OZud.rst | 4 ++ Modules/_ssl.c | 38 ++++++++++--------- Modules/_ssl/debughelpers.c | 1 - 4 files changed, 60 insertions(+), 19 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2025-08-09-08-53-32.gh-issue-137583.s6OZud.rst diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index b5263129baed..7d1eb5649303 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -4627,6 +4627,42 @@ class ThreadedTests(unittest.TestCase): with client_context.wrap_socket(socket.socket()) as s: s.connect((HOST, server.port)) + def test_thread_recv_while_main_thread_sends(self): + # GH-137583: Locking was added to calls to send() and recv() on SSL + # socket objects. This seemed fine at the surface level because those + # calls weren't re-entrant, but recv() calls would implicitly mimick + # holding a lock by blocking until it received data. This means that + # if a thread started to infinitely block until data was received, calls + # to send() would deadlock, because it would wait forever on the lock + # that the recv() call held. + data = b"1" * 1024 + event = threading.Event() + def background(sock): + event.set() + received = sock.recv(len(data)) + self.assertEqual(received, data) + + client_context, server_context, hostname = testing_context() + server = ThreadedEchoServer(context=server_context) + with server: + with client_context.wrap_socket(socket.socket(), + server_hostname=hostname) as sock: + sock.connect((HOST, server.port)) + sock.settimeout(1) + sock.setblocking(1) + # Ensure that the server is ready to accept requests + sock.sendall(b"123") + self.assertEqual(sock.recv(3), b"123") + with threading_helper.catch_threading_exception() as cm: + thread = threading.Thread(target=background, + args=(sock,), daemon=True) + thread.start() + event.wait() + sock.sendall(data) + thread.join() + if cm.exc_value is not None: + raise cm.exc_value + @unittest.skipUnless(has_tls_version('TLSv1_3') and ssl.HAS_PHA, "Test needs TLS 1.3 PHA") diff --git a/Misc/NEWS.d/next/Library/2025-08-09-08-53-32.gh-issue-137583.s6OZud.rst b/Misc/NEWS.d/next/Library/2025-08-09-08-53-32.gh-issue-137583.s6OZud.rst new file mode 100644 index 000000000000..3843cc7c8c55 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2025-08-09-08-53-32.gh-issue-137583.s6OZud.rst @@ -0,0 +1,4 @@ +Fix a deadlock introduced in 3.13.6 when a call to +:meth:`ssl.SSLSocket.recv ` was blocked in one thread, +and then another method on the object (such as :meth:`ssl.SSLSocket.send `) +was subsequently called in another thread. diff --git a/Modules/_ssl.c b/Modules/_ssl.c index ab30258faf3f..a74654b7573f 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -366,9 +366,6 @@ typedef struct { * and shutdown methods check for chained exceptions. */ PyObject *exc; - /* Lock to synchronize calls when the thread state is detached. - See also gh-134698. */ - PyMutex tstate_mutex; } PySSLSocket; #define PySSLSocket_CAST(op) ((PySSLSocket *)(op)) @@ -918,7 +915,6 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock, self->server_hostname = NULL; self->err = err; self->exc = NULL; - self->tstate_mutex = (PyMutex){0}; /* Make sure the SSL error state is initialized */ ERR_clear_error(); @@ -994,12 +990,12 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock, BIO_set_nbio(SSL_get_wbio(self->ssl), 1); } - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS; if (socket_type == PY_SSL_CLIENT) SSL_set_connect_state(self->ssl); else SSL_set_accept_state(self->ssl); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; self->socket_type = socket_type; if (sock != NULL) { @@ -1068,10 +1064,11 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self) /* Actually negotiate SSL connection */ /* XXX If SSL_do_handshake() returns 0, it's also a failure. */ do { - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS ret = SSL_do_handshake(self->ssl); err = _PySSL_errno(ret < 1, self->ssl, ret); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; if (PyErr_CheckSignals()) @@ -2615,10 +2612,11 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset, } do { - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS retval = SSL_sendfile(self->ssl, fd, (off_t)offset, size, flags); err = _PySSL_errno(retval < 0, self->ssl, (int)retval); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; if (PyErr_CheckSignals()) { @@ -2746,10 +2744,11 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b) } do { - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS; retval = SSL_write_ex(self->ssl, b->buf, (size_t)b->len, &count); err = _PySSL_errno(retval == 0, self->ssl, retval); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; if (PyErr_CheckSignals()) @@ -2807,10 +2806,11 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self) int count = 0; _PySSLError err; - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS; count = SSL_pending(self->ssl); err = _PySSL_errno(count < 0, self->ssl, count); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; if (count < 0) @@ -2901,10 +2901,11 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len, deadline = _PyDeadline_Init(timeout); do { - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS; retval = SSL_read_ex(self->ssl, mem, (size_t)len, &count); err = _PySSL_errno(retval == 0, self->ssl, retval); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; if (PyErr_CheckSignals()) @@ -3003,7 +3004,7 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self) } while (1) { - PySSL_BEGIN_ALLOW_THREADS(self) + Py_BEGIN_ALLOW_THREADS; /* Disable read-ahead so that unwrap can work correctly. * Otherwise OpenSSL might read in too much data, * eating clear text data that happens to be @@ -3016,7 +3017,8 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self) SSL_set_read_ahead(self->ssl, 0); ret = SSL_shutdown(self->ssl); err = _PySSL_errno(ret < 0, self->ssl, ret); - PySSL_END_ALLOW_THREADS(self) + Py_END_ALLOW_THREADS; + _PySSL_FIX_ERRNO; self->err = err; /* If err == 1, a secure shutdown with SSL_shutdown() is complete */ diff --git a/Modules/_ssl/debughelpers.c b/Modules/_ssl/debughelpers.c index 211fe15a439b..aee446d0ccb1 100644 --- a/Modules/_ssl/debughelpers.c +++ b/Modules/_ssl/debughelpers.c @@ -140,7 +140,6 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line) * critical debug helper. */ - assert(PyMutex_IsLocked(&ssl_obj->tstate_mutex)); Py_BEGIN_ALLOW_THREADS PyThread_acquire_lock(lock, 1); res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line); -- 2.47.2