Skip to content

Commit 7d155d7

Browse files
colesburycolesbury
authored
gh-140795: Remove 'exc' field in SSLObject (gh-143491)
The 'exc' field was used by our debug SSL callbacks. Keep the exception in the normal per-thread state to avoid shared mutable state between threads. This also avoids a reference count leak if the Python callback raised an exception because it can be called multiple times per SSL operation.
1 parent 1de4671 commit 7d155d7

File tree

3 files changed

+97
-53
lines changed

3 files changed

+97
-53
lines changed

‎Lib/test/test_ssl.py‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5277,6 +5277,20 @@ def msg_cb(conn, direction, version, content_type, msg_type, data):
52775277
with self.assertRaises(TypeError):
52785278
client_context._msg_callback = object()
52795279

5280+
def test_msg_callback_exception(self):
5281+
client_context, server_context, hostname = testing_context()
5282+
5283+
def msg_cb(conn, direction, version, content_type, msg_type, data):
5284+
raise RuntimeError("msg_cb exception")
5285+
5286+
client_context._msg_callback = msg_cb
5287+
server = ThreadedEchoServer(context=server_context, chatty=False)
5288+
with server:
5289+
with client_context.wrap_socket(socket.socket(),
5290+
server_hostname=hostname) as s:
5291+
with self.assertRaisesRegex(RuntimeError, "msg_cb exception"):
5292+
s.connect((HOST, server.port))
5293+
52805294
def test_msg_callback_tls12(self):
52815295
client_context, server_context, hostname = testing_context()
52825296
client_context.maximum_version = ssl.TLSVersion.TLSv1_2

‎Modules/_ssl.c‎

Lines changed: 70 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -360,11 +360,6 @@ typedef struct {
360360
enum py_ssl_server_or_client socket_type;
361361
PyObject *owner; /* Python level "owner" passed to servername callback */
362362
PyObject *server_hostname;
363-
/* Some SSL callbacks don't have error reporting. Callback wrappers
364-
* store exception information on the socket. The handshake, read, write,
365-
* and shutdown methods check for chained exceptions.
366-
*/
367-
PyObject *exc;
368363
} PySSLSocket;
369364

370365
#define PySSLSocket_CAST(op) ((PySSLSocket *)(op))
@@ -657,18 +652,12 @@ fill_and_set_sslerror(_sslmodulestate *state,
657652
PyUnicodeWriter_Discard(writer);
658653
}
659654

660-
static int
661-
PySSL_ChainExceptions(PySSLSocket *sslsock) {
662-
if (sslsock->exc == NULL)
663-
return 0;
664-
665-
_PyErr_ChainExceptions1(sslsock->exc);
666-
sslsock->exc = NULL;
667-
return -1;
668-
}
669-
655+
// Set the appropriate SSL error exception.
656+
// err - error information from SSL and libc
657+
// exc - if not NULL, an exception from _debughelpers.c callback to be chained
670658
static PyObject *
671-
PySSL_SetError(PySSLSocket *sslsock, _PySSLError err, const char *filename, int lineno)
659+
PySSL_SetError(PySSLSocket *sslsock, _PySSLError err, PyObject *exc,
660+
const char *filename, int lineno)
672661
{
673662
PyObject *type;
674663
char *errstr = NULL;
@@ -776,7 +765,7 @@ PySSL_SetError(PySSLSocket *sslsock, _PySSLError err, const char *filename, int
776765
}
777766
fill_and_set_sslerror(state, sslsock, type, p, errstr, lineno, e);
778767
ERR_clear_error();
779-
PySSL_ChainExceptions(sslsock);
768+
_PyErr_ChainExceptions1(exc); // chain any exceptions from callbacks
780769
return NULL;
781770
}
782771

@@ -908,7 +897,6 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
908897
self->shutdown_seen_zero = 0;
909898
self->owner = NULL;
910899
self->server_hostname = NULL;
911-
self->exc = NULL;
912900

