classification
Title: SSLSocket read/write thread-unsafety
Type: behavior Stage: resolved
Components: SSL Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: Alexey Baldin, christian.heimes, devkid, miss-islington, steve.dower, tacocat
Priority: normal Keywords: patch

Created on 2018-01-11 13:41 by Alexey Baldin, last changed 2018-09-21 20:47 by steve.dower. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 7158 merged steve.dower, 2018-05-28 15:57
PR 9363 merged miss-islington, 2018-09-17 18:35
PR 9365 merged steve.dower, 2018-09-17 20:18
Messages (9)
msg309805 - (view) Author: Alexey Baldin (Alexey Baldin) Date: 2018-01-11 13:41
_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 e6eb48c10dc389d1d70657593de6a6cb3087d3d1 is the cause.
msg309832 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-01-11 22:51
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?
msg309838 - (view) Author: Alexey Baldin (Alexey Baldin) Date: 2018-01-12 06:05
I'd gather errno and win error into local variables (or struct) just after SSL call and then pass them to PySSL_SetError.
msg315919 - (view) Author: Alfred Krohmer (devkid) Date: 2018-04-29 20:07
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.
msg315921 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-04-29 22:03
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.)
msg325563 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-09-17 18:34
New changeset c6fd1c1c3a65217958b68df3a4991e4f306e9b7d by Steve Dower in branch 'master':
bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
https://github.com/python/cpython/commit/c6fd1c1c3a65217958b68df3a4991e4f306e9b7d
msg325565 - (view) Author: miss-islington (miss-islington) Date: 2018-09-17 19:12
New changeset 1229664f30dd5fd4da32174a19258f8312464d45 by Miss Islington (bot) in branch '3.7':
bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
https://github.com/python/cpython/commit/1229664f30dd5fd4da32174a19258f8312464d45
msg325582 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-09-17 21:41
New changeset 1a107eea8e91e50c5c9025e945c78eb1aa9b874d by Steve Dower in branch '3.6':
bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)
https://github.com/python/cpython/commit/1a107eea8e91e50c5c9025e945c78eb1aa9b874d
msg326038 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2018-09-21 20:47
Fixed, with a fix for the regression coming in issue34759
History
Date User Action Args
2018-09-21 20:48:13steve.dowerlinkissue33091 superseder
2018-09-21 20:47:55steve.dowersetstatus: open -> closed
resolution: fixed
messages: + msg326038

stage: patch review -> resolved
2018-09-17 21:41:46steve.dowersetmessages: + msg325582
2018-09-17 20:18:38steve.dowersetpull_requests: + pull_request8789
2018-09-17 19:12:22miss-islingtonsetnosy: + miss-islington
messages: + msg325565
2018-09-17 18:35:05miss-islingtonsetpull_requests: + pull_request8787
2018-09-17 18:34:59steve.dowersetmessages: + msg325563
2018-05-28 15:57:12steve.dowersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request6794
2018-04-29 22:03:08steve.dowersetmessages: + msg315921
2018-04-29 20:07:45devkidsetnosy: + devkid
messages: + msg315919
2018-04-17 21:18:48tacocatsetnosy: + tacocat
2018-02-26 08:54:46christian.heimessetassignee: christian.heimes -> steve.dower
stage: needs patch
versions: + Python 3.8
2018-01-12 06:05:30Alexey Baldinsetmessages: + msg309838
2018-01-11 22:51:42steve.dowersetmessages: + msg309832
versions: + Python 3.7
2018-01-11 13:43:22christian.heimessetnosy: + steve.dower
2018-01-11 13:41:08Alexey Baldincreate