gh-99813: Start using `SSL_sendfile` when available #99907

illia-v · 2022-11-30T18:14:26Z

Issue: Use SSL_sendfile() from OpenSSL 3.0 in SSLSocket.sendfile when available. #99813

New _ssl__SSLSocket_sendfile_impl function is very similar to _ssl__SSLSocket_write_impl with a few adjustments for the SSL_sendfile syscall.

I am glad there was non-SSL socket.socket.sendfile and underlying socket.socket._sendfile_use_sendfile implemented. I reused that code for new ssl.SSLSocket._sendfile_use_ssl_sendfile by moving shared logic to socket.socket._sendfile_zerocopy.

And test.test_ssl.ThreadedTests.test_sendfile needed to have a socket bound before an SSL context wrapped it because this seemed to affect kTLS availability.

Conflicts: Modules/clinic/_ssl.c.h

illia-v · 2023-03-13T21:30:29Z

@gpshead could you please review this when you have a chance?

This is similar to https://github.com/nginx/nginx/blob/1c6183725247024f1bca73ac9a833098af7558af/src/event/ngx_event_openssl.c#L3078-L3089

illia-v · 2023-07-26T18:02:00Z

I've recently pushed some changes after testing the new functionality manually by sending small and large files from Linux with the tls module loaded, it worked well

gpshead

I won't have time to look at this in detail for a while. I'm just dropping a note about one issue seen while quickly skimming it.

Modules/_ssl.c

picnixz

Some comments. The if-else if blocks need to follow PEP-7 but you can also use a switch if you want (it can be clearer to read).

Lib/socket.py

Lib/ssl.py

Lib/socket.py

Modules/_ssl.c

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

picnixz

few final comments

Doc/library/ssl.rst

picnixz · 2025-04-17T16:57:00Z

Lib/test/test_ssl.py

-            with client_context.wrap_socket(socket.socket(),
-                                            server_hostname=hostname) as s:
-                s.connect((HOST, server.port))
+        # kTLS seems to work only with a connection created before


Can you check whether it "seems" to work or not? (for a personal project I would accept this but I'm interested in knowing whether this is an issue with Python SSL or OpenSSL)

Okay, I'll post more details about this soon

I can reproduce the same issue with C code using OpenSSL (without Python). Please check my findings here. I mentioned a possible patch, let me know if we can apply it or there can be undesirable consequences.

FYI, ssl.OP_ENABLE_KTLS to enable kTLS has been available since Python 3.12 (and it could be added to options as a simple integer even before). And the issue is not specific to SSL_sendfile, it affects kTLS availability for simple reads and writes.

I added my reproducer to an existing OpenSSL issue openssl/openssl#19676 (comment).

Lib/test/test_ssl.py

Misc/NEWS.d/next/Library/2023-03-13-22-51-40.gh-issue-99813.40TV02.rst

Modules/_ssl.c

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

openssl/openssl@524bac5

illia-v · 2025-04-19T15:31:54Z

@picnixz OpenSSL provides SSL_OP_ENABLE_KTLS_TX_ZEROCOPY_SENDFILE which will be good to have in Python together with the change. Should I add it in this PR or separately after this one is merged?

picnixz

I feel we need explicit functions for common error messages just to always use the same ones (but it's a separate issue)

Modules/_ssl.c

Lib/test/test_ssl.py

Modules/_ssl.c

picnixz

I'd like @ZeroIntensity to have a look for threads safety and @gpshead to confirm the implementation as well. We could add a What's New entry but it's more of an implementation detail so I would prefer not to in the first place.

picnixz · 2025-04-30T07:47:05Z

Modules/_ssl.c

@@ -74,6 +74,33 @@
 #endif


+#ifdef BIO_get_ktls_send
+#  ifdef MS_WINDOWS
+typedef long long Py_off_t;


I think I've seen this construction elsewhere. This is off topic but I think we should add it to the default clinic converters or be able to share code like that across clinic generated code. I need to think about it later.

picnixz · 2025-04-30T07:47:36Z

Modules/_ssl.c

+    // Also, it returns -1 for failure before OpenSSL 3.0.4.
+    return Py_NewRef(uses == 1 ? Py_True : Py_False);
+#else
+    return Py_NewRef(Py_False);


For this one, use Py_RETURN_FALSE

ZeroIntensity · 2025-05-01T12:22:18Z

Modules/_ssl.c

+[clinic start generated code]*/
+
+static PyObject *
+_ssl__SSLSocket_uses_ktls_for_send_impl(PySSLSocket *self)


This function needs a critical section. Concurrent calls to SSL_get_wbio aren't thread-safe without the lock.

ZeroIntensity · 2025-05-01T12:22:30Z

Modules/_ssl.c

+    int uses = BIO_get_ktls_send(SSL_get_wbio(self->ssl));
+    // BIO_get_ktls_send() returns 1 if kTLS is used and 0 if not.
+    // Also, it returns -1 for failure before OpenSSL 3.0.4.
+    return Py_NewRef(uses == 1 ? Py_True : Py_False);


This is an immortal reference, you can omit Py_NewRef.

Ideally it's better to make it explicit. We had a similar discussion on the discord and on some issues

Oh? I thought the consensus was to omit immortal reference counting.

ZeroIntensity · 2025-05-01T12:22:42Z

Modules/_ssl.c

+[clinic start generated code]*/
+
+static PyObject *
+_ssl__SSLSocket_uses_ktls_for_recv_impl(PySSLSocket *self)


Ditto, let's add a critical section.

ZeroIntensity · 2025-05-01T12:27:29Z

Misc/NEWS.d/next/Library/2023-03-13-22-51-40.gh-issue-99813.40TV02.rst

@@ -0,0 +1,4 @@
+Python now uses ``SSL_sendfile`` internally when it is possible (see


We know it's Python, but we don't know it's SSL:

Suggested change

Python now uses ``SSL_sendfile`` internally when it is possible (see

:mod:`ssl` now uses ``SSL_sendfile`` internally when it is possible (see

It's the same wording as the previous entry (in the docs)

I think it makes sense in the docs because there's context. It's not immediately clear that we're talking about ssl here.

ZeroIntensity · 2025-05-01T12:31:59Z

Modules/_ssl.c

+    int has_timeout;
+
+    if (sock != NULL) {
+        if ((PyObject *)sock == Py_None) {


Not a real issue, but thinking out loud for me and Bénédikt: does Py_Is come with _PyObject_CAST? If so, it seems more useful than == here. If not, we should add it.

ZeroIntensity · 2025-05-01T12:32:55Z

Modules/_ssl.c

+        Py_INCREF(sock);
+    }
+
+    if (sock != NULL) {


Why isn't this merged with the case above?

ZeroIntensity · 2025-05-01T12:40:37Z

Doc/library/ssl.rst

-   - :meth:`~socket.socket.sendfile` (but :mod:`os.sendfile` will be used
-     for plain-text sockets only, else :meth:`~socket.socket.send` will be used)
+   - :meth:`~socket.socket.sendfile` (but it may be high-performant only when
+     the kernel TLS is enabled (see :data:`~ssl.OP_ENABLE_KTLS`) or when a


I'm not sure what we do about nested parentheses in the docs. I haven't seen them anywhere else, so I'd like to avoid them here. Could we reword this a little to avoid that?

illia-v added 2 commits November 30, 2022 20:05

Add methods for checking whether kTLS is used

a018beb

Start using SSL_sendfile when available

72e7f5e

bedevere-bot mentioned this pull request Nov 30, 2022

Use SSL_sendfile() from OpenSSL 3.0 in SSLSocket.sendfile when available. #99813

Open

bedevere-bot added the awaiting review label Nov 30, 2022

illia-v changed the title ~~gh-99813: Start using SSL_sendfile when available~~ gh-99813: Start using SSL_sendfile when available Nov 30, 2022

illia-v added 9 commits December 5, 2022 21:37

Try fixing warnings

cf24d78

Use unsigned size_t instead of Py_ssize_t

e9ef747

Merge branch 'main' into SSL_sendfile

646d328

Merge branch 'main'

76353c7

Conflicts: Modules/clinic/_ssl.c.h

Make sendfile_impl more similar to write_impl

14832de

Modify test_ssl to test SSL_sendfile calls

6b41d31

Modify documentation and add a news entry

8ac6ff9

Fix a test

5cf9483

Rename uses_ktls_for_write to uses_ktls_for_send

fc8e82f

illia-v marked this pull request as ready for review March 13, 2023 21:29

illia-v added 8 commits July 25, 2023 21:53

Merge branch 'main' into SSL_sendfile

7fc9b50

Update versionchanged

1ba04ae

Update a test to get successful HTTP responses

365e0c0

Add setting errors

8fbc955

Fix conditions

21ea0ca

Add handling of SSL_R_UNINITIALIZED

17d9685

This is similar to https://github.com/nginx/nginx/blob/1c6183725247024f1bca73ac9a833098af7558af/src/event/ngx_event_openssl.c#L3078-L3089

Merge branch 'main' into SSL_sendfile

a92ae7e

Modify test_sendfile to avoid the internet

4e8b6d8

AlexWaygood requested a review from gpshead July 26, 2023 17:49

Refactor test_sendfile a bit

379d242

gpshead reviewed Jul 30, 2023

View reviewed changes

Modules/_ssl.c Outdated Show resolved Hide resolved

illia-v requested a review from gpshead August 2, 2023 19:54

Merge branch 'main' into SSL_sendfile

7de3dcb

illia-v added 4 commits December 27, 2024 21:41

Merge branch 'main' into SSL_sendfile

dc626c8

Set versionchanged to next in docs

291a5b7

Merge branch 'main' into SSL_sendfile

3193ba4

Merge branch 'main' into SSL_sendfile

d870f92

illia-v requested a review from picnixz as a code owner April 10, 2025 23:17

picnixz reviewed Apr 12, 2025

View reviewed changes

illia-v and others added 4 commits April 12, 2025 16:27

Apply some suggestions from code review

94e522e

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Apply suggestions to Python code

25bde6f

Improve style of _ssl__SSLSocket_sendfile_impl

9a6a120

Merge branch 'main' into SSL_sendfile

eaa0b2c

illia-v requested a review from picnixz April 12, 2025 14:22

illia-v added 2 commits April 13, 2025 18:41

Drop _sendfile_use_ssl_sendfile

05a0c6c

Merge remote-tracking branch 'python/main' into SSL_sendfile

6b4eed6

picnixz reviewed Apr 17, 2025

View reviewed changes

illia-v and others added 3 commits April 18, 2025 16:27

Apply suggestions from code review

31ed52d

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Merge branch 'main' into SSL_sendfile

5a22e6b

Improve kTLS checks for older OpenSSL

ada3f30

openssl/openssl@524bac5

illia-v requested a review from picnixz April 19, 2025 15:33

picnixz reviewed Apr 24, 2025

View reviewed changes

Modules/_ssl.c Outdated Show resolved Hide resolved

Lib/test/test_ssl.py Show resolved Hide resolved

Modules/_ssl.c Show resolved Hide resolved

Modules/_ssl.c Outdated Show resolved Hide resolved

Modules/_ssl.c Outdated Show resolved Hide resolved

illia-v added 4 commits April 28, 2025 23:42

Merge branch 'main' into SSL_sendfile

3350854

Rename uses_ktls_for_read to uses_ktls_for_recv

3339673

Apply PEP 7

ee573ad

Use an alternative method of returning booleans

6795f62

illia-v requested a review from picnixz April 28, 2025 21:16

picnixz approved these changes Apr 30, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting review labels Apr 30, 2025

ZeroIntensity reviewed May 1, 2025

View reviewed changes

ZeroIntensity added the topic-SSL label May 1, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-99813: Start using `SSL_sendfile` when available #99907

gh-99813: Start using `SSL_sendfile` when available #99907

illia-v commented Nov 30, 2022 •

edited

Loading

illia-v commented Mar 13, 2023

illia-v commented Jul 26, 2023

gpshead left a comment

picnixz left a comment

picnixz left a comment

picnixz Apr 17, 2025

illia-v Apr 18, 2025

illia-v Apr 19, 2025

illia-v commented Apr 19, 2025

picnixz left a comment •

edited

Loading

picnixz left a comment

picnixz Apr 30, 2025

picnixz Apr 30, 2025

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

picnixz May 1, 2025

ZeroIntensity May 1, 2025 •

edited

Loading

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

picnixz May 1, 2025 •

edited

Loading

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

		@@ -0,0 +1,4 @@
		Python now uses ``SSL_sendfile`` internally when it is possible (see

	Python now uses ``SSL_sendfile`` internally when it is possible (see
	:mod:`ssl` now uses ``SSL_sendfile`` internally when it is possible (see

gh-99813: Start using SSL_sendfile when available #99907

Are you sure you want to change the base?

gh-99813: Start using SSL_sendfile when available #99907

Conversation

illia-v commented Nov 30, 2022 • edited Loading

illia-v commented Mar 13, 2023

illia-v commented Jul 26, 2023

gpshead left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-v commented Apr 19, 2025

picnixz left a comment • edited Loading

Choose a reason for hiding this comment

picnixz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroIntensity May 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

picnixz May 1, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gh-99813: Start using `SSL_sendfile` when available #99907

gh-99813: Start using `SSL_sendfile` when available #99907

illia-v commented Nov 30, 2022 •

edited

Loading

picnixz left a comment •

edited

Loading

ZeroIntensity May 1, 2025 •

edited

Loading

picnixz May 1, 2025 •

edited

Loading