913901
/* Make sure the SSL error state is initialized */
914902
ERR_clear_error();
@@ -1029,6 +1017,7 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
10291017
{
10301018
int ret;
10311019
_PySSLError err;
1020+
PyObject *exc = NULL;
10321021
int sockstate, nonblocking;
10331022
PySocketSockObject *sock = GET_SOCKET(self);
10341023
PyTime_t timeout, deadline = 0;
@@ -1064,6 +1053,12 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
10641053
Py_END_ALLOW_THREADS;
10651054
_PySSL_FIX_ERRNO;
10661055

1056+
// Get any exception that occurred in a debughelpers.c callback
1057+
exc = PyErr_GetRaisedException();
1058+
if (exc != NULL) {
1059+
break;
1060+
}
1061+
10671062
if (PyErr_CheckSignals())
10681063
goto error;
10691064

@@ -1098,13 +1093,15 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
10981093
Py_XDECREF(sock);
10991094

11001095
if (ret < 1)
1101-
return PySSL_SetError(self, err, __FILE__, __LINE__);
1102-
if (PySSL_ChainExceptions(self) < 0)
1096+
return PySSL_SetError(self, err, exc, __FILE__, __LINE__);
1097+
if (exc != NULL) {
1098+
PyErr_SetRaisedException(exc);
11031099
return NULL;
1100+
}
11041101
Py_RETURN_NONE;
11051102
error:
1103+
assert(exc == NULL);
11061104
Py_XDECREF(sock);
1107-
PySSL_ChainExceptions(self);
11081105
return NULL;
11091106
}
11101107

@@ -2434,17 +2431,7 @@ _ssl__SSLSocket_owner_set_impl(PySSLSocket *self, PyObject *value)
24342431
static int
24352432
PySSL_traverse(PyObject *op, visitproc visit, void *arg)
24362433
{
2437-
PySSLSocket *self = PySSLSocket_CAST(op);
2438-
Py_VISIT(self->exc);
2439-
Py_VISIT(Py_TYPE(self));
2440-
return 0;
2441-
}
2442-
2443-
static int
2444-
PySSL_clear(PyObject *op)
2445-
{
2446-
PySSLSocket *self = PySSLSocket_CAST(op);
2447-
Py_CLEAR(self->exc);
2434+
Py_VISIT(Py_TYPE(op));
24482435
return 0;
24492436
}
24502437

@@ -2619,6 +2606,7 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset,
26192606
Py_ssize_t retval;
26202607
int sockstate;
26212608
_PySSLError err;
2609+
PyObject *exc = NULL;
26222610
PySocketSockObject *sock = GET_SOCKET(self);
26232611
PyTime_t timeout, deadline = 0;
26242612
int has_timeout;
@@ -2666,6 +2654,11 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset,
26662654
Py_END_ALLOW_THREADS;
26672655
_PySSL_FIX_ERRNO;
26682656

2657+
exc = PyErr_GetRaisedException();
2658+
if (exc != NULL) {
2659+
break;
2660+
}
2661+
26692662
if (PyErr_CheckSignals()) {
26702663
goto error;
26712664
}
@@ -2715,15 +2708,18 @@ _ssl__SSLSocket_sendfile_impl(PySSLSocket *self, int fd, Py_off_t offset,
27152708
}
27162709
Py_XDECREF(sock);
27172710
if (retval < 0) {
2718-
return PySSL_SetError(self, err, __FILE__, __LINE__);
2711+
return PySSL_SetError(self, err, exc, __FILE__, __LINE__);
27192712
}
2720-
if (PySSL_ChainExceptions(self) < 0) {
2713+
if (exc != NULL) {
2714+
PyErr_SetRaisedException(exc);
27212715
return NULL;
27222716
}
27232717
return PyLong_FromSize_t(retval);
27242718
error:
27252719
Py_XDECREF(sock);
2726-
(void)PySSL_ChainExceptions(self);
2720+
if (exc != NULL) {
2721+
_PyErr_ChainExceptions1(exc);
2722+
}
27272723
return NULL;
27282724
}
27292725
#endif /* BIO_get_ktls_send */
@@ -2747,6 +2743,7 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
27472743
int retval;
27482744
int sockstate;
27492745
_PySSLError err;
2746+
PyObject *exc = NULL;
27502747
int nonblocking;
27512748
PySocketSockObject *sock = GET_SOCKET(self);
27522749
PyTime_t timeout, deadline = 0;
@@ -2797,6 +2794,11 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
27972794
Py_END_ALLOW_THREADS;
27982795
_PySSL_FIX_ERRNO;
27992796

