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.

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

Git bisect says:


1229664f30dd5fd4da32174a19258f8312464d45 is the first bad commit
commit 1229664f30dd5fd4da32174a19258f8312464d45
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"...

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.)

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.

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".

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 :-)

Steve?  Christian?

Oh, here it is:

https://github.com/python/cpython/commit/1229664f30dd5fd4da32174a19258f8312464d45#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.

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

Good catch, Nathaniel.

I was about to ask for a reproducer test, but then I saw that you have added one already. Fantastic!

New changeset c0da582b227f311126e278b5553a7fa89c79b054 by Miss Islington (bot) (Nathaniel J. Smith) in branch 'master':
bpo-34759: Fix error handling in ssl 'unwrap()' (GH-9468)
https://github.com/python/cpython/commit/c0da582b227f311126e278b5553a7fa89c79b054

New changeset c00f7037df3607c89323e68db3ab996b7df394de by Miss Islington (bot) in branch '3.7':
bpo-34759: Fix error handling in ssl 'unwrap()' (GH-9468)
https://github.com/python/cpython/commit/c00f7037df3607c89323e68db3ab996b7df394de

New changeset 7529754d26f5e744ae25bee56fdc1937bcf08c7e by Miss Islington (bot) (Christian Heimes) in branch '3.6':
[3.6] bpo-34759: Fix error handling in ssl 'unwrap()' (GH-9468) (GH-9492)
https://github.com/python/cpython/commit/7529754d26f5e744ae25bee56fdc1937bcf08c7e

This issue can now be closed, right?

Whoops, yep, I forgot it doesn't auto-close.