From 119d33a541e47497380f173aa0eacc567ad6a6b2 Mon Sep 17 00:00:00 2001 From: Jeff Van Voorst Date: Thu, 22 Aug 2024 20:46:46 -0500 Subject: [PATCH 1/9] gh-116810: fixed memory leak ssl module PySSL_get_session leaks a session object each time it is called. For programs like cherrypy (cheroot), a leak occurs with each handled web request. --- Modules/_ssl.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 1f5f0215980971..dfab6fa9e3bc68 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2853,10 +2853,6 @@ PySSL_get_session(PySSLSocket *self, void *closure) { if ((session = _ssl_session_dup(session)) == NULL) { return NULL; } - session = SSL_get1_session(self->ssl); - if (session == NULL) { - Py_RETURN_NONE; - } pysess = PyObject_GC_New(PySSLSession, self->ctx->state->PySSLSession_Type); if (pysess == NULL) { SSL_SESSION_free(session); From c98be51855061a7de280ea60d3c8b4315d8fcbfc Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 23 Aug 2024 15:49:11 +0000 Subject: [PATCH 2/9] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst diff --git a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst new file mode 100644 index 00000000000000..430fd0866bc3ed --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst @@ -0,0 +1 @@ +Resolved SSL session memory leak in the ssl module. From 8fff948aab7603a74f0aa9b20e75bdb6640d7d28 Mon Sep 17 00:00:00 2001 From: "Jeffrey R. Van Voorst" Date: Mon, 26 Aug 2024 10:29:03 -0500 Subject: [PATCH 3/9] Update Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst Co-authored-by: Peter Bierma --- .../next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst index 430fd0866bc3ed..e3c1107dfc8b8a 100644 --- a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst +++ b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst @@ -1 +1 @@ -Resolved SSL session memory leak in the ssl module. +Resolved SSL session memory leak in :mod:`ssl`. From 537c5bb1c09582d6da0611fff9f5f9f8b156a228 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 18 Sep 2024 23:25:41 -0700 Subject: [PATCH 4/9] Make the NEWS entry more detailed. --- .../Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst index e3c1107dfc8b8a..8725fa1dfe6648 100644 --- a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst +++ b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst @@ -1 +1,2 @@ -Resolved SSL session memory leak in :mod:`ssl`. +Resolve a memory leak introduced in CPython 3.10's :mod:`ssl` when the +:attr:`ssl.SSLSocket.session` property was accessed. From 0ddc1e2d7731a1a9fad0c9905c01351b37da2a0a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Wed, 18 Sep 2024 23:25:55 -0700 Subject: [PATCH 5/9] Use the simpler `SSL_get1_session` API that CPython <=3.9 used on all recent OpenSSL versions. --- Modules/_ssl.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index dfab6fa9e3bc68..7b0d3ee9cc3f1f 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2844,15 +2844,13 @@ PySSL_get_session(PySSLSocket *self, void *closure) { PySSLSession *pysess; SSL_SESSION *session; - /* duplicate session as workaround for session bug in OpenSSL 1.1.0, - * https://github.com/openssl/openssl/issues/1550 */ - session = SSL_get0_session(self->ssl); /* borrowed reference */ + /* See discussion on https://github.com/python/cpython/pull/123249 and + * older discussion on https://github.com/openssl/openssl/issues/1550. + * CPython is NOT doing the right thing here. */ + session = SSL_get1_session(self->ssl); if (session == NULL) { Py_RETURN_NONE; } - if ((session = _ssl_session_dup(session)) == NULL) { - return NULL; - } pysess = PyObject_GC_New(PySSLSession, self->ctx->state->PySSLSession_Type); if (pysess == NULL) { SSL_SESSION_free(session); From 46126b42980e14e88727657a1ca967253c161a53 Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Thu, 19 Sep 2024 21:47:02 +0200 Subject: [PATCH 6/9] Call SSL_set_shutdown before SSL_free, remove session deecopy hack entirely --- Modules/_ssl.c | 67 ++++++++++---------------------------------------- 1 file changed, 13 insertions(+), 54 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 7b0d3ee9cc3f1f..7659c7a7ea0d9a 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2251,6 +2251,17 @@ PySSL_dealloc(PySSLSocket *self) PyTypeObject *tp = Py_TYPE(self); PyObject_GC_UnTrack(self); if (self->ssl) { + // If we free the SSL socket object without having called SSL_shutdown, + // OpenSSL will invalidate the linked SSL session object. While this + // behavior is strictly RFC-compliant, it makes session reuse less + // likely and it would also break compatibility with older stdlib + // versions (which used an ugly workaround of duplicating the + // SSL_SESSION object). + // Therefore, we ensure the socket is marked as shutdown in any case. + // + // See elaborate explanation at + // https://github.com/python/cpython/pull/123249#discussion_r1766164530 + SSL_set_shutdown(self->ssl, SSL_SENT_SHUTDOWN); SSL_free(self->ssl); } Py_XDECREF(self->Socket); @@ -2795,48 +2806,6 @@ _ssl__SSLSocket_verify_client_post_handshake_impl(PySSLSocket *self) #endif } -static SSL_SESSION* -_ssl_session_dup(SSL_SESSION *session) { - SSL_SESSION *newsession = NULL; - int slen; - unsigned char *senc = NULL, *p; - const unsigned char *const_p; - - if (session == NULL) { - PyErr_SetString(PyExc_ValueError, "Invalid session"); - goto error; - } - - /* get length */ - slen = i2d_SSL_SESSION(session, NULL); - if (slen == 0 || slen > 0xFF00) { - PyErr_SetString(PyExc_ValueError, "i2d() failed"); - goto error; - } - if ((senc = PyMem_Malloc(slen)) == NULL) { - PyErr_NoMemory(); - goto error; - } - p = senc; - if (!i2d_SSL_SESSION(session, &p)) { - PyErr_SetString(PyExc_ValueError, "i2d() failed"); - goto error; - } - const_p = senc; - newsession = d2i_SSL_SESSION(NULL, &const_p, slen); - if (newsession == NULL) { - PyErr_SetString(PyExc_ValueError, "d2i() failed"); - goto error; - } - PyMem_Free(senc); - return newsession; - error: - if (senc != NULL) { - PyMem_Free(senc); - } - return NULL; -} - static PyObject * PySSL_get_session(PySSLSocket *self, void *closure) { /* get_session can return sessions from a server-side connection, @@ -2865,11 +2834,8 @@ PySSL_get_session(PySSLSocket *self, void *closure) { } static int PySSL_set_session(PySSLSocket *self, PyObject *value, - void *closure) - { + void *closure) { PySSLSession *pysess; - SSL_SESSION *session; - int result; if (!Py_IS_TYPE(value, get_state_sock(self)->PySSLSession_Type)) { PyErr_SetString(PyExc_TypeError, "Value is not a SSLSession."); @@ -2892,14 +2858,7 @@ static int PySSL_set_session(PySSLSocket *self, PyObject *value, "Cannot set session after handshake."); return -1; } - /* duplicate session */ - if ((session = _ssl_session_dup(pysess->session)) == NULL) { - return -1; - } - result = SSL_set_session(self->ssl, session); - /* free duplicate, SSL_set_session() bumps ref count */ - SSL_SESSION_free(session); - if (result == 0) { + if (SSL_set_session(self->ssl, pysess->session) == 0) { _setSSLError(get_state_sock(self), NULL, 0, __FILE__, __LINE__); return -1; } From 71318370ac6a0062cd1c83bd5e6e386bf6899ade Mon Sep 17 00:00:00 2001 From: Antoine Pitrou Date: Thu, 19 Sep 2024 21:52:29 +0200 Subject: [PATCH 7/9] Remove stale comment --- Modules/_ssl.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index 7659c7a7ea0d9a..a591c1dba5d57a 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2813,9 +2813,6 @@ PySSL_get_session(PySSLSocket *self, void *closure) { PySSLSession *pysess; SSL_SESSION *session; - /* See discussion on https://github.com/python/cpython/pull/123249 and - * older discussion on https://github.com/openssl/openssl/issues/1550. - * CPython is NOT doing the right thing here. */ session = SSL_get1_session(self->ssl); if (session == NULL) { Py_RETURN_NONE; From 96ab92b7dbb4fb8a4ebeb4d6de7173edfbe81b9a Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 19 Sep 2024 14:03:18 -0700 Subject: [PATCH 8/9] expands news to mention the lack of internal serialization speedup. --- .../Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst index 8725fa1dfe6648..0e5256e7151c5a 100644 --- a/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst +++ b/Misc/NEWS.d/next/Library/2024-08-23-15-49-10.gh-issue-116810.QLBUU8.rst @@ -1,2 +1,4 @@ Resolve a memory leak introduced in CPython 3.10's :mod:`ssl` when the -:attr:`ssl.SSLSocket.session` property was accessed. +:attr:`ssl.SSLSocket.session` property was accessed. Speeds up read and +write access to said property by no longer unnecessarily cloning session +objects via serialization. From cb415bc60061b7518df74b0eca790c122d5aa9ae Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 19 Sep 2024 21:39:07 -0700 Subject: [PATCH 9/9] per davidben, lets not clear a shutdown bit. --- Modules/_ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_ssl.c b/Modules/_ssl.c index a591c1dba5d57a..f2d3b331226a7a 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2261,7 +2261,7 @@ PySSL_dealloc(PySSLSocket *self) // // See elaborate explanation at // https://github.com/python/cpython/pull/123249#discussion_r1766164530 - SSL_set_shutdown(self->ssl, SSL_SENT_SHUTDOWN); + SSL_set_shutdown(self->ssl, SSL_SENT_SHUTDOWN | SSL_get_shutdown(self->ssl)); SSL_free(self->ssl); } Py_XDECREF(self->Socket);