2797+
exc = PyErr_GetRaisedException();
2798+
if (exc != NULL) {
2799+
break;
2800+
}
2801+
28002802
if (PyErr_CheckSignals())
28012803
goto error;
28022804

@@ -2828,13 +2830,15 @@ _ssl__SSLSocket_write_impl(PySSLSocket *self, Py_buffer *b)
28282830

28292831
Py_XDECREF(sock);
28302832
if (retval == 0)
2831-
return PySSL_SetError(self, err, __FILE__, __LINE__);
2832-
if (PySSL_ChainExceptions(self) < 0)
2833+
return PySSL_SetError(self, err, exc, __FILE__, __LINE__);
2834+
if (exc != NULL) {
2835+
PyErr_SetRaisedException(exc);
28332836
return NULL;
2837+
}
28342838
return PyLong_FromSize_t(count);
28352839
error:
2840+
assert(exc == NULL);
28362841
Py_XDECREF(sock);
2837-
PySSL_ChainExceptions(self);
28382842
return NULL;
28392843
}
28402844

@@ -2860,7 +2864,7 @@ _ssl__SSLSocket_pending_impl(PySSLSocket *self)
28602864
_PySSL_FIX_ERRNO;
28612865

28622866
if (count < 0)
2863-
return PySSL_SetError(self, err, __FILE__, __LINE__);
2867+
return PySSL_SetError(self, err, NULL, __FILE__, __LINE__);
28642868
else
28652869
return PyLong_FromLong(count);
28662870
}
@@ -2888,6 +2892,7 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
28882892
int retval;
28892893
int sockstate;
28902894
_PySSLError err;
2895+
PyObject *exc = NULL;
28912896
int nonblocking;
28922897
PySocketSockObject *sock = GET_SOCKET(self);
28932898
PyTime_t timeout, deadline = 0;
@@ -2955,6 +2960,11 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
29552960
Py_END_ALLOW_THREADS;
29562961
_PySSL_FIX_ERRNO;
29572962

2963+
exc = PyErr_GetRaisedException();
2964+
if (exc != NULL) {
2965+
break;
2966+
}
2967+
29582968
if (PyErr_CheckSignals())
29592969
goto error;
29602970

@@ -2986,13 +2996,18 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
29862996
err.ssl == SSL_ERROR_WANT_WRITE);
29872997

29882998
if (retval == 0) {
2989-
PySSL_SetError(self, err, __FILE__, __LINE__);
2999+
PySSL_SetError(self, err, exc, __FILE__, __LINE__);
3000+
exc = NULL;
29903001
goto error;
29913002
}
2992-
if (self->exc != NULL)
3003+
else if (exc != NULL) {
3004+
PyErr_SetRaisedException(exc);
3005+
exc = NULL;
29933006
goto error;
3007+
}
29943008

