Skip to content

Commit 55788a9

Browse files
gh-137583: Only lock the SSL context, not the SSL socket (GH-137588)
Fixes a deadlock in 3.13.6.
1 parent 046a4e3 commit 55788a9

File tree

4 files changed

+60
-19
lines changed

4 files changed

+60
-19
lines changed

Lib/test/test_ssl.py

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

4630+
def test_thread_recv_while_main_thread_sends(self):
4631+
# GH-137583: Locking was added to calls to send() and recv() on SSL
4632+
# socket objects. This seemed fine at the surface level because those
4633+
# calls weren't re-entrant, but recv() calls would implicitly mimick
4634+
# holding a lock by blocking until it received data. This means that
4635+
# if a thread started to infinitely block until data was received, calls
4636+
# to send() would deadlock, because it would wait forever on the lock
4637+
# that the recv() call held.
4638+
data = b"1" * 1024
4639+
event = threading.Event()
4640+
def background(sock):
4641+
event.set()
4642+
received = sock.recv(len(data))
4643+
self.assertEqual(received, data)
4644+
4645+
client_context, server_context, hostname = testing_context()
4646+
server = ThreadedEchoServer(context=server_context)
4647+
with server:
4648+
with client_context.wrap_socket(socket.socket(),
4649+
server_hostname=hostname) as sock:
4650+
sock.connect((HOST, server.port))
4651+
sock.settimeout(1)
4652+
sock.setblocking(1)
4653+
# Ensure that the server is ready to accept requests
4654+
sock.sendall(b"123")
4655+
self.assertEqual(sock.recv(3), b"123")
4656+
with threading_helper.catch_threading_exception() as cm:
4657+
thread = threading.Thread(target=background,
4658+
args=(sock,), daemon=True)
4659+
thread.start()
4660+
event.wait()
4661+
sock.sendall(data)
4662+
thread.join()
4663+
if cm.exc_value is not None:
4664+
raise cm.exc_value
4665+
46304666

46314667
@unittest.skipUnless(has_tls_version('TLSv1_3') and ssl.HAS_PHA,
46324668
"Test needs TLS 1.3 PHA")
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: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -366,9 +366,6 @@ typedef struct {
366366
* and shutdown methods check for chained exceptions.
367367
*/
368368
PyObject *exc;
369-
/* Lock to synchronize calls when the thread state is detached.
370-
See also gh-134698. */
371-
PyMutex tstate_mutex;
372369
} PySSLSocket;
373370

374371
#define PySSLSocket_CAST(op) ((PySSLSocket *)(op))
@@ -918,7 +915,6 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
918915
self->server_hostname = NULL;
919916
self->err = err;
920917
self->exc = NULL;
921-
self->tstate_mutex = (PyMutex){0};
922918

923919
/* Make sure the SSL error state is initialized */
924920
ERR_clear_error();
@@ -994,12 +990,12 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
994990
BIO_set_nbio(SSL_get_wbio(self->ssl), 1);
995991
}
996992

997-
PySSL_BEGIN_ALLOW_THREADS(self)
993+
Py_BEGIN_ALLOW_THREADS;
998994
if (socket_type == PY_SSL_CLIENT)
999995
SSL_set_connect_state(self->ssl);
1000996
else
1001997
SSL_set_accept_state(self->ssl);
1002-
PySSL_END_ALLOW_THREADS(self)
998+
Py_END_ALLOW_THREADS;
1003999

