Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

gh-127257: ssl: Raise OSError for ERR_LIB_SYS #127361

Merged
merged 2 commits into from
Dec 10, 2024
Merged

gh-127257: ssl: Raise OSError for ERR_LIB_SYS #127361

merged 2 commits into from
Dec 10, 2024
+12 −0

Conversation

encukou
Copy link
Member

@encukou encukou commented Nov 28, 2024 •
edited by bedevere-app bot
Loading

One way OpenSSL can report a failed syscall is SSL_ERROR_SYSCALL. We have that one covered.

But, another way is ERR_LIB_SYS. Apparently, it's being used more now, and causing issues on OpenSSL 3.4.0 on Arch buildbots.

From the ERR_raise manpage:

ERR_LIB_SYS

    This "library code" indicates that a system error is
    being reported.  In this case, the reason code given
    to `ERR_raise()` and `ERR_raise_data()` *must* be
    `errno(3)`.

This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.

I believe it's correct to raise OSError rather that SSLError
in these cases. That makes this a backwards-incompatible change
(but one we still might want to backport as a bugfix).

The error on Arch with OpenSSL 3.4.0 is broken pipe (EPIPE) when a client reads after the server shuts down. On my Arch, raising OSError makes test_poplip pass.

@encukou
ssl: Raise OSError for ERR_LIB_SYS
11e0523 
From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.
@encukou encukou requested a review from gpshead November 28, 2024 13:19
@bedevere-app bedevere-app bot mentioned this pull request Nov 28, 2024
Closed
@encukou
Copy link
Member Author

encukou commented Nov 28, 2024

!buildbot Arch.Linux

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @encukou for commit 0098897 🤖

The command will test the builders whose names match following regular expression: Arch.Linux

The builders matched are:

  • AMD64 Arch Linux Perf PR
  • AMD64 Arch Linux Asan PR
  • AMD64 Arch Linux VintageParser PR
  • AMD64 Arch Linux Valgrind PR
  • AMD64 Arch Linux Usan PR
  • AMD64 Arch Linux Usan Function PR
  • AMD64 Arch Linux Asan Debug PR
  • AMD64 Arch Linux TraceRefs PR

@petermarko
Copy link

This fixes #125936 for me.
Thanks!

halstead pushed a commit to yoctoproject/poky that referenced this pull request Nov 29, 2024
@petermarko @rpurdie
python: backport patches to support openssl 3.4.0
7b1afc5 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e271e9cbf896f1fb97d56c426e4217a6d2105ea4)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
halstead pushed a commit to openembedded/openembedded-core that referenced this pull request Nov 29, 2024
@petermarko @rpurdie
python: backport patches to support openssl 3.4.0
e271e9c 
python/cpython#127331
python/cpython#127361

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
halstead pushed a commit to yoctoproject/poky that referenced this pull request Nov 29, 2024
@petermarko @rpurdie
python: backport patches to support openssl 3.4.0
cdea008 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e5f3a1793e34fb4cd1e53ca60b67f9a9f084b7a6)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
halstead pushed a commit to openembedded/openembedded-core that referenced this pull request Nov 29, 2024
@petermarko @rpurdie
python: backport patches to support openssl 3.4.0
e5f3a17 
python/cpython#127331
python/cpython#127361

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
daregit pushed a commit to daregit/yocto-combined that referenced this pull request Nov 29, 2024
@petermarko @rpurdie
src/poky:python: backport patches to support openssl 3.4.0
204b22b 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e5f3a1793e34fb4cd1e53ca60b67f9a9f084b7a6)

Signed-off-by: Peter Marko <peter.markosiemens.com>
Signed-off-by: Richard Purdie <richard.purdielinuxfoundation.org>
daregit pushed a commit to daregit/yocto-combined that referenced this pull request Dec 1, 2024
@petermarko @rpurdie
src/poky:python: backport patches to support openssl 3.4.0
7b9baee 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e5f3a1793e34fb4cd1e53ca60b67f9a9f084b7a6)

Signed-off-by: Peter Marko <peter.markosiemens.com>
Signed-off-by: Richard Purdie <richard.purdielinuxfoundation.org>
@encukou
Copy link
Member Author

encukou commented Dec 4, 2024

ssl experts (@jackjansen, @tiran, @dstufft, @alex): Do you want to check the change?
I think this is the correct place, and the correct error to raise, but I'd appreciate someone with domain knowledge.

@encukou
Copy link
Member Author

encukou commented Dec 9, 2024

If there are no objections, I'll merge tomorrow to unblock the buildbots.

