From 3f79ceaf76cabb14228f63813a1101b6944fbf39 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 14:56:35 -0800 Subject: Fix test_export_invalid with certain versions of OpenSSL. Previously OpenSSL did the NULL check for us, now we do it. --- OpenSSL/crypto/crl.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/OpenSSL/crypto/crl.c b/OpenSSL/crypto/crl.c index 614a606..543708e 100644 --- a/OpenSSL/crypto/crl.c +++ b/OpenSSL/crypto/crl.c @@ -138,6 +138,16 @@ crypto_CRL_export(crypto_CRLObj *self, PyObject *args, PyObject *keywds) { return NULL; } + /* Some versions of OpenSSL check for this, but more recent versions seem + * not to. + */ + if (!key->pkey->ameth) { + PyErr_SetString( + crypto_Error, "Cannot export with an unitialized key"); + return NULL; + } + + bio = BIO_new(BIO_s_mem()); tmptm = ASN1_TIME_new(); if (!tmptm) { -- cgit v1.2.1 From 7eb2e748eb372826ff8057957b1592c4e36d1232 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 14:57:47 -0800 Subject: Get rid of the OPENSSL_NO_SSL2 check, which is not a complete solution on all platforms; replace it with a check (which should always have been there) of the SSL_CTX_new return value. If SSLv2 is unavailable, the context creation should fail and we will notice at that point. --- OpenSSL/ssl/context.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/OpenSSL/ssl/context.c b/OpenSSL/ssl/context.c index 0b9f4b6..534d207 100644 --- a/OpenSSL/ssl/context.c +++ b/OpenSSL/ssl/context.c @@ -1246,12 +1246,7 @@ ssl_Context_init(ssl_ContextObj *self, int i_method) { switch (i_method) { case ssl_SSLv2_METHOD: -#ifdef OPENSSL_NO_SSL2 - PyErr_SetString(PyExc_ValueError, "SSLv2_METHOD not supported by this version of OpenSSL"); - return NULL; -#else method = SSLv2_method(); -#endif break; case ssl_SSLv23_METHOD: method = SSLv23_method(); @@ -1268,6 +1263,11 @@ ssl_Context_init(ssl_ContextObj *self, int i_method) { } self->ctx = SSL_CTX_new(method); + if (self->ctx == NULL) { + exception_from_error_queue(ssl_Error); + return NULL; + } + Py_INCREF(Py_None); self->passphrase_callback = Py_None; Py_INCREF(Py_None); -- cgit v1.2.1 From b587107276e8ff548ab8f65cf504bb4906f2fd83 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 14:58:00 -0800 Subject: Fix an incorrect exception name in a test method docstring. --- OpenSSL/test/test_crypto.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSSL/test/test_crypto.py b/OpenSSL/test/test_crypto.py index 62b9429..16799ef 100644 --- a/OpenSSL/test/test_crypto.py +++ b/OpenSSL/test/test_crypto.py @@ -2603,7 +2603,7 @@ class CRLTests(TestCase): def test_export_invalid(self): """ If :py:obj:`CRL.export` is used with an uninitialized :py:obj:`X509` - instance, :py:obj:`ValueError` is raised. + instance, :py:obj:`OpenSSL.crypto.Error` is raised. """ crl = CRL() self.assertRaises(Error, crl.export, X509(), PKey()) -- cgit v1.2.1 From 15f3a8683af475c0fdd0ecddbaf6d3df5c1e8bf5 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 15:04:04 -0800 Subject: Re-instate the OPENSSL_NO_SSL2 check; it is necessary for the case where SSLv2_method is not a defined symbol at all. --- OpenSSL/ssl/context.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/OpenSSL/ssl/context.c b/OpenSSL/ssl/context.c index 534d207..e971c0a 100644 --- a/OpenSSL/ssl/context.c +++ b/OpenSSL/ssl/context.c @@ -1246,7 +1246,12 @@ ssl_Context_init(ssl_ContextObj *self, int i_method) { switch (i_method) { case ssl_SSLv2_METHOD: +#ifdef OPENSSL_NO_SSL2 + PyErr_SetString(PyExc_ValueError, "SSLv2_METHOD not supported by this version of OpenSSL"); + return NULL; +#else method = SSLv2_method(); +#endif break; case ssl_SSLv23_METHOD: method = SSLv23_method(); -- cgit v1.2.1 From 7e8dab02ce8a8acf750153c46c19215223dcc67c Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 15:25:48 -0800 Subject: Add a version check, since older versions of OpenSSL are missing ameth --- OpenSSL/crypto/crl.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/OpenSSL/crypto/crl.c b/OpenSSL/crypto/crl.c index 543708e..3f56d83 100644 --- a/OpenSSL/crypto/crl.c +++ b/OpenSSL/crypto/crl.c @@ -2,7 +2,6 @@ #define crypto_MODULE #include "crypto.h" - static X509_REVOKED * X509_REVOKED_dup(X509_REVOKED *orig) { X509_REVOKED *dupe = NULL; @@ -138,15 +137,19 @@ crypto_CRL_export(crypto_CRLObj *self, PyObject *args, PyObject *keywds) { return NULL; } - /* Some versions of OpenSSL check for this, but more recent versions seem - * not to. + +#if OPENSSL_VERSION_NUMBER >= 0x01000000L + /* Older versions of OpenSSL had no problem with trying to export using an + * uninitialized key. Newer versions segfault, instead. We can only check + * on the new versions, though, because the old versions don't even have the + * field that the segfault is triggered by. */ if (!key->pkey->ameth) { PyErr_SetString( crypto_Error, "Cannot export with an unitialized key"); return NULL; } - +#endif bio = BIO_new(BIO_s_mem()); tmptm = ASN1_TIME_new(); -- cgit v1.2.1 From 5b79104333748cb4336252b8a7b2e3498a313353 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Fri, 9 Mar 2012 15:27:50 -0800 Subject: Be more lenient in the test --- OpenSSL/test/test_ssl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OpenSSL/test/test_ssl.py b/OpenSSL/test/test_ssl.py index 721c1c0..3e4e3da 100644 --- a/OpenSSL/test/test_ssl.py +++ b/OpenSSL/test/test_ssl.py @@ -313,7 +313,7 @@ class ContextTests(TestCase, _LoopbackMixin): try: Context(SSLv2_METHOD) - except ValueError: + except (Error, ValueError): # Some versions of OpenSSL have SSLv2, some don't. # Difficult to say in advance. pass -- cgit v1.2.1 From 294dc1e1ed71984b72ac3120165d4695f3c58779 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Sat, 10 Mar 2012 16:55:52 -0800 Subject: Allocate Py_buffer on the heap (leaking memory sometimes) to make this code PyPy-friendly (hopefully; test pending). --- OpenSSL/ssl/connection.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/OpenSSL/ssl/connection.c b/OpenSSL/ssl/connection.c index ebbe39f..a2075a2 100755 --- a/OpenSSL/ssl/connection.c +++ b/OpenSSL/ssl/connection.c @@ -425,17 +425,20 @@ ssl_Connection_send(ssl_ConnectionObj *self, PyObject *args) { char *buf; #if PY_VERSION_HEX >= 0x02060000 - Py_buffer pbuf; - if (!PyArg_ParseTuple(args, "s*|i:send", &pbuf, &flags)) + Py_buffer *pbuf = PyMem_Malloc(sizeof *pbuf); + + if (!PyArg_ParseTuple(args, "s*|i:send", pbuf, &flags)) { return NULL; + } - buf = pbuf.buf; - len = pbuf.len; + buf = pbuf->buf; + len = pbuf->len; #else - if (!PyArg_ParseTuple(args, "s#|i:send", &buf, &len, &flags)) + if (!PyArg_ParseTuple(args, "s#|i:send", &buf, &len, &flags)) { return NULL; + } #endif MY_BEGIN_ALLOW_THREADS(self->tstate) @@ -443,7 +446,8 @@ ssl_Connection_send(ssl_ConnectionObj *self, PyObject *args) { MY_END_ALLOW_THREADS(self->tstate) #if PY_VERSION_HEX >= 0x02060000 - PyBuffer_Release(&pbuf); + PyBuffer_Release(pbuf); + PyMem_Free(pbuf); #endif if (PyErr_Occurred()) @@ -482,16 +486,18 @@ ssl_Connection_sendall(ssl_ConnectionObj *self, PyObject *args) PyObject *pyret = Py_None; #if PY_VERSION_HEX >= 0x02060000 - Py_buffer pbuf; + Py_buffer *pbuf = PyMem_Malloc(sizeof *pbuf); - if (!PyArg_ParseTuple(args, "s*|i:sendall", &pbuf, &flags)) + if (!PyArg_ParseTuple(args, "s*|i:sendall", pbuf, &flags)) { return NULL; + } - buf = pbuf.buf; - len = pbuf.len; + buf = pbuf->buf; + len = pbuf->len; #else - if (!PyArg_ParseTuple(args, "s#|i:sendall", &buf, &len, &flags)) + if (!PyArg_ParseTuple(args, "s#|i:sendall", &buf, &len, &flags)) { return NULL; + } #endif do { @@ -520,7 +526,8 @@ ssl_Connection_sendall(ssl_ConnectionObj *self, PyObject *args) } while (len > 0); #if PY_VERSION_HEX >= 0x02060000 - PyBuffer_Release(&pbuf); + PyBuffer_Release(pbuf); + PyMem_Free(pbuf); #endif Py_XINCREF(pyret); -- cgit v1.2.1 From 4cc06a7ab794aa55f4a55a5757adebadd9151fc3 Mon Sep 17 00:00:00 2001 From: Jean-Paul Calderone Date: Sat, 10 Mar 2012 17:18:02 -0800 Subject: explain the way things are --- OpenSSL/ssl/connection.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/OpenSSL/ssl/connection.c b/OpenSSL/ssl/connection.c index a2075a2..9428376 100755 --- a/OpenSSL/ssl/connection.c +++ b/OpenSSL/ssl/connection.c @@ -426,6 +426,84 @@ ssl_Connection_send(ssl_ConnectionObj *self, PyObject *args) { #if PY_VERSION_HEX >= 0x02060000 + /* + * Sit back and I'll tell you a story of intrigue and corruption, deceit and + * murder. + * + * A Py_buffer is used to hold any kind of byte-like data - a string, a + * memoryview, a buffer, etc. PyArg_ParseTuple takes whatever kind of + * object was supplied, notices the "s*" format specifier, and tries to copy + * the metadata for that object into the Py_buffer also passed in. + * + * According to the Python documentation: + * + * ... the caller is responsible for calling PyBuffer_Release with the + * structure after it has processed the data. + * + * Correct use of PyBuffer_Release is necessary due to the fact that + * Py_buffer must hold a reference to the original Python object from which + * it was initialized. PyBuffer_Release will decrement the reference count + * of that original object, allowing it to eventually be deallocated - but + * only after the Py_buffer is no longer in use. + * + * To support failures partway through parsing a format string, + * PyArg_ParseTuple maintains an internal PyListObject of PyCObjects it has + * created so far. This allows it to easily clean up these objects when + * parsing fails, before returning an error to the caller (incidentally, it + * also makes sure to clean up the Py_buffer it initialized in this case, by + * calling PyBuffer_Release - which means the caller *must not* use + * PyBuffer_Release when PyArg_ParseTuple fails; not exactly what the + * documentation directs). + * + * The PyCObjects are given destructors which clean up some structure + * PyArg_ParseTuple has created (or initialized) - often another PyObject + * which needs to be decref'd. + * + * When parsing completes, the reference count of the PyListObject is merely + * decremented. The normal Python garbage collection logic (the reference + * counting logic, in this case) takes over and collects both the list and + * all of the objects in it. When each PyCObject in the list is collected, + * it triggers its destructor to clean up the structure it wraps. This all + * happens immediately, before the Py_XDECREF of the PyListObject returns. + * + * The PyListObject is similarly destroyed in the success case, but not + * until each PyCObject it contains has had its destructor set to NULL to + * prevent it from cleaning up its contents. + * + * When PyArg_ParseTuple returns in an error case, therefore, + * PyRelease_Buffer has already been used on the Py_buffer passed to + * PyArg_ParseTuple. + * + * This is fortuitous, as the Py_buffer is typically (always?) allocated on + * the stack of the caller of PyArg_ParseTuple. Once that caller returns, + * that stack memory is no longer valid, and the Py_buffer may no longer be + * used. + * + * On a Python runtime which does not use reference counting, the + * PyListObject may not actually be collected before Py_XDECREF returns. It + * may not even be collected before PyArg_ParseTuple returns. In fact, in + * particularly unfortunate cases, it may even not be collected before the + * caller of PyArg_ParseTuple returns. It may not be called until long, + * long after the stack memory the Py_buffer was allocated on has been + * re-used for some other function call, after the memory holding a pointer + * to the structure that owns the memory the Py_buffer wrapped has been + * overwritten. When PyBuffer_Release is used on the Py_buffer in this + * case, it will try to decrement a random integer - probably part of a + * local variable on some part of the stack. + * + * The PyPy runtime does not use reference counting. + * + * The solution adopted here is to allocate the Py_buffer on the heap, + * instead. As there is no mechanism for learning when the PyCObject used + * by PyArg_ParseTuple to do its internal cleanup has had its way with the + * Py_buffer, the Py_buffer is leaked in the error case, to ensure it is + * still valid whenever PyBuffer_Release is called on it. + * + * Real programs should rarely, if ever, trigger the error case of + * PyArg_ParseTuple, so this is probably okay. Plus, I'm tired of this + * stupid bug. -exarkun + */ + Py_buffer *pbuf = PyMem_Malloc(sizeof *pbuf); if (!PyArg_ParseTuple(args, "s*|i:send", pbuf, &flags)) { -- cgit v1.2.1