classification
Title: SSLSocket read/write thread-unsafety
Type: behavior Stage:
Components: SSL Versions: Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Alexey Baldin, christian.heimes, steve.dower
Priority: normal Keywords:

Created on 2018-01-11 13:41 by Alexey Baldin, last changed 2018-01-12 06:05 by Alexey Baldin.

Messages (3)
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.
History
Date User Action Args
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