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

SSLSocket read/write thread-unsafety #76714

Closed
AlexeyBaldin mannequin opened this issue Jan 11, 2018 · 9 comments
Closed

SSLSocket read/write thread-unsafety #76714

AlexeyBaldin mannequin opened this issue Jan 11, 2018 · 9 comments
Assignees
@zooba
Labels
3.7 (EOL) end of life 3.8 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@AlexeyBaldin
Copy link
Mannequin

AlexeyBaldin mannequin commented Jan 11, 2018

BPO 32533
Nosy @tiran, @zooba, @glolol, @miss-islington
PRs
  • bpo-32533: Fixed thread-safety of error handling in _ssl. #7158
  • [3.7] bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158) #9363
  • [3.6] bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158) #9365
    		•

    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/zooba'
closed_at = <Date 2018-09-21.20:47:55.666>
created_at = <Date 2018-01-11.13:41:08.578>
labels = ['expert-SSL', '3.8', 'type-bug', '3.7']
title = 'SSLSocket read/write thread-unsafety'
updated_at = <Date 2018-09-21.20:47:55.664>
user = 'https://bugs.python.org/AlexeyBaldin'

    bugs.python.org fields:

    activity = <Date 2018-09-21.20:47:55.664>
actor = 'steve.dower'
assignee = 'steve.dower'
closed = True
closed_date = <Date 2018-09-21.20:47:55.666>
closer = 'steve.dower'
components = ['SSL']
creation = <Date 2018-01-11.13:41:08.578>
creator = 'Alexey Baldin'
dependencies = []
files = []
hgrepos = []
issue_num = 32533
keywords = ['patch']
message_count = 9.0
messages = ['309805', '309832', '309838', '315919', '315921', '325563', '325565', '325582', '326038']
nosy_count = 6.0
nosy_names = ['christian.heimes', 'steve.dower', 'devkid', 'tacocat', 'Alexey Baldin', 'miss-islington']
pr_nums = ['7158', '9363', '9365']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32533'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']
    @AlexeyBaldin
    Copy link
    Mannequin Author

    AlexeyBaldin mannequin commented Jan 11, 2018

    _ssl.c has thread-usafe code in implementation of read, write and other methods. E.g. 'write' method:

    2099 PySSL_BEGIN_ALLOW_THREADS
    2100 len = SSL_write(self->ssl, b->buf, (int)b->len);
    2101 _PySSL_UPDATE_ERRNO_IF(len <= 0, self, len);
    2102 PySSL_END_ALLOW_THREADS
    2103 err = self->ssl_errno;

    _PySSL_UPDATE_ERRNO_IF updates self->ssl_errno without lock. Similar code used in 'read' method. Later self->ssl_errno is used for decision on retrying the operation. As result, SSL_write can be incorrectly repeated because ssl_errno was updated by 'read' method to SSL_ERROR_WANT_READ from another thread.

    Looks like commit e6eb48c is the cause.

    @AlexeyBaldin AlexeyBaldin mannequin assigned tiran Jan 11, 2018
    @AlexeyBaldin AlexeyBaldin mannequin added topic-SSL type-bug An unexpected behavior, bug, or error labels Jan 11, 2018
    @zooba
    Copy link
    Member

    zooba commented Jan 11, 2018

    Almost seems like an unwinnable race here. We need to collect the error numbers before reacquiring the GIL (which could change some of them), but then if we update the object after getting the lock back we could still have raced with another thread.

    Perhaps we need to make more dramatic changes to not store error codes against the object?

    @zooba zooba added the 3.7 (EOL) end of life label Jan 11, 2018
    @AlexeyBaldin
    Copy link
    Mannequin Author

    AlexeyBaldin mannequin commented Jan 12, 2018

    I'd gather errno and win error into local variables (or struct) just after SSL call and then pass them to PySSL_SetError.

    @tiran tiran added the 3.8 only security fixes label Feb 26, 2018
    @tiran tiran assigned zooba and unassigned tiran Feb 26, 2018
    @devkid
    Copy link
    Mannequin

    devkid mannequin commented Apr 29, 2018

    Is there anything on the roadmap to fix this? This is a pretty severe bug given that this breaks multi-threaded OpenSSL while the documentation says it's thread-safe.

    @zooba
    Copy link
    Member

    zooba commented Apr 29, 2018

    We don't have a roadmap, just volunteers. When someone is sufficiently motivated to work on it, it will happen. (You're welcome to try and motivate people with reason, pleading, money and/or abuse, though I don't recommend the last one :) Money doesn't work either on many of us who are really just time-poor.)

    @zooba
    Copy link
    Member

    zooba commented Sep 17, 2018

    New changeset c6fd1c1 by Steve Dower in branch 'master':
    bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
    c6fd1c1

    @miss-islington
    Copy link
    Contributor

    New changeset 1229664 by Miss Islington (bot) in branch '3.7':
    bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
    1229664

    @zooba
    Copy link
    Member

    zooba commented Sep 17, 2018

    New changeset 1a107ee by Steve Dower in branch '3.6':
    bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
    1a107ee

    @zooba
    Copy link
    Member

    zooba commented Sep 21, 2018

    Fixed, with a fix for the regression coming in bpo-34759

    @zooba zooba closed this as completed Sep 21, 2018
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @zooba zooba

    Labels
    3.7 (EOL) end of life 3.8 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @tiran @zooba @miss-islington