29953009
done:
3010+
assert(exc == NULL);
29963011
Py_XDECREF(sock);
29973012
if (!group_right_1) {
29983013
return PyBytesWriter_FinishWithSize(writer, count);
@@ -3002,7 +3017,7 @@ _ssl__SSLSocket_read_impl(PySSLSocket *self, Py_ssize_t len,
30023017
}
30033018

30043019
error:
3005-
PySSL_ChainExceptions(self);
3020+
assert(exc == NULL);
30063021
Py_XDECREF(sock);
30073022
if (!group_right_1) {
30083023
PyBytesWriter_Discard(writer);
@@ -3022,6 +3037,7 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
30223037
/*[clinic end generated code: output=ca1aa7ed9d25ca42 input=98d9635cd4e16514]*/
30233038
{
30243039
_PySSLError err;
3040+
PyObject *exc = NULL;
30253041
int sockstate, nonblocking, ret;
30263042
int zeros = 0;
30273043
PySocketSockObject *sock = GET_SOCKET(self);
@@ -3067,6 +3083,11 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
30673083
Py_END_ALLOW_THREADS;
30683084
_PySSL_FIX_ERRNO;
30693085

3086+
exc = PyErr_GetRaisedException();
3087+
if (exc != NULL) {
3088+
break;
3089+
}
3090+
30703091
/* If err == 1, a secure shutdown with SSL_shutdown() is complete */
30713092
if (ret > 0)
30723093
break;
@@ -3113,20 +3134,23 @@ _ssl__SSLSocket_shutdown_impl(PySSLSocket *self)
31133134
}
31143135
if (ret < 0) {
31153136
Py_XDECREF(sock);
3116-
PySSL_SetError(self, err, __FILE__, __LINE__);
3137+
PySSL_SetError(self, err, exc, __FILE__, __LINE__);
3138+
return NULL;
3139+
}
3140+
else if (exc != NULL) {
3141+
Py_XDECREF(sock);
3142+
PyErr_SetRaisedException(exc);
31173143
return NULL;
31183144
}
3119-
if (self->exc != NULL)
3120-
goto error;
31213145
if (sock)
31223146
/* It's already INCREF'ed */
31233147
return (PyObject *) sock;
31243148
else
31253149
Py_RETURN_NONE;
31263150

31273151
error:
3152+
assert(exc == NULL);
31283153
Py_XDECREF(sock);
3129-
PySSL_ChainExceptions(self);
31303154
return NULL;
31313155
}
31323156

@@ -3335,7 +3359,6 @@ static PyType_Slot PySSLSocket_slots[] = {
33353359
{Py_tp_getset, ssl_getsetlist},
33363360
{Py_tp_dealloc, PySSL_dealloc},
33373361
{Py_tp_traverse, PySSL_traverse},
3338-
{Py_tp_clear, PySSL_clear},
33393362
{0, 0},
33403363
};
33413364

‎Modules/_ssl/debughelpers.c‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ _PySSL_msg_callback(int write_p, int version, int content_type,
2626
return;
2727
}
2828

29+
PyObject *exc = PyErr_GetRaisedException();
30+
2931
PyObject *ssl_socket; /* ssl.SSLSocket or ssl.SSLObject */
3032
if (ssl_obj->owner)
3133
PyWeakref_GetRef(ssl_obj->owner, &ssl_socket);
@@ -73,13 +75,13 @@ _PySSL_msg_callback(int write_p, int version, int content_type,
7375
version, content_type, msg_type,
7476
buf, len
7577
);
76-
if (res == NULL) {
77-
ssl_obj->exc = PyErr_GetRaisedException();
78-
} else {
79-
Py_DECREF(res);
80-
}
78+
Py_XDECREF(res);
8179
Py_XDECREF(ssl_socket);
8280

81+
if (exc != NULL) {
82+
_PyErr_ChainExceptions1(exc);
83+
}
84+
8385
PyGILState_Release(threadstate);
8486
}
8587

@@ -122,10 +124,13 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line)
122124
{
123125
PyGILState_STATE threadstate;
124126
PySSLSocket *ssl_obj = NULL; /* ssl._SSLSocket, borrowed ref */
127+
PyObject *exc;
125128
int res, e;
126129

127130
threadstate = PyGILState_Ensure();
128131

132+
exc = PyErr_GetRaisedException();
133+
129134
ssl_obj = (PySSLSocket *)SSL_get_app_data(ssl);
130135
assert(Py_IS_TYPE(ssl_obj, get_state_sock(ssl_obj)->PySSLSocket_Type));
131136
PyThread_type_lock lock = get_state_sock(ssl_obj)->keylog_lock;
@@ -153,10 +158,12 @@ _PySSL_keylog_callback(const SSL *ssl, const char *line)
153158
errno = e;
154159
PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError,
155160
ssl_obj->ctx->keylog_filename);
156-
ssl_obj->exc = PyErr_GetRaisedException();
157161
}
158162

159163
done:
164+
if (exc != NULL) {
165+
_PyErr_ChainExceptions1(exc);
166+
}
160167
PyGILState_Release(threadstate);
161168
}
162169

0 commit comments

Comments
 (0)