bpo-22559: Implementation of the PEP 546: Backport MemoryBIO#2133

vstinner

Implementation of the PEP 546: backport ssl.MemoryBIO and ssl.SSLObject from master to the 2.7 branch.

Co-Authored-By: Alex Gaynor alex.gaynor@gmail.com

http://bugs.python.org/issue22559

https://bugs.python.org/issue22559

vstinner

cc @Lukasa

alex

It's been a while since I looked at this patch, but from an initial glance it looks like what I expected.

alex

Tests appear to be hanging; these suck to debug. Usually there's a hidden exception coming from one of the non-main threads.

Implementation of the PEP 546: backport ssl.MemoryBIO and ssl.SSLObject from master to the 2.7 branch. Co-Authored-By: Alex Gaynor <alex.gaynor@gmail.com>

vstinner

I rebased my change and addressed @alex and my comments. I removed [WIP] from the PR title, it's now ready for a real review :-)

alex

Tests still appear to be hanging, so that's going to be a blocker, I'll review for everything else.

alex

Patch looks fine (and largely the same as my original one!). If you'd like help tracking down the hang in test_ssl, let me know.

alex

Sigh, it took quite a while, but the answer is that svn.python.org doesn't behave the exact way these tests expect. There's a reason we ported all the rest of the tests away from it :-(

Switching to REMOTE_HOST fixes test_read_write_data, but it looks like test_handshake is really only happy with the in-memoryserver that the py3k versions of these tests use.

Here's the diff I used to do all this testing:

diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py
index 4e34734662..c99db781c1 100644
--- a/Lib/test/test_ssl.py
+++ b/Lib/test/test_ssl.py
@@ -1757,17 +1757,17 @@ class NetworkedBIOTests(unittest.TestCase):
         return ret
 
     def test_handshake(self):
-        with support.transient_internet("svn.python.org"):
+        with support.transient_internet(REMOTE_HOST):
             sock = socket.socket(socket.AF_INET)
-            sock.connect(("svn.python.org", 443))
+            sock.connect((REMOTE_HOST, 443))
             incoming = ssl.MemoryBIO()
             outgoing = ssl.MemoryBIO()
             ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
             ctx.verify_mode = ssl.CERT_REQUIRED
-            ctx.load_verify_locations(SVN_PYTHON_ORG_ROOT_CERT)
+            ctx.load_verify_locations(REMOTE_ROOT_CERT)
             if ssl.HAS_SNI:
                 ctx.check_hostname = True
-                sslobj = ctx.wrap_bio(incoming, outgoing, False, 'svn.python.org')
+                sslobj = ctx.wrap_bio(incoming, outgoing, False, REMOTE_HOST)
             else:
                 ctx.check_hostname = False
                 sslobj = ctx.wrap_bio(incoming, outgoing, False)
@@ -1786,9 +1786,9 @@ class NetworkedBIOTests(unittest.TestCase):
             sock.close()
 
     def test_read_write_data(self):
-        with support.transient_internet("svn.python.org"):
+        with support.transient_internet(REMOTE_HOST):
             sock = socket.socket(socket.AF_INET)
-            sock.connect(("svn.python.org", 443))
+            sock.connect((REMOTE_HOST, 443))
             incoming = ssl.MemoryBIO()
             outgoing = ssl.MemoryBIO()
             ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)

Patch written by Alex Gaynor.

vstinner

@alex: I'm unable to reproduce the failure on my Fedora 25, so I tried your patch to see if it's going better on the CI.

alex

FWIW, I had no problem reproducing on macOS. What version of OpenSSL does Fedora 25 use?

vstinner

Fedora 25 uses OpenSSL 1.0.2k.

tiran

You have to use BIO_up_ref() instead of CRYPTO_add. 3.6 has:

diff --git a/Modules/_ssl.c b/Modules/_ssl.c
index 8202fd0262..defb50ed4a 100644
--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -171,6 +171,12 @@ static X509 *X509_OBJECT_get0_X509(X509_OBJECT *x)
     return x->data.x509;
 }
 
