gh-137583: Only lock the SSL context, not the SSL socket (GH-137588)
Fixes a deadlock in 3.13.6.
This commit is contained in:
@@ -4627,6 +4627,42 @@ class ThreadedTests(unittest.TestCase):
|
|||||||
with client_context.wrap_socket(socket.socket()) as s:
|
with client_context.wrap_socket(socket.socket()) as s:
|
||||||
s.connect((HOST, server.port))
|
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,
|
@unittest.skipUnless(has_tls_version('TLSv1_3') and ssl.HAS_PHA,
|
||||||
"Test needs TLS 1.3 PHA")
|
"Test needs TLS 1.3 PHA")
|
||||||
|
|||||||
@@ -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.
|
||||||
@@ -366,9 +366,6 @@ typedef struct {
|
|||||||
* and shutdown methods check for chained exceptions.
|
* and shutdown methods check for chained exceptions.
|
||||||
*/
|
*/
|
||||||
PyObject *exc;
|
PyObject *exc;
|
||||||
/* Lock to synchronize calls when the thread state is detached.
|
|
||||||
See also gh-134698. */
|
|
||||||
PyMutex tstate_mutex;
|
|
||||||
} PySSLSocket;
|
} PySSLSocket;
|
||||||
|
|
||||||
#define PySSLSocket_CAST(op) ((PySSLSocket *)(op))
|
#define PySSLSocket_CAST(op) ((PySSLSocket *)(op))
|
||||||
@@ -918,7 +915,6 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
|
|||||||
self->server_hostname = NULL;
|
self->server_hostname = NULL;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
self->exc = NULL;
|
self->exc = NULL;
|
||||||
self->tstate_mutex = (PyMutex){0};
|
|
||||||
|
|
||||||
/* Make sure the SSL error state is initialized */
|
/* Make sure the SSL error state is initialized */
|
||||||
ERR_clear_error();
|
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)
|
if (socket_type == PY_SSL_CLIENT)
|
||||||
SSL_set_connect_state(self->ssl);
|
SSL_set_connect_state(self->ssl);
|
||||||
else
|
else
|
||||||
SSL_set_accept_state(self->ssl);
|
SSL_set_accept_state(self->ssl);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
|
||||||
self->socket_type = socket_type;
|
self->socket_type = socket_type;
|
||||||
if (sock != NULL) {
|
if (sock != NULL) {
|
||||||
@@ -1068,10 +1064,11 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
|
|||||||
/* Actually negotiate SSL connection */
|
/* Actually negotiate SSL connection */
|
||||||
/* XXX If SSL_do_handshake() returns 0, it's also a failure. */
|
/* XXX If SSL_do_handshake() returns 0, it's also a failure. */
|
||||||
do {
|
do {
|
||||||
PySSL_BEGIN_ALLOW_THREADS(self)
|
Py_BEGIN_ALLOW_THREADS
|
||||||
ret = SSL_do_handshake(self->ssl);
|
ret = SSL_do_handshake(self->ssl);
|
||||||
err = _PySSL_errno(ret < 1, self->ssl, ret);
|
err = _PySSL_errno(ret < 1, self->ssl, ret);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
if (PyErr_CheckSignals())
|
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);
|
retval = SSL_sendfile(self->ssl, fd, (off_t)offset, size, flags);
|
||||||
err = _PySSL_errno(retval < 0, self->ssl, (int)retval);
|
err = _PySSL_errno(retval < 0, self->ssl, (int)retval);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
if (PyErr_CheckSignals()) {
|
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);
|
retval = SSL_write_ex(self->ssl, b->buf, (size_t)b->len, &count);
|
||||||
err = _PySSL_errno(retval == 0, self->ssl, retval);
|
err = _PySSL_errno(retval == 0, self->ssl, retval);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
if (PyErr_CheckSignals())
|
if (PyErr_CheckSignals())
|
||||||
@@ -2807,10 +2806,11 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self)
|
|||||||
int count = 0;
|
int count = 0;
|
||||||
_PySSLError err;
|
_PySSLError err;
|
||||||
|
|
||||||
PySSL_BEGIN_ALLOW_THREADS(self)
|
Py_BEGIN_ALLOW_THREADS;
|
||||||
count = SSL_pending(self->ssl);
|
count = SSL_pending(self->ssl);
|
||||||
err = _PySSL_errno(count < 0, self->ssl, count);
|
err = _PySSL_errno(count < 0, self->ssl, count);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
if (count < 0)
|
if (count < 0)
|
||||||
@@ -2901,10 +2901,11 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
|
|||||||
deadline = _PyDeadline_Init(timeout);
|
deadline = _PyDeadline_Init(timeout);
|
||||||
|
|
||||||
do {
|
do {
|
||||||
PySSL_BEGIN_ALLOW_THREADS(self)
|
Py_BEGIN_ALLOW_THREADS;
|
||||||
retval = SSL_read_ex(self->ssl, mem, (size_t)len, &count);
|
retval = SSL_read_ex(self->ssl, mem, (size_t)len, &count);
|
||||||
err = _PySSL_errno(retval == 0, self->ssl, retval);
|
err = _PySSL_errno(retval == 0, self->ssl, retval);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
if (PyErr_CheckSignals())
|
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.
|
/* Disable read-ahead so that unwrap can work correctly.
|
||||||
* Otherwise OpenSSL might read in too much data,
|
* Otherwise OpenSSL might read in too much data,
|
||||||
* eating clear text data that happens to be
|
* 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);
|
SSL_set_read_ahead(self->ssl, 0);
|
||||||
ret = SSL_shutdown(self->ssl);
|
ret = SSL_shutdown(self->ssl);
|
||||||
err = _PySSL_errno(ret < 0, self->ssl, ret);
|
err = _PySSL_errno(ret < 0, self->ssl, ret);
|
||||||
PySSL_END_ALLOW_THREADS(self)
|
Py_END_ALLOW_THREADS;
|
||||||
|
_PySSL_FIX_ERRNO;
|
||||||
self->err = err;
|
self->err = err;
|
||||||
|
|
||||||
/* If err == 1, a secure shutdown with SSL_shutdown() is complete */
|
/* If err == 1, a secure shutdown with SSL_shutdown() is complete */
|
||||||
|
|||||||
@@ -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
|
Py_BEGIN_ALLOW_THREADS
|
||||||
PyThread_acquire_lock(lock, 1);
|
PyThread_acquire_lock(lock, 1);
|
||||||
res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);
|
res = BIO_printf(ssl_obj->ctx->keylog_bio, "%s\n", line);
|
||||||
|
|||||||
Reference in New Issue
Block a user