]> git.ipfire.org Git - thirdparty/Python/cpython.git/commitdiff
gh-137583: Only lock the SSL context, not the SSL socket (GH-137588) main
authorPeter Bierma <zintensitydev@gmail.com>
Sun, 10 Aug 2025 14:47:11 +0000 (10:47 -0400)
committerGitHub <noreply@github.com>
Sun, 10 Aug 2025 14:47:11 +0000 (14:47 +0000)
Fixes a deadlock in 3.13.6.

Lib/test/test_ssl.py
Misc/NEWS.d/next/Library/2025-08-09-08-53-32.gh-issue-137583.s6OZud.rst [new file with mode: 0644]
Modules/_ssl.c
Modules/_ssl/debughelpers.c

index b5263129baed3f9b23762d8a63f2d24a35ae641a..7d1eb564930367b93594ce882aab007deb3e508f 100644 (file)
@@ -4627,6 +4627,42 @@ class ThreadedTests(unittest.TestCase):
             with client_context.wrap_socket(socket.socket()) as s:
                 s.connect((HOST, server.port))
 
             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")
 
 @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 (file)
index 0000000..3843cc7
--- /dev/null
@@ -0,0 +1,4 @@
+Fix a deadlock introduced in 3.13.6 when a call to
+:meth:`ssl.SSLSocket.recv <socket.socket.recv>` was blocked in one thread,
+and then another method on the object (such as :meth:`ssl.SSLSocket.send <socket.socket.send>`)
+was subsequently called in another thread.
index ab30258faf3f627aa65a6d298ec3b6445c284aa5..a74654b7573f4584c0b4f6927dd507acbbc45100 100644 (file)
@@ -366,9 +366,6 @@ typedef struct {
      * and shutdown methods check for chained exceptions.
      */
     PyObject *exc;
      * 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))
 } 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->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();
 
     /* 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);
     }
 
         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);
     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) {
 
     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 {
     /* 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);
         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())
         self->err = err;
 
         if (PyErr_CheckSignals())
@@ -2615,10 +2612,11 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset,
     }
 
     do {
     }
 
     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);
         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()) {
         self->err = err;
 
         if (PyErr_CheckSignals()) {
@@ -2746,10 +2744,11 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
     }
 
     do {
     }
 
     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);
         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())
         self->err = err;
 
         if (PyErr_CheckSignals())
@@ -2807,10 +2806,11 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self)
     int count = 0;
     _PySSLError err;
 
     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);
     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)
     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 {
         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);
         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())
         self->err = err;
 
         if (PyErr_CheckSignals())
@@ -3003,7 +3004,7 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
     }
 
     while (1) {
     }
 
     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
         /* 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);
             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 */
         self->err = err;
 
         /* If err == 1, a secure shutdown with SSL_shutdown() is complete */
index 211fe15a439bfc10766f0f9737883db9f6f6b6d3..aee446d0ccb1b816fad559699d97d2cd03402cc8 100644 (file)
@@ -140,7 +140,6 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line)
      * critical debug helper.
      */
 
      * 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);
     Py_BEGIN_ALLOW_THREADS
     PyThread_acquire_lock(lock, 1);
     res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);