But I think this exposes a more generic bug in test_ssl.py, which is that the server thread doesn't die when one of these failures occurs. It probably should. I'll make a patch -- but I don't have a system that this fails on, how will I test it?

Here's a patch which makes test_ssl a better player in the buildbots environment. I deep-ended on "try-except-else" clauses. Should I post this as an issue to the Tracker? Bill Index: Lib/ssl.py =================================================================== --- Lib/ssl.py (revision 57506) +++ Lib/ssl.py (working copy) @@ -100,12 +100,13 @@ # see if it's connected try: socket.getpeername(self) - # yes + except: + # no, no connection yet + self._sslobj = None + else: + # yes, create the SSL object self._sslobj = _ssl.sslwrap(self._sock, 0, keyfile, certfile, cert_reqs, ssl_version, ca_certs) - except: - # no - self._sslobj = None self.keyfile = keyfile self.certfile = certfile self.cert_reqs = cert_reqs Index: Lib/test/test_ssl.py =================================================================== --- Lib/test/test_ssl.py (revision 57506) +++ Lib/test/test_ssl.py (working copy) @@ -91,38 +91,66 @@ def testTLSecho (self): s1 = socket.socket() - s1.connect(('127.0.0.1', 10024)) - c1 = ssl.sslsocket(s1, ssl_version=ssl.PROTOCOL_TLSv1) - indata = "FOO\n" - c1.write(indata) - outdata = c1.read() - if outdata != indata.lower(): - sys.stderr.write("bad data <<%s>> received\n" % data) - c1.close() + try: + s1.connect(('127.0.0.1', 10024)) + except: + sys.stdout.write("connection failure:\n" + string.join( + traceback.format_exception(*sys.exc_info()))) + raise test_support.TestFailed("Can't connect to test server") + else: + try: + c1 = ssl.sslsocket(s1, ssl_version=ssl.PROTOCOL_TLSv1) + except: + sys.stdout.write("SSL handshake failure:\n" + string.join( + traceback.format_exception(*sys.exc_info()))) + raise test_support.TestFailed("Can't SSL-handshake with test server") + else: + if not c1: + raise test_support.TestFailed("Can't SSL-handshake with test server") + indata = "FOO\n" + c1.write(indata) + outdata = c1.read() + if outdata != indata.lower(): + raise test_support.TestFailed("bad data <<%s>> received; expected <<%s>>\n" % (data, indata.lower())) + c1.close() def testReadCert(self): s2 = socket.socket() - s2.connect(('127.0.0.1', 10024)) - c2 = ssl.sslsocket(s2, ssl_version=ssl.PROTOCOL_TLSv1, - cert_reqs=ssl.CERT_REQUIRED, ca_certs=CERTFILE) - cert = c2.getpeercert() - if not cert: - raise test_support.TestFailed("Can't get peer certificate.") - if not cert.has_key('subject'): - raise test_support.TestFailed( - "No subject field in certificate: %s." % - pprint.pformat(cert)) - if not (cert['subject'].has_key('organizationName')): - raise test_support.TestFailed( - "No 'organizationName' field in certificate subject: %s." % - pprint.pformat(cert)) - if (cert['subject']['organizationName'] != - "Python Software Foundation"): - raise test_support.TestFailed( - "Invalid 'organizationName' field in certificate subject; " - "should be 'Python Software Foundation'."); - c2.close() + try: + s2.connect(('127.0.0.1', 10024)) + except: + sys.stdout.write("connection failure:\n" + string.join( + traceback.format_exception(*sys.exc_info()))) + raise test_support.TestFailed("Can't connect to test server") + else: + try: + c2 = ssl.sslsocket(s2, ssl_version=ssl.PROTOCOL_TLSv1, + cert_reqs=ssl.CERT_REQUIRED, ca_certs=CERTFILE) + except: + sys.stdout.write("SSL handshake failure:\n" + string.join( + traceback.format_exception(*sys.exc_info()))) + raise test_support.TestFailed("Can't SSL-handshake with test server") + else: + if not c2: + raise test_support.TestFailed("Can't SSL-handshake with test server") + cert = c2.getpeercert() + if not cert: + raise test_support.TestFailed("Can't get peer certificate.") + if not cert.has_key('subject'): + raise test_support.TestFailed( + "No subject field in certificate: %s." % + pprint.pformat(cert)) + if not (cert['subject'].has_key('organizationName')): + raise test_support.TestFailed( + "No 'organizationName' field in certificate subject: %s." % + pprint.pformat(cert)) + if (cert['subject']['organizationName'] != + "Python Software Foundation"): + raise test_support.TestFailed( + "Invalid 'organizationName' field in certificate subject; " + "should be 'Python Software Foundation'."); + c2.close() class threadedEchoServer(threading.Thread): @@ -138,10 +166,22 @@ def run (self): self.running = True - sslconn = ssl.sslsocket(self.sock, server_side=True, - certfile=self.server.certificate, - ssl_version=self.server.protocol, - cert_reqs=self.server.certreqs) + try: + sslconn = ssl.sslsocket(self.sock, server_side=True, + certfile=self.server.certificate, + ssl_version=self.server.protocol, + cert_reqs=self.server.certreqs) + except: + # here, we want to stop the server, because this shouldn't + # happen in the context of our test case + sys.stdout.write("Test server failure:\n" + string.join( + traceback.format_exception(*sys.exc_info()))) + self.running = False + # normally, we'd just stop here, but for the test + # harness, we want to stop the server + self.server.stop() + return + while self.running: try: msg = sslconn.read() @@ -154,15 +194,18 @@ self.server.stop() self.running = False else: - # print "server:", msg.strip().lower() + sys.stdout.write("\nserver: %s\n" % msg.strip().lower()) sslconn.write(msg.lower()) except ssl.sslerror: - sys.stderr.write(string.join( + sys.stdout.write("Test server failure:\n" + string.join( traceback.format_exception(*sys.exc_info()))) sslconn.close() self.running = False + # normally, we'd just stop here, but for the test + # harness, we want to stop the server + self.server.stop() except: - sys.stderr.write(string.join( + sys.stdout.write(string.join( traceback.format_exception(*sys.exc_info()))) def __init__(self, port, certificate, ssl_version=None, @@ -192,21 +235,21 @@ while self.active: try: newconn, connaddr = self.sock.accept() - # sys.stderr.write('new connection from ' + str(connaddr)) + sys.stdout.write('\nserver: new connection from ' + str(connaddr) + '\n') handler = self.connectionHandler(self, newconn) handler.start() except socket.timeout: pass except KeyboardInterrupt: - self.active = False + self.stop() except: - sys.stderr.write(string.join( + sys.stdout.write("Test server failure:\n" + string.join( traceback.format_exception(*sys.exc_info()))) def stop (self): self.active = False + self.sock.close() - CERTFILE_CONFIG_TEMPLATE = """ # create RSA certs - Server

Some of the code sets the error string in this directly before returning NULL, and other pieces of the code call PySSL_SetError, which creates the error string. I think some of the places which set the string directly probably shouldn't; instead, they should call PySSL_SetError to cons up the error name directly from the err code. However, PySSL_SetError only works after the construction of an ssl object, which means it can't be used there... I'll take a longer look at it and see if there's a reasonable fix.

Here's a patch which addresses this. It also fixes the indentation in PySSL_SetError, bringing it into line with PEP 7, fixes a compile warning about one of the OpenSSL macros, and makes the namespace a bit more consistent. I've tested it on FC 7 and OS X 10.4. % ./python ./Lib/test/regrtest.py -R :1: -u all test_ssl test_ssl beginning 6 repetitions 123456 ...... 1 test OK. [29244 refs] % Bill Index: Modules/_ssl.c =================================================================== --- Modules/_ssl.c (revision 57564) +++ Modules/_ssl.c (working copy) @@ -122,71 +122,74 @@ char buf[2048]; char *errstr; int err; - enum py_ssl_error p; + enum py_ssl_error p = PY_SSL_ERROR_NONE; assert(ret <= 0); - err = SSL_get_error(obj->ssl, ret); + if ((obj != NULL) && (obj->ssl != NULL)) { + err = SSL_get_error(obj->ssl, ret); - switch (err) { - case SSL_ERROR_ZERO_RETURN: - errstr = "TLS/SSL connection has been closed"; - p = PY_SSL_ERROR_ZERO_RETURN; - break; - case SSL_ERROR_WANT_READ: - errstr = "The operation did not complete (read)"; - p = PY_SSL_ERROR_WANT_READ; - break; - case SSL_ERROR_WANT_WRITE: - p = PY_SSL_ERROR_WANT_WRITE; - errstr = "The operation did not complete (write)"; - break; - case SSL_ERROR_WANT_X509_LOOKUP: - p = PY_SSL_ERROR_WANT_X509_LOOKUP; - errstr = "The operation did not complete (X509 lookup)"; - break; - case SSL_ERROR_WANT_CONNECT: - p = PY_SSL_ERROR_WANT_CONNECT; - errstr = "The operation did not complete (connect)"; - break; - case SSL_ERROR_SYSCALL: - { - unsigned long e = ERR_get_error(); - if (e == 0) { - if (ret == 0 || !obj->Socket) { - p = PY_SSL_ERROR_EOF; - errstr = "EOF occurred in violation of protocol"; - } else if (ret == -1) { - /* the underlying BIO reported an I/O error */ - return obj->Socket->errorhandler(); - } else { /* possible? */ + switch (err) { + case SSL_ERROR_ZERO_RETURN: + errstr = "TLS/SSL connection has been closed"; + p = PY_SSL_ERROR_ZERO_RETURN; + break; + case SSL_ERROR_WANT_READ: + errstr = "The operation did not complete (read)"; + p = PY_SSL_ERROR_WANT_READ; + break; + case SSL_ERROR_WANT_WRITE: + p = PY_SSL_ERROR_WANT_WRITE; + errstr = "The operation did not complete (write)"; + break; + case SSL_ERROR_WANT_X509_LOOKUP: + p = PY_SSL_ERROR_WANT_X509_LOOKUP; + errstr = "The operation did not complete (X509 lookup)"; + break; + case SSL_ERROR_WANT_CONNECT: + p = PY_SSL_ERROR_WANT_CONNECT; + errstr = "The operation did not complete (connect)"; + break; + case SSL_ERROR_SYSCALL: + { + unsigned long e = ERR_get_error(); + if (e == 0) { + if (ret == 0 || !obj->Socket) { + p = PY_SSL_ERROR_EOF; + errstr = "EOF occurred in violation of protocol"; + } else if (ret == -1) { + /* the underlying BIO reported an I/O error */ + return obj->Socket->errorhandler(); + } else { /* possible? */ + p = PY_SSL_ERROR_SYSCALL; + errstr = "Some I/O error occurred"; + } + } else { p = PY_SSL_ERROR_SYSCALL; - errstr = "Some I/O error occurred"; + /* XXX Protected by global interpreter lock */ + errstr = ERR_error_string(e, NULL); } - } else { - p = PY_SSL_ERROR_SYSCALL; - /* XXX Protected by global interpreter lock */ - errstr = ERR_error_string(e, NULL); + break; } - break; - } - case SSL_ERROR_SSL: - { - unsigned long e = ERR_get_error(); - p = PY_SSL_ERROR_SSL; - if (e != 0) - /* XXX Protected by global interpreter lock */ - errstr = ERR_error_string(e, NULL); - else { /* possible? */ - errstr = "A failure in the SSL library occurred"; + case SSL_ERROR_SSL: + { + unsigned long e = ERR_get_error(); + p = PY_SSL_ERROR_SSL; + if (e != 0) + /* XXX Protected by global interpreter lock */ + errstr = ERR_error_string(e, NULL); + else { /* possible? */ + errstr = "A failure in the SSL library occurred"; + } + break; } - break; + default: + p = PY_SSL_ERROR_INVALID_ERROR_CODE; + errstr = "Invalid error code"; + } + } else { + errstr = ERR_error_string(ERR_peek_last_error(), NULL); } - default: - p = PY_SSL_ERROR_INVALID_ERROR_CODE; - errstr = "Invalid error code"; - } - PyOS_snprintf(buf, sizeof(buf), "_ssl.c:%d: %s", lineno, errstr); v = Py_BuildValue("(is)", p, buf); if (v != NULL) { @@ -256,8 +259,8 @@ ret = SSL_CTX_load_verify_locations(self->ctx, cacerts_file, NULL); Py_END_ALLOW_THREADS - if (ret < 1) { - errstr = ERRSTR("SSL_CTX_load_verify_locations"); + if (ret != 1) { + PySSL_SetError(NULL, 0, __FILE__, __LINE__); goto fail; } } @@ -267,8 +270,8 @@ ret = SSL_CTX_use_PrivateKey_file(self->ctx, key_file, SSL_FILETYPE_PEM); Py_END_ALLOW_THREADS - if (ret < 1) { - errstr = ERRSTR("SSL_CTX_use_PrivateKey_file error"); + if (ret != 1) { + PySSL_SetError(NULL, 0, __FILE__, __LINE__); goto fail; } @@ -276,8 +279,8 @@ ret = SSL_CTX_use_certificate_chain_file(self->ctx, cert_file); Py_END_ALLOW_THREADS - if (ret < 1) { - errstr = ERRSTR("SSL_CTX_use_certificate_chain_file error") ; + if (ret != 1) { + PySSL_SetError(NULL, 0, __FILE__, __LINE__); goto fail; } SSL_CTX_set_options(self->ctx, SSL_OP_ALL); /* ssl compatibility */ @@ -375,7 +378,7 @@ } static PyObject * -PySocket_ssl(PyObject *self, PyObject *args) +PySSL_sslwrap(PyObject *self, PyObject *args) { PySocketSockObject *Sock; int server_side = 0; @@ -431,6 +434,9 @@ PyObject *pd = PyDict_New(); int index_counter; + if (pd == NULL) + return NULL; + for (index_counter = 0; index_counter < X509_NAME_entry_count(xname); index_counter++) @@ -548,7 +554,7 @@ } Py_DECREF(pnotBefore); - BIO_reset(biobuf); + (void) BIO_reset(biobuf); notAfter = X509_get_notAfter(self->peer_cert); ASN1_TIME_print(biobuf, notAfter); len = BIO_gets(biobuf, buf, sizeof(buf)-1); @@ -857,7 +863,7 @@ static PyTypeObject PySSL_Type = { PyVarObject_HEAD_INIT(NULL, 0) - "socket.SSL", /*tp_name*/ + "ssl.SSLContext", /*tp_name*/ sizeof(PySSLObject), /*tp_basicsize*/ 0, /*tp_itemsize*/ /* methods */ @@ -932,7 +938,7 @@ "RAND_egd(path) -> bytes\n\ \n\ Queries the entropy gather daemon (EGD) on socket path. Returns number\n\ -of bytes read. Raises socket.sslerror if connection to EGD fails or\n\ +of bytes read. Raises ssl.sslerror if connection to EGD fails or\n\ if it does provide enough data to seed PRNG."); #endif @@ -940,7 +946,7 @@ /* List of functions exported by this module. */ static PyMethodDef PySSL_methods[] = { - {"sslwrap", PySocket_ssl, + {"sslwrap", PySSL_sslwrap, METH_VARARGS, ssl_doc}, #ifdef HAVE_OPENSSL_RAND {"RAND_add", PySSL_RAND_add, METH_VARARGS, @@ -979,7 +985,7 @@ SSLeay_add_ssl_algorithms(); /* Add symbols to module dict */ - PySSLErrorObject = PyErr_NewException("socket.sslerror", + PySSLErrorObject = PyErr_NewException("ssl.sslerror", PySocketModule.error, NULL); if (PySSLErrorObject == NULL)