Skip to content

Commit da39cb9

Browse files
[3.13] gh-137583: Only lock the SSL context, not the SSL socket (GH-137588) (GH-137613)
Fixes a deadlock introduced in 3.13.6. (cherry picked from commit 55788a9)
1 parent c1e1c88 commit da39cb9

File tree

4 files changed

+57
-17
lines changed

4 files changed

+57
-17
lines changed

Lib/test/test_ssl.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4584,6 +4584,42 @@ def server_callback(identity):
45844584
with client_context.wrap_socket(socket.socket()) as s:
45854585
s.connect((HOST, server.port))
45864586

4587+
def test_thread_recv_while_main_thread_sends(self):
4588+
# GH-137583: Locking was added to calls to send() and recv() on SSL
4589+
# socket objects. This seemed fine at the surface level because those
4590+
# calls weren't re-entrant, but recv() calls would implicitly mimick
4591+
# holding a lock by blocking until it received data. This means that
4592+
# if a thread started to infinitely block until data was received, calls
4593+
# to send() would deadlock, because it would wait forever on the lock
4594+
# that the recv() call held.
4595+
data = b"1" * 1024
4596+
event = threading.Event()
4597+
def background(sock):
4598+
event.set()
4599+
received = sock.recv(len(data))
4600+
self.assertEqual(received, data)
4601+
4602+
client_context, server_context, hostname = testing_context()
4603+
server = ThreadedEchoServer(context=server_context)
4604+
with server:
4605+
with client_context.wrap_socket(socket.socket(),
4606+
server_hostname=hostname) as sock:
4607+
sock.connect((HOST, server.port))
4608+
sock.settimeout(1)
4609+
sock.setblocking(1)
4610+
# Ensure that the server is ready to accept requests
4611+
sock.sendall(b"123")
4612+
self.assertEqual(sock.recv(3), b"123")
4613+
with threading_helper.catch_threading_exception() as cm:
4614+
thread = threading.Thread(target=background,
4615+
args=(sock,), daemon=True)
4616+
thread.start()
4617+
event.wait()
4618+
sock.sendall(data)
4619+
thread.join()
4620+
if cm.exc_value is not None:
4621+
raise cm.exc_value
4622+
45874623

45884624
@unittest.skipUnless(has_tls_version('TLSv1_3'), "Test needs TLS 1.3")
45894625
class TestPostHandshakeAuth(unittest.TestCase):
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
Fix a deadlock introduced in 3.13.6 when a call to
2+
:meth:`ssl.SSLSocket.recv <socket.socket.recv>` was blocked in one thread,
3+
and then another method on the object (such as :meth:`ssl.SSLSocket.send <socket.socket.send>`)
4+
was subsequently called in another thread.

Modules/_ssl.c

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -332,9 +332,6 @@ typedef struct {
332332
* and shutdown methods check for chained exceptions.
333333
*/
334334
PyObject *exc;
335-
/* Lock to synchronize calls when the thread state is detached.
336-
See also gh-134698. */
337-
PyMutex tstate_mutex;
338335
} PySSLSocket;
339336

340337
typedef struct {
@@ -846,7 +843,6 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
846843
self->server_hostname = NULL;
847844
self->err = err;
848845
self->exc = NULL;
849-
self->tstate_mutex = (PyMutex){0};
850846

851847
/* Make sure the SSL error state is initialized */
852848
ERR_clear_error();
@@ -919,12 +915,12 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
919915
BIO_set_nbio(SSL_get_wbio(self->ssl), 1);
920916
}
921917

922-
PySSL_BEGIN_ALLOW_THREADS(self)
918+
Py_BEGIN_ALLOW_THREADS;
923919
if (socket_type == PY_SSL_CLIENT)
924920
SSL_set_connect_state(self->ssl);
925921
else
926922
SSL_set_accept_state(self->ssl);
927-
PySSL_END_ALLOW_THREADS(self)
923+
Py_END_ALLOW_THREADS;
928924

