Possible regression in ssl module in 3.7.1 and master

[njsmith]

BPO	34759
Nosy	@tiran, @ned-deily, @njsmith, @zooba, @miss-islington
PRs	bpo-34759: Fix error handling in ssl 'unwrap()' #9468 [3.7] bpo-34759: Fix error handling in ssl 'unwrap()' (GH-9468) #9491 [3.6] bpo-34759: Fix error handling in ssl 'unwrap()' (GH-9468) #9492
Files	ssl-unwrap-regression-example.py

^{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 2018-09-25.05:43:42.529>
created_at = <Date 2018-09-21.02:05:23.154>
labels = ['expert-SSL', '3.7', '3.8', 'release-blocker']
title = 'Possible regression in ssl module in 3.7.1 and master'
updated_at = <Date 2018-09-25.05:43:42.527>
user = 'https://github.com/njsmith'

bugs.python.org fields:

activity = <Date 2018-09-25.05:43:42.527>
actor = 'njs'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2018-09-25.05:43:42.529>
closer = 'njs'
components = ['SSL']
creation = <Date 2018-09-21.02:05:23.154>
creator = 'njs'
dependencies = []
files = ['47817']
hgrepos = []
issue_num = 34759
keywords = ['patch', '3.6regression', '3.7regression']
message_count = 16.0
messages = ['325940', '325941', '325942', '325943', '325944', '325945', '325951', '325952', '325954', '325962', '325968', '326070', '326074', '326075', '326323', '326324']
nosy_count = 5.0
nosy_names = ['christian.heimes', 'ned.deily', 'njs', 'steve.dower', 'miss-islington']
pr_nums = ['9468', '9491', '9492']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue34759'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[njsmith]

Hey Ned, we just noticed that since a few days ago the trio testsuite is failing on 3.7-dev (but not 3.7.0), in a test checking an obscure feature in the ssl module: https://travis-ci.org/python-trio/trio/builds/431291929

And I just reproduced the issue locally with a build of the 3.7 branch HEAD, so somehow whatever is happening is definitely new in 3.7.1.

It could just be a bug in the test or something, I don't know because I haven't debugged it yet (I'll do that now). But with 3.7.1rc1 imminent I figured you'd rather know earlier rather than later.

[njsmith]

(And Christian, if you know of any risky-sounding recent changes in SSLObject.unwrap, lmk :-))

[njsmith]

Git bisect says:

1229664 is the first bad commit
commit 1229664
Author: Miss Islington (bot) <31488909+miss-islington@users.noreply.github.com>
Date: Mon Sep 17 12:12:13 2018 -0700

bpo-32533: Fixed thread-safety of error handling in _ssl. (GH-7158)

(cherry picked from commit c6fd1c1c3a65217958b68df3a4991e4f306e9b7d)

Co-authored-by: Steve Dower <steve.dower@microsoft.com>

Now let's see if I can narrow down exactly what the behavioral change is, besides "this random test started failing"...

[zooba]

It should just be gathering SSL error codes before acquiring the GIL, which could switch thread and then get the wrong code.

Of course, it's possible it is in the backport. How easily can you test against master? (I'm AFK, so can't easily do anything right now.)

[njsmith]

The test doesn't involve any threads, so it does seem strange that this PR changed its behavior.

I haven't checked against master carefully, but the original observation was that our Travis "3.7-dev" and "3.8-dev" tests started failing in the same way sometime in the last week-ish, so my tentative guess is that it it's the same for both.

[ned-deily]

Thanks for the heads-up! If you become convinced it's a post-3.7.0 regression that would affect 3.7.1, please change the priority to "release blocker".

[njsmith]

The ssl module's unwrap() method is intended to do a clean shutdown of TLS encryption on a socket (or memory BIO or whatever). The idea is that it sends a "close notify" frame (i.e., tells the peer that we're shutting down), and then it waits for the peer to send a "close notify" back, and then the TLS layer is cleanly shut down.

If you have a non-blocking socket or memory BIO, of course, it can't actually wait for the peer to send the "close notify" back. So in this case its semantics are: the first time you call it, it sends a "close notify", and then if it hasn't seen a "close notify", it raises SSLWantReadError to tell you that, and that you need to keep calling it.

In the current 3.7 branch, however, it never raises SSLWantReadError, so there's no way for calling code to figure out whether the other side has sent a "close notify". So in the current 3.7 branch, it's impossible to do a clean shutdown of the TLS layer on a non-blocking socket or memory BIO, and this is a regression from 3.7.0.

The attached script demonstrates this -- on 3.7.0 it completes successfully, but on the 3.7 branch it fails.

Impact: This is maybe not as bad as it sounds, because 'unwrap' is *very* rarely used? (Lots of protocols have STARTTLS, but not many have STOPTLS!) But I'm still going to mark it as a release blocker for now because:

• it is a genuine regression, that makes a rare-but-intentional-and-documented feature unusable

• while I haven't fully understood why the thread-safety patch causes this problem, the changes that patch makes to 'unwrap' are very similar to the changes it makes to much more important functions like 'do_handshake' and 'send' and 'recv', so I'm nervous that the underlying issue might affect those as well.

• I figure if Ned doesn't think it's a release blocker, he can always remove the tag :-)

[ned-deily]

Steve? Christian?

[njsmith]

Oh, here it is:

1229664#diff-e1cc5bf74055e388cda128367a814c8fR2587

• if (err < 0) {
  + if (err.ssl < 0) {

Before in this function, 'err' was the return code from SSL_shutdown, which is -1 if an error occurred (and you're supposed to look at the SSL error code) or 0/1 on success. Now 'err.ssl' is the actual SSL error code, which as it turns out is never negative.

[njsmith]

PR posted. Also seems to affect 3.6, so adding that to the tags.