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.

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

OpenSSL 1.1.1: use SSL_write_ex() and SSL_read_ex() #87020

Closed
tiran opened this issue Jan 7, 2021 · 17 comments
Closed

OpenSSL 1.1.1: use SSL_write_ex() and SSL_read_ex() #87020

tiran opened this issue Jan 7, 2021 · 17 comments
Assignees
@tiran
Labels
3.10 only security fixes easy topic-SSL type-feature A feature request or enhancement

Comments

@tiran
Copy link
Member

tiran commented Jan 7, 2021 •
edited by bedevere-app bot

BPO 42854
Nosy @tiran, @ethanfurman, @pablogsal, @miss-islington, @FFY00, @geofft
PRs
  • bpo-42854: Use SSL_read/write_ex() (GH-25468) #25468
  • bpo-42854: Correctly use size_t for _ssl._SSLSocket.read and _ssl._SSLSocket.write #27271
  • [3.10] bpo-42854: Correctly use size_t for _ssl._SSLSocket.read and _ssl._SSLSocket.write (GH-27271) #27308
    		•

    Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/tiran'
closed_at = <Date 2021-07-23.15:26:03.224>
created_at = <Date 2021-01-07.08:23:09.806>
labels = ['easy', 'expert-SSL', 'type-feature', '3.10']
title = 'OpenSSL 1.1.1: use SSL_write_ex() and SSL_read_ex()'
updated_at = <Date 2021-07-23.15:26:03.937>
user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-07-23.15:26:03.937>
actor = 'pablogsal'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2021-07-23.15:26:03.224>
closer = 'pablogsal'
components = ['SSL']
creation = <Date 2021-01-07.08:23:09.806>
creator = 'christian.heimes'
dependencies = []
files = []
hgrepos = []
issue_num = 42854
keywords = ['patch', 'easy (C)']
message_count = 13.0
messages = ['384567', '384568', '391295', '391358', '391363', '391422', '391521', '397822', '397832', '397833', '397906', '398063', '398068']
nosy_count = 6.0
nosy_names = ['christian.heimes', 'ethan.furman', 'pablogsal', 'miss-islington', 'FFY00', 'geofft']
pr_nums = ['25468', '27271', '27308']
priority = 'high'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue42854'
versions = ['Python 3.10']

    Linked PRs
    @tiran
    Copy link
    Member Author

    tiran commented Jan 7, 2021

    SSL_read() and SSL_write() are limited to int. The new SSL_write_ex() and SSL_read_ex() APIs support size_t just like read(2) and recv(2). Also see bpo-42853.

    int SSL_write_ex(SSL *s, const void *buf, size_t num, size_t *written);
int SSL_read_ex(SSL *ssl, void *buf, size_t num, size_t *readbytes);

    Both functions return 1 for success or 0 for failure.

    @tiran tiran added the 3.10 only security fixes label Jan 7, 2021
    @tiran tiran self-assigned this Jan 7, 2021
    @tiran tiran added topic-SSL type-feature A feature request or enhancement labels Jan 7, 2021
    @tiran
    Copy link
    Member Author

    tiran commented Jan 7, 2021

    As of version 3.3.1, LibreSSL does not have SSL_write_ex() and SSL_read_ex(). The read and write functions are limited to int.

    @tiran
    Copy link
    Member Author

    tiran commented Apr 17, 2021

    3.10 branch now requires OpenSSL 1.1.1. This should be easy to implement.

    @tiran tiran added the easy label Apr 17, 2021
    @tiran
    Copy link
    Member Author

    tiran commented Apr 19, 2021

    SSL_write_ex() and SSL_read_ex() solve two issues:

    • bpo-42853: SSLSocket no longer raises overflow error when sending or receiving more than 2 GB of data
    • bpo-31711: empty send(b"") no longer fails with protocol violation exception

    @tiran
    Copy link
    Member Author

    tiran commented Apr 19, 2021

    New changeset 89d1550 by Christian Heimes in branch 'master':
    bpo-42854: Use SSL_read/write_ex() (GH-25468)
    89d1550

    @tiran tiran closed this as completed Apr 19, 2021
    @tiran
    Copy link
    Member Author

    tiran commented Apr 20, 2021

    Ethan, what's your platform and OpenSSL version?

    @tiran tiran reopened this Apr 20, 2021
    @ethanfurman
    Copy link
    Member

    False alarm, sorry. Still getting used to merging, rebasing, etc.

    Current tests run fine.

    @geofft
    Copy link
    Mannequin

    geofft mannequin commented Jul 19, 2021

    I am still seeing failures to read responses over 2 GB in Python 3.10b1. I'm working on a reproducer, but I'm getting the same "OverflowError: signed integer is greater than maximum" that I get in 3.9.

    @geofft
    Copy link
    Mannequin

    geofft mannequin commented Jul 19, 2021

    Christian mentioned on Twitter that this is probably due to a missing argument clinic change from "int" to "Py_ssize_t". I can confirm that fixing that and rerunning argument clinic makes things start to work.

    I don't have the ability to reopen this bug (I think), can someone reopen it please?

    @FFY00
    Copy link
    Member

    FFY00 commented Jul 19, 2021

    https://twitter.com/geofft/status/1417167982665551877

    @FFY00 FFY00 reopened this Jul 19, 2021
    @pablogsal
    Copy link
    Member

    I will push a fix today

    @pablogsal
    Copy link
    Member

    New changeset 83d1430 by Pablo Galindo Salgado in branch 'main':
    bpo-42854: Correctly use size_t for _ssl._SSLSocket.read and _ssl._SSLSocket.write (GH-27271)
    83d1430

    @pablogsal
    Copy link
    Member

    New changeset 5ec2757 by Miss Islington (bot) in branch '3.10':
    bpo-42854: Correctly use size_t for _ssl._SSLSocket.read and _ssl._SSLSocket.write (GH-27271) (GH-27308)
    5ec2757

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @illia-v illia-v mentioned this issue Jun 10, 2022
    Closed
    3 tasks
    @erlend-aasland
    Copy link
    Contributor

    Two reverts were opened after this issue was resolved:

    I could not find any discussion regarding these reverts, so it is not immediately obvious if they are still needed. @pablogsal, your review was requested for both of these reverts; do you remember the discussion?

    This was referenced Jan 4, 2024
    Closed
    Closed
    @pablogsal
    Copy link
    Member

    I could not find any discussion regarding these reverts, so it is not immediately obvious if they are still needed. @pablogsal, your review was requested for both of these reverts; do you remember the discussion?

    I remember my commit but unfortunately I don't remember the discussion for the reverts :(

    @erlend-aasland
    Copy link
    Contributor

    Well, given that the reverts were not applied for 1,5 years, perhaps we could close those PRs. It would seem to me that there would exist an issue for this if these were release blockers or if they represented critical bugs.

    @erlend-aasland
    Copy link
    Contributor

    I closed the revert PRs. We can open them later if needed.

    @jakirkham jakirkham mentioned this issue Feb 16, 2024
    Merged
    @keepworking keepworking mentioned this issue Feb 18, 2024
    Open
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @tiran tiran

    Labels
    3.10 only security fixes easy topic-SSL type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    5 participants
    @tiran @ethanfurman @pablogsal @FFY00 @erlend-aasland