929925
self->socket_type = socket_type;
930926
if (sock != NULL) {
@@ -993,10 +989,11 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
993989
/* Actually negotiate SSL connection */
994990
/* XXX If SSL_do_handshake() returns 0, it's also a failure. */
995991
do {
996-
PySSL_BEGIN_ALLOW_THREADS(self)
992+
Py_BEGIN_ALLOW_THREADS
997993
ret = SSL_do_handshake(self->ssl);
998994
err = _PySSL_errno(ret < 1, self->ssl, ret);
999-
PySSL_END_ALLOW_THREADS(self)
995+
Py_END_ALLOW_THREADS;
996+
_PySSL_FIX_ERRNO;
1000997
self->err = err;
1001998

1002999
if (PyErr_CheckSignals())
@@ -2462,10 +2459,11 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
24622459
}
24632460

24642461
do {
2465-
PySSL_BEGIN_ALLOW_THREADS(self)
2462+
Py_BEGIN_ALLOW_THREADS;
24662463
retval = SSL_write_ex(self->ssl, b->buf, (size_t)b->len, &count);
24672464
err = _PySSL_errno(retval == 0, self->ssl, retval);
2468-
PySSL_END_ALLOW_THREADS(self)
2465+
Py_END_ALLOW_THREADS;
2466+
_PySSL_FIX_ERRNO;
24692467
self->err = err;
24702468

24712469
if (PyErr_CheckSignals())
@@ -2523,10 +2521,11 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self)
25232521
int count = 0;
25242522
_PySSLError err;
25252523

2526-
PySSL_BEGIN_ALLOW_THREADS(self)
2524+
Py_BEGIN_ALLOW_THREADS;
25272525
count = SSL_pending(self->ssl);
25282526
err = _PySSL_errno(count < 0, self->ssl, count);
2529-
PySSL_END_ALLOW_THREADS(self)
2527+
Py_END_ALLOW_THREADS;
2528+
_PySSL_FIX_ERRNO;
25302529
self->err = err;
25312530

25322531
if (count < 0)
@@ -2617,10 +2616,11 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
26172616
deadline = _PyDeadline_Init(timeout);
26182617

26192618
do {
2620-
PySSL_BEGIN_ALLOW_THREADS(self)
2619+
Py_BEGIN_ALLOW_THREADS;
26212620
retval = SSL_read_ex(self->ssl, mem, (size_t)len, &count);
26222621
err = _PySSL_errno(retval == 0, self->ssl, retval);
2623-
PySSL_END_ALLOW_THREADS(self)
2622+
Py_END_ALLOW_THREADS;
2623+
_PySSL_FIX_ERRNO;
26242624
self->err = err;
26252625

26262626
if (PyErr_CheckSignals())
@@ -2719,7 +2719,7 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
27192719
}
27202720

27212721
while (1) {
2722-
PySSL_BEGIN_ALLOW_THREADS(self)
2722+
Py_BEGIN_ALLOW_THREADS;
27232723
/* Disable read-ahead so that unwrap can work correctly.
27242724
* Otherwise OpenSSL might read in too much data,
27252725
* eating clear text data that happens to be
@@ -2732,7 +2732,8 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
27322732
SSL_set_read_ahead(self->ssl, 0);
27332733
ret = SSL_shutdown(self->ssl);
27342734
err = _PySSL_errno(ret < 0, self->ssl, ret);
2735-
PySSL_END_ALLOW_THREADS(self)
2735+
Py_END_ALLOW_THREADS;
2736+
_PySSL_FIX_ERRNO;
27362737
self->err = err;
27372738

27382739
/* If err == 1, a secure shutdown with SSL_shutdown() is complete */

Modules/_ssl/debughelpers.c

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,6 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line)
135135
* critical debug helper.
136136
*/
137137

138-
assert(PyMutex_IsLocked(&ssl_obj->tstate_mutex));
139138
Py_BEGIN_ALLOW_THREADS
140139
PyThread_acquire_lock(lock, 1);
141140
res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);

0 commit comments

Comments
 (0)