10041000
self->socket_type = socket_type;
10051001
if (sock != NULL) {
@@ -1068,10 +1064,11 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
10681064
/* Actually negotiate SSL connection */
10691065
/* XXX If SSL_do_handshake() returns 0, it's also a failure. */
10701066
do {
1071-
PySSL_BEGIN_ALLOW_THREADS(self)
1067+
Py_BEGIN_ALLOW_THREADS
10721068
ret = SSL_do_handshake(self->ssl);
10731069
err = _PySSL_errno(ret < 1, self->ssl, ret);
1074-
PySSL_END_ALLOW_THREADS(self)
1070+
Py_END_ALLOW_THREADS;
1071+
_PySSL_FIX_ERRNO;
10751072
self->err = err;
10761073

10771074
if (PyErr_CheckSignals())
@@ -2615,10 +2612,11 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset,
26152612
}
26162613

26172614
do {
2618-
PySSL_BEGIN_ALLOW_THREADS(self)
2615+
Py_BEGIN_ALLOW_THREADS
26192616
retval = SSL_sendfile(self->ssl, fd, (off_t)offset, size, flags);
26202617
err = _PySSL_errno(retval < 0, self->ssl, (int)retval);
2621-
PySSL_END_ALLOW_THREADS(self)
2618+
Py_END_ALLOW_THREADS;
2619+
_PySSL_FIX_ERRNO;
26222620
self->err = err;
26232621

26242622
if (PyErr_CheckSignals()) {
@@ -2746,10 +2744,11 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
27462744
}
27472745

27482746
do {
2749-
PySSL_BEGIN_ALLOW_THREADS(self)
2747+
Py_BEGIN_ALLOW_THREADS;
27502748
retval = SSL_write_ex(self->ssl, b->buf, (size_t)b->len, &count);
27512749
err = _PySSL_errno(retval == 0, self->ssl, retval);
2752-
PySSL_END_ALLOW_THREADS(self)
2750+
Py_END_ALLOW_THREADS;
2751+
_PySSL_FIX_ERRNO;
27532752
self->err = err;
27542753

27552754
if (PyErr_CheckSignals())
@@ -2807,10 +2806,11 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self)
28072806
int count = 0;
28082807
_PySSLError err;
28092808

2810-
PySSL_BEGIN_ALLOW_THREADS(self)
2809+
Py_BEGIN_ALLOW_THREADS;
28112810
count = SSL_pending(self->ssl);
28122811
err = _PySSL_errno(count < 0, self->ssl, count);
2813-
PySSL_END_ALLOW_THREADS(self)
2812+
Py_END_ALLOW_THREADS;
2813+
_PySSL_FIX_ERRNO;
28142814
self->err = err;
28152815

28162816
if (count < 0)
@@ -2901,10 +2901,11 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
29012901
deadline = _PyDeadline_Init(timeout);
29022902

29032903
do {
2904-
PySSL_BEGIN_ALLOW_THREADS(self)
2904+
Py_BEGIN_ALLOW_THREADS;
29052905
retval = SSL_read_ex(self->ssl, mem, (size_t)len, &count);
29062906
err = _PySSL_errno(retval == 0, self->ssl, retval);
2907-
PySSL_END_ALLOW_THREADS(self)
2907+
Py_END_ALLOW_THREADS;
2908+
_PySSL_FIX_ERRNO;
29082909
self->err = err;
29092910

29102911
if (PyErr_CheckSignals())
@@ -3003,7 +3004,7 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
30033004
}
30043005

30053006
while (1) {
3006-
PySSL_BEGIN_ALLOW_THREADS(self)
3007+
Py_BEGIN_ALLOW_THREADS;
30073008
/* Disable read-ahead so that unwrap can work correctly.
30083009
* Otherwise OpenSSL might read in too much data,
30093010
* eating clear text data that happens to be
@@ -3016,7 +3017,8 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
30163017
SSL_set_read_ahead(self->ssl, 0);
30173018
ret = SSL_shutdown(self->ssl);
30183019
err = _PySSL_errno(ret < 0, self->ssl, ret);
3019-
PySSL_END_ALLOW_THREADS(self)
3020+
Py_END_ALLOW_THREADS;
3021+
_PySSL_FIX_ERRNO;
30203022
self->err = err;
30213023

30223024
/* 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
@@ -140,7 +140,6 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line)
140140
* critical debug helper.
141141
*/
142142

143-
assert(PyMutex_IsLocked(&ssl_obj->tstate_mutex));
144143
Py_BEGIN_ALLOW_THREADS
145144
PyThread_acquire_lock(lock, 1);
146145
res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);

0 commit comments

Comments
 (0)