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

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.

Modules/_ssl.c

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!

It started with returning Py_False without the wrapper as it looked like the most clean option, than I was asked to use Py_NewRef and Py_RETURN_FALSE.
"There should be one-- and preferably only one --obvious way to do it." is not applicable to returning booleans in CPython's C code definitely 😄

Modules/_ssl.c

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

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.

Modules/_ssl.c

Doc/library/ssl.rst

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

illia-v · 2025-05-02T14:35:09Z

@ZeroIntensity @picnixz thanks for the suggestion! I applied them

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

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

illia-v and others added 6 commits May 2, 2025 17:05

Mark new functions with critical_section

6a35ac6

Merge two if blocks

9dffdbd

Reword docs

19d5746

Update the news entry

ef3744e

Co-authored-by: Peter Bierma <zintensitydev@gmail.com>

Use Py_RETURN_FALSE again

e64f329

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

028067a

illia-v requested review from ZeroIntensity and picnixz May 2, 2025 14:35

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 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

ZeroIntensity May 1, 2025

picnixz May 1, 2025

ZeroIntensity May 1, 2025 •

edited

Loading

picnixz May 2, 2025

illia-v May 2, 2025

ZeroIntensity May 1, 2025

illia-v commented May 2, 2025 •

edited

Loading

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

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

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

illia-v commented May 2, 2025 • edited Loading

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

illia-v commented May 2, 2025 •

edited

Loading