[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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

--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Ned Deily]

Ned Deily  added the comment:

This issue can now be closed, right?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[miss-islington]

miss-islington  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[miss-islington]

miss-islington  added the comment:


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


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Christian Heimes]

Change by Christian Heimes :


--
pull_requests: +8902

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[miss-islington]

miss-islington  added the comment:


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


--
nosy: +miss-islington

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[miss-islington]

Change by miss-islington :


--
pull_requests: +8901

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Christian Heimes]

Christian Heimes  added the comment:

Good catch, Nathaniel.

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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

--
keywords: +3.6regression
versions: +Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Change by Nathaniel Smith :


--
keywords: +patch
pull_requests: +8881
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Ned Deily]

Ned Deily  added the comment:

Steve?  Christian?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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

--
priority: normal -> release blocker
Added file: https://bugs.python.org/file47817/ssl-unwrap-regression-example.py

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Ned Deily]

Ned Deily  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Steve Dower]

Steve Dower  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

Git bisect says:


1229664f30dd5fd4da32174a19258f8312464d45 is the first bad commit
commit 1229664f30dd5fd4da32174a19258f8312464d45
Author: Miss Islington (bot) <31488909+miss-isling...@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 


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

--
nosy: +steve.dower

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

Nathaniel Smith  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue34759] Possible regression in ssl module in 3.7.1 and master

[Nathaniel Smith]

New submission from Nathaniel Smith :

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.

--
assignee: christian.heimes
components: SSL
keywords: 3.7regression
messages: 325940
nosy: christian.heimes, ned.deily, njs
priority: normal
severity: normal
status: open
title: Possible regression in ssl module in 3.7.1 and master
versions: Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com