vstinner
vstinner approved these changes Dec 9, 2024
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me. I don't know how to test it in a reliable way. Let's see how it goes with EPIPE and OpenSSL 3.4.

@encukou encukou merged commit f4b31ed into python:main Dec 10, 2024
50 of 53 checks passed
@encukou encukou deleted the ssl-sys-error branch December 10, 2024 10:56
@encukou encukou added the needs backport to 3.13 bugs and security fixes label Dec 11, 2024
@miss-islington-app
Copy link

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 11, 2024
@encukou @miss-islington
pythongh-127257: ssl: Raise OSError for ERR_LIB_SYS (pythonGH-127361)
d7a56c9 
From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.

This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.
(cherry picked from commit f4b31ed)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Dec 11, 2024

GH-127812 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Dec 11, 2024
encukou added a commit that referenced this pull request Dec 12, 2024
@miss-islington @encukou
[3.13] gh-127257: ssl: Raise OSError for ERR_LIB_SYS (GH-127361) (GH-…
2a78663 
…127812)

gh-127257: ssl: Raise OSError for ERR_LIB_SYS (GH-127361)

From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.

This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.
(cherry picked from commit f4b31ed)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@encukou encukou added the needs backport to 3.12 bug and security fixes label Dec 12, 2024
@miss-islington-app
Copy link

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@petermarko petermarko mentioned this pull request Dec 12, 2024
Closed
@encukou encukou added needs backport to 3.12 bug and security fixes and removed needs backport to 3.12 bug and security fixes labels Dec 13, 2024
@miss-islington-app
Copy link

Thanks @encukou for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 13, 2024
@encukou @miss-islington
pythongh-127257: ssl: Raise OSError for ERR_LIB_SYS (pythonGH-127361)
c646249 
From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.

This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.
(cherry picked from commit f4b31ed)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Dec 13, 2024

GH-127905 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Dec 13, 2024
encukou added a commit that referenced this pull request Dec 16, 2024
@miss-islington @encukou
[3.12] gh-127257: ssl: Raise OSError for ERR_LIB_SYS (GH-127361) (GH-…
7f707fa 
…127905)

gh-127257: ssl: Raise OSError for ERR_LIB_SYS (GH-127361)

From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.

This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.
(cherry picked from commit f4b31ed)

Co-authored-by: Petr Viktorin <encukou@gmail.com>
leimaohui pushed a commit to ubinux/yocto-ubinux that referenced this pull request Dec 25, 2024
@petermarko @leimaohui
python: backport patches to support openssl 3.4.0
e856ce7 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e5f3a1793e34fb4cd1e53ca60b67f9a9f084b7a6)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
leimaohui pushed a commit to ubinux/yocto-ubinux that referenced this pull request Dec 25, 2024
@petermarko @leimaohui
python: backport patches to support openssl 3.4.0
05affd7 
python/cpython#127331
python/cpython#127361

(From OE-Core rev: e5f3a1793e34fb4cd1e53ca60b67f9a9f084b7a6)

Signed-off-by: Peter Marko <peter.marko@siemens.com>
Signed-off-by: Richard Purdie <richard.purdie@linuxfoundation.org>
srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025
@encukou @srinivasreddy
pythongh-127257: ssl: Raise OSError for ERR_LIB_SYS (pythonGH-127361)
98b960b 
From the ERR_raise manpage:

    ERR_LIB_SYS

        This "library code" indicates that a system error is
        being reported.  In this case, the reason code given
        to `ERR_raise()` and `ERR_raise_data()` *must* be
        `errno(3)`.


This PR only handles ERR_LIB_SYS for the high-lever error types
SSL_ERROR_SYSCALL and SSL_ERROR_SSL, i.e., not the ones where
OpenSSL indicates it has some more information about the issue.
cjwatson added a commit to cjwatson/urllib3 that referenced this pull request Jan 9, 2025
@cjwatson
SingleTLSLayerTestCase: Catch BrokenPipeError
5161c39 
OpenSSL 3.4.0 returns `ERR_LIB_SYS` in some more situations than it used
to.  In the case exercised by
`SingleTLSLayerTestCase.test_close_after_handshake`,
python/cpython#127361 (also backported to the
3.12 and 3.13 branches) turns this into `BrokenPipeError`.  It seems
reasonable to treat this in the same way as `ConnectionAbortedError` and
`ConnectionResetError`.
@cjwatson cjwatson mentioned this pull request Jan 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@gpshead gpshead Awaiting requested review from gpshead

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@encukou @bedevere-bot @petermarko @vstinner