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

[M.-A. Lemburg]

On 2009-03-17 00:28, Jean-Paul Calderone wrote:
> On Mon, 16 Mar 2009 21:43:49 +0100, "M.-A. Lemburg" <mal at egenix.com> 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.

> What would make things much easier for me is if someone could provide a
> test case which demonstrates the current bug.  The patch which "fixes"
> the current problem looks simple enough (it's mostly noise - it could
> be 10 lines long instead of a hundred), but since I think it re-introduces
> another thread bug, I probably would prefer /not/ to apply it exactly,
> but to work out a solution which preserves the old fix and eliminates
> the new bug.
> 
> If a unit test is too hard, then something along the lines of
> leakcheck/thread-crash.py would also be useful.
> 
> If that's too hard, an explanation of why the current code is wrong
> would help.

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.

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.

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.

> 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'm hoping to have 0.9.0 out soon.  Before this issue was raised, I
> was only blocked on some issues building Windows packages.  I suppose
> it would be good to include some resolution to this issue in the
> next release though.  If anyone will be at PyCon and wants to help out,
> let me know.

-- 
Marc-Andre Lemburg
eGenix.com

Professional Python Services directly from the Source  (#1, Mar 17 2009)
>>> Python/Zope Consulting and Support ...        http://www.egenix.com/
>>> mxODBC.Zope.Database.Adapter ...             http://zope.egenix.com/
>>> mxODBC, mxDateTime, mxTextTools ...        http://python.egenix.com/
________________________________________________________________________

::: Try our new mxODBC.Connect Python Database Interface for free ! ::::


   eGenix.com Software, Skills and Services GmbH  Pastor-Loeh-Str.48
    D-40764 Langenfeld, Germany. CEO Dipl.-Math. Marc-Andre Lemburg
           Registered at Amtsgericht Duesseldorf: HRB 46611
               http://www.egenix.com/company/contact/