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.

I think there was no clear consensus but I think it's better to use the macro when possible and use Py_NewRef otherwise, even if the macro does not use Py_NewRef. IIRC, the discussion involved Irit and Serhiy so if you can find it and see what we eventually decided, it would be great! (I don't have the time nor the resources for that). And if I incorrectly remembered the consensus then I apologise in advance!

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.

Oh right! let's do it your way

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

picnixz May 2, 2025

ZeroIntensity May 1, 2025

ZeroIntensity May 1, 2025

picnixz May 1, 2025 •

edited

Loading

ZeroIntensity May 1, 2025

picnixz May 2, 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

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

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