+static int BIO_up_ref(BIO *b)
+{
+    CRYPTO_add(&b->references, 1, CRYPTO_LOCK_BIO);
+    return 1;
+}
+
 static STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(X509_STORE *store) {
     return store->objs;
 }
@@ -599,8 +605,8 @@ newPySSLSocket(PySSLContext *sslctx, PySocketSockObject *sock,
         /* BIOs are reference counted and SSL_set_bio borrows our reference.
          * To prevent a double free in memory_bio_dealloc() we need to take an
          * extra reference here. */
-        CRYPTO_add(&inbio->bio->references, 1, CRYPTO_LOCK_BIO);
-        CRYPTO_add(&outbio->bio->references, 1, CRYPTO_LOCK_BIO);
+        BIO_up_ref(inbio->bio);
+        BIO_up_ref(outbio->bio);
         SSL_set_bio(self->ssl, inbio->bio, outbio->bio);
     }
     mode = SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER;

vstinner

Sadly, MemoryBIO will have to wait for Python 2.7.15 which has no schedule yet :-(

tiran

The backport has a bug. sslobj.unwrap call in NetworkedBIOTests.test_handshake blocks indefinitely. The test is only executed with -u network resource enabled..

vstinner

./python -m test -v -u all -m test_handshake test_ssl

This command loops on the "SSLSyscallError: Some I/O error occurred (_ssl.c:2097)" error while running SSLObject.unwrap(): "self.ssl_io_loop(sock, incoming, outgoing, sslobj.unwrap)".

vstinner requested a review from alex June 12, 2017 15:56

the-knights-who-say-ni added the CLA signed label Jun 12, 2017

vstinner commented Jun 12, 2017

View reviewed changes

alex reviewed Jun 19, 2017

View reviewed changes

vstinner and others added 3 commits July 5, 2017 11:00

bpo-22559: Backport MemoryBIO …

2671297

Implementation of the PEP 546: backport ssl.MemoryBIO and ssl.SSLObject from master to the 2.7 branch. Co-Authored-By: Alex Gaynor <alex.gaynor@gmail.com>

Update ssl.py from master

df550ad

Address Alex Gaynor and my own comments

6c0eab9

vstinner changed the title ~~[WIP] bpo-22559: Backport MemoryBIO~~ Jul 5, 2017

alex reviewed Jul 5, 2017

View reviewed changes

vstinner added 3 commits July 21, 2017 02:22

Merge branch '2.7' into memorybio27

6332c11

test_ssl: Replace svn.python.org with REMOTE_HOST …

f9fe9a9

Patch written by Alex Gaynor.

Oops, remove duplicated documentation

95c40cc

vstinner changed the title ~~bpo-22559: Backport MemoryBIO~~ Aug 11, 2017

thezoggy mentioned this pull request Aug 13, 2017

SABnzbd 2.1.0 Beta 1 on macOS doesn't download any nzbs sabnzbd/sabnzbd#929

Closed

vstinner added 3 commits September 7, 2017 20:00

Merge branch '2.7' into memorybio27

1d71c25

Replace CRYPTO_add() with BIO_up_ref() for OpenSSL 1.1

980b3f3

Add NEWS entry.

d52ab10

vstinner closed this Feb 1, 2018

vstinner deleted the memorybio27 branch May 29, 2018 22:28

Conversation

vstinner commented Jun 12, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jun 12, 2017

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

alex commented Jun 20, 2017

Uh oh!

vstinner commented Jul 5, 2017

Uh oh!

alex commented Jul 5, 2017

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

alex commented Jul 20, 2017

Uh oh!

vstinner commented Jul 21, 2017

Uh oh!

alex commented Jul 21, 2017

Uh oh!

vstinner commented Jul 24, 2017

Uh oh!

tiran commented Sep 8, 2017

Uh oh!

vstinner commented Sep 8, 2017

Uh oh!

tiran commented Sep 8, 2017

Uh oh!

vstinner commented Sep 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vstinner commented Jun 12, 2017 •

edited by bedevere-bot

Loading