gh-99813: Start using `SSL_sendfile` when available by illia-v · Pull Request #99907 · python/cpython

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

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


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

picnixz · 2025-05-03T08:44:13Z

I'm very sorry but I'm travelling and I'm not back before the beta release. Since features are only accepted until then, it will need to wait for 3.15 unless @gpshead reviews, accepts and merges this.

ZeroIntensity

I forgot to approve this, LGTM as well.

illia-v · 2025-05-19T14:05:54Z

@picnixz is it possible to merge it for 3.15 now?

picnixz · 2025-05-19T14:29:34Z

For me it's ok but @gpshead hasn't re-reviewed it yet so I'm waiting.

Note: 3.15 has many alphas so it will anyway be merged IMO.

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

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

Closed

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

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

ZeroIntensity approved these changes May 3, 2025

View reviewed changes

illia-v added 2 commits May 5, 2025 16:35

Merge branch 'main' into SSL_sendfile

6e099fa

Merge branch 'main' into SSL_sendfile

f363ec3

picnixz approved these changes May 19, 2025

View reviewed changes

Merge branch 'main' into SSL_sendfile

7c071fd

picnixz enabled auto-merge (squash) July 12, 2025 12:17

picnixz merged commit 5a20e79 into python:main Jul 12, 2025
41 checks passed

bedevere-app bot removed the awaiting merge label Jul 12, 2025

picnixz mentioned this pull request Dec 8, 2025

_sendfile_zerocopy leaks the file selector #142403

Open

Uh oh!

Conversation

illia-v commented Nov 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

illia-v commented Mar 13, 2023

Uh oh!

illia-v commented Jul 26, 2023

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

picnixz Apr 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity May 1, 2025

Choose a reason for hiding this comment

Uh oh!

picnixz May 1, 2025

Choose a reason for hiding this comment

Uh oh!

ZeroIntensity May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz May 2, 2025

Choose a reason for hiding this comment

Uh oh!

illia-v May 2, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZeroIntensity May 1, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

illia-v commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz commented May 3, 2025

Uh oh!

ZeroIntensity left a comment

Choose a reason for hiding this comment

Uh oh!

illia-v commented May 19, 2025

Uh oh!

picnixz commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

illia-v commented Nov 30, 2022 •

edited

Loading

ZeroIntensity May 1, 2025 •

edited

Loading

illia-v commented May 2, 2025 •

edited

Loading

picnixz commented May 19, 2025 •

edited

Loading