Re: [pyOpenSSL] Python core dumps when using pyOpenSSL 0.8.0 and threads

[Jean-Paul C. <ex...@di...>]

On Tue, 17 Mar 2009 12:36:48 +0100, "M.-A. Lemburg" <ma...@eg...> wrote:
>On 2009-03-17 00:28, Jean-Paul Calderone wrote:
>> On Mon, 16 Mar 2009 21:43:49 +0100, "M.-A. Lemburg" <ma...@eg...> wrote:
>>> It appears as if the new thread lock support in pyOpenSSL 0.8.0 is
>>> causing serious problems with Python and threads. We have observed
>>> regular core dumps using pyOpenSSL 0.8 which did not happen with
>>> 0.7.
>>
>> Ouch.
>>
>>> There's a patch available on SF which appears to fix the problem:
>>>
>>> http://sourceforge.net/tracker/index.php?func=detail&aid=2543118&group_id=31249&atid=401760
>>>
>>> Any chance of getting that into a new release ?
>>
>> A quick skim of the patch leads me to believe it largely reverts one of
>> the bug fixes I made in 0.8.0 which was causing thread-related crashes
>> in 0.7.0.  https://bugs.launchpad.net/pyopenssl/+bug/262381 includes
>> some discussion of that issue.  In the light of my final comment on
>> that ticket, it may be that the new failure is much worse than the old
>> failure.
>
>The old code wasn't really protecting the programmer against
>core dumps caused by misusing OpenSSL (e.g. sharing SSL connections
>between threads), but it also did not get in the way of multi-threaded
>programs which did implement the correct way of not sharing connections.
>
>However, with the 0.8.0 approach, SSL connections started getting
>in the way of perfectly fine working applications. At least in our
>application we also had the problem of not being able to reproduce
>the problem easily - it occurred seemingly random and the core dumps
>did not relate to the SSL code either.

Isn't this just how it goes with threading bugs? ;)  You're right, though,
it's worse now than it was.

> [snip]
>
>I think you should not try to unroll the
>
>Py_BEGIN_ALLOW_THREADS
>...Do some blocking I/O operation...
>Py_END_ALLOW_THREADS
>
>blocks and just use those macros in the code.

pyOpenSSL has always done this.  It's necessary if Python-defined
callbacks are going to be supported, because the GIL must be re-acquired
in the callback.  This means thread state structured needs to be available
in the callback, which in turn means Py_BEGIN_ALLOW_THREADS isn't usable.

It's possible that PyGILState_Ensure and PyGILState_Release would be
useful here, but I'm not sure.  I need to investigate further.

>Storing the thread state in some application data object is not a
>good approach, since there's nothing preventing some other thread
>from restoring the thread state saved by the thread that just
>released the lock.
>
>By contrast, the above macros will add a local stack variable and
>store the thread state there, so it's not possible for another
>thread to accidentally restore a thread state that it doesn't
>own.

Right.  That's why I changed pyOpenSSL /from/ doing that (prior to 0.8)
to using thread local storage instead (0.8).  However, it seems there's
some bug in the thread local storage version of the locking scheme.


>See
>http://docs.python.org/c-api/init.html#thread-state-and-the-global-interpreter-lock
>
>IMHO, programmers should just be made aware of the fact that OpenSSL
>connections should not be shared among threads - just like the DB-API
>strongly suggests to not share database connections between threads,
>but doesn't try to prevent this.
>
>>> As it stands, version 0.8.0 is not really usable in multi-threaded
>>> applications.
>>
>> It seems that 0.7.0 wasn't really either.  And I suspect that without
>> various vendor-supplied patches that didn't make it upstream until
>> 0.8.0, neither were any of the releases before that one either. :/
>
>See above.
>
>0.7 worked just fine for applications using the correct
>implementation approach.

Right.  I meant that 0.6 and earlier would crash randomly when pyOpenSSL
APIs were being used from more than one thread.  0.7 is probably the most
stable version of pyOpenSSL wrt threading.

>> Having to revisit this part of pyOpenSSL again, I'm tempted to go for
>> a solution I previously ruled out for performance reasons - explicitly
>> tag each OpenSSL object wrapper (for those OpenSSL structures which
>> can only be used in one thread) with a thread id and before each
>> operation, check to make sure the current thread matches that thread id.
>> This is similar to the approach taken by SQLite3, I think.  It should
>> provide the most robust protection against thread issues, but at the
>> cost of a thread check per method.
>
>I wouldn't go down that road... most Python C extensions simply use the
>above macro pair around blocking operations and don't bother with
>any further tricks to work around issues in the underlying C library
>code.

I'll think about that.  I'd prefer for there to be no way to crash the
interpreter using pyOpenSSL, though.  Probably which decision I make will
depend in part on what the actual problem with the thread local storage
approach in 0.8 is.

Jean-Paul