Page MenuHomePhabricator

TypeError with ndg-httpsclient on Python2.7
Closed, ResolvedPublic

Description

Installing ndg-httpsclient on Python2.7 can cause a TypeError

https://travis-ci.org/VcamX/pywikibot-core/jobs/70758871

======================================================================
ERROR: test_https_cert_error (tests.http_tests.HttpsCertificateTestCase)
Test http.request fails on invalid omegawiki SSL certificate.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/VcamX/pywikibot-core/tests/http_tests.py", line 92, in test_https_cert_error
    uri='https://www.omegawiki.org/')
  File "/opt/python/2.7.9/lib/python2.7/unittest/case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "/home/travis/build/VcamX/pywikibot-core/pywikibot/tools/__init__.py", line 1227, in wrapper
    return obj(*__args, **__kw)
  File "/home/travis/build/VcamX/pywikibot-core/pywikibot/comms/http.py", line 230, in request
    r = fetch(uri, method, body, headers, **kwargs)
  File "/home/travis/build/VcamX/pywikibot-core/pywikibot/comms/http.py", line 363, in fetch
    error_handling_callback(request)
  File "/home/travis/build/VcamX/pywikibot-core/pywikibot/comms/http.py", line 280, in error_handling_callback
    raise request.data
TypeError: __str__ returned non-string (type Error)

Event Timeline

jayvdb raised the priority of this task from to Needs Triage.
jayvdb updated the task description. (Show Details)
jayvdb added a subscriber: jayvdb.

Yes, that looks like it. Honestly it doesnt appear to difficult to solve in urllib3. Maybe clone the urllib3 repo and attempt to fix it? If you find a solution, we can provide a first code review, then you can submit a pull request to urllib3.

I think it's a platform-specific bug for ssl.SSLError, or a misuse of ssl.SSLError.
I executed the following commands on different platforms:

>>> import ssl
>>> class AError(Exception): pass
>>> str(ssl.SSLError('a', AError('b')))

and got different results:

2.6.9:

>>> str(ssl.SSLError('a', AError('b')))
'[Errno a] b'

2.7.9:

>>> str(ssl.SSLError('a', AError('b')))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: __str__ returned non-string (type AError)

3.4.3:

>>> str(ssl.SSLError('a', AError('b')))
"('a', AError('b',))"

On 2.7.9, it seems that if the second element of args of ssl.SSLError is not a string (str or unicode), TypeError is always raised.
And urllib3 raise a ssl.SSLError with OpenSSL.SSL.Error as its second element of args. https://github.com/kennethreitz/requests/blob/master/requests/packages/urllib3/contrib/pyopenssl.py#L290

My patch is replace raise ssl.SSLError('bad handshake', e) with raise ssl.SSLError('bad handshake', str(e))

Can you re-try that in 2.7.8? It might be specific to 2.7.9+, which backported the Python3 ssl module.

@jayvdb, 2.7.8 works well, 2.7.10 has the same problem.

Thanks. That is what I expected.

Prior to py2.7.9 's backport of Python 3 ssl, aka PEP 466 aka issue 21308 , ssl.SSLError was a normal simple subclass of socket.error created using PyErr_NewException. socket.error doesnt have a __str__ handler, and is a subclass of IOError, which is an EnvironmentError.

https://hg.python.org/cpython/rev/92163#l23.3608

That change introduced a custom __str__ handler for SSLError.

https://hg.python.org/cpython/rev/92163#l23.3617
https://docs.python.org/2/c-api/typeobj.html#c.PyTypeObject.tp_str

static PyObject *
SSLError_str(PyEnvironmentErrorObject *self)
{
    if (self->strerror != NULL) {
        Py_INCREF(self->strerror);
        return self->strerror;
    }
    else
        return PyObject_Str(self->args);
}

The above simpler this returns strerror, without creating a copy and ignores the other args.
So in prior versions, str(SSLError) would be handled by a generic and 'safe' implementation which will construct the string representation using the args, but then in py279 the above assumes that self.strerror is a string.

SSLError is not at fault; the strerror passed to the SSLError constructor is expected to be a string.
https://docs.python.org/2/library/exceptions.html#exceptions.EnvironmentError
https://docs.python.org/3/library/exceptions.html#OSError

So, I believe this is a misuse of SSLError, and raise ssl.SSLError('bad handshake', str(e)) is a good solution. Does that change pass the urllib3 test suite?

It appears that this has been fixed in urllib3
https://github.com/shazow/urllib3/commit/e2de9f67f276f5a9c2ca3689ce50eac2c7361f4f

but that change has not been syncronised into requests.

@jayvdb Thanks for your explain! I was stuck in the C implementation of SSLError. It helps me a lot! :D

I've done a build of requests with 'security' enabled, and their tests dont fail, so I guess they do not have a test which covers this scenario.
https://travis-ci.org/jayvdb/requests/builds/71113089

The problem appears to have been fixed in https://github.com/shazow/urllib3/releases/tag/1.11

We're still waiting for requests 2.8.0 to be released with the urllib3 fix: https://github.com/kennethreitz/requests/pull/2678

AbdealiJK claimed this task.
AbdealiJK added a subscriber: AbdealiJK.

Closing this as requests has released 2.8.0 and even moved on to 2.10

Note: Someone had found that requests 2.9 still had an issue and that was fixed in https://github.com/shazow/urllib3/pull/792 finally. That commit landed in requests 2.10 which was released a few days ago.

Reopen if there's still an issue with it.