I seem to have a rather unique setup that causes this crash to be 100% reproducible. My application embeds python in order to execute user code. It is constantly loading and unloading the libraries so that they're only in memory during execution of user code. The problem I'm seeing is with the calls to CRYPTO_set_locking_callback and CRYPTO_set_id_callback in _setup_ssl_threads in _ssl.c. These calls will override whatever callbacks my application has already set up, and then when we unload python, callbacks are never restored. When my application later makes an SSL call that requires use of locking_callback or id_callback, it will attempt to call one of the functions in _ssl.so address space. Since nothing is there, this causes the program to crash. Worse yet would be if something were loaded into the same address space and arbitrary code were executed (though I don't see how malicious code could be executed in this way).

I haven't confirmed with other version of Python, but this was discovered while upgrading the embedded version from 2.4.5 to 2.6.4, so it's very likely to exist in many other version since the code was put in place in 2007.

Why do you think this is a bug in Python?

Because Python is not cleaning up after itself. I don't see how a multi-threaded app could work around this issue. The only solution I can think of at the app level is to reset those callbacks once python exits, but a different thread could call an SSL function at any point and cause the crash between the time that _ssl.so is unloaded and the app resets the callbacks.

One solution for Python would be to call CRYPTO_get_id_callback and CRYPTO_get_locking_callback and check that they're NULL before setting them. However, it's also stated in the documentation that id_callback doesn't need to be set for all platforms, so a NULL value could still be safe there. I haven't looked at the callbacks python is setting up, so I can't be sure this solution would work.

What operating system is this on?

IIUC, Python has no chance to perform any cleanup. So it's not a bug that no cleanup happens.

I don't understand the proposal of not setting the callback if they are already set. If that is done, how can we be sure that the callbacks that have been installed work correctly?

The issue was debugged on AMD64 Linux, but I was seeing similar crashing on OSX but could not debug because I wasn't getting a proper stack trace (probably because something else was being loaded into that memory space). I have yet to test if not setting those functions on OSX fixes the crashing, but I will soon.

Well Python's installed callbacks don't work correctly, so wouldn't it be safe to assume that if an app has installed callbacks of their own that those work correctly, or at least better than Python's, i.e. they don't bring down the entire app when Python is unloaded.

So how do you do cleanup on Linux, or on OSX, in a shared library?

I'd claim that the bug is in your application. It shouldn't unload a DLL that is still in use (by having a function pointer into it stored globally).

Testing for a prior pointer value may fix it for you. It won't fix it in the general case, i.e. when you unload the ssl module and *didn't* install your own callbacks, OpenSSl would still call Python's callback.

You've got init_* that Python calls whenever it loads a library, you could just as easily have destroy_*. But that would probably be overkill.

How would the application know that Python has created callbacks? This is just one instance. Who knows where else this is done in any number of libraries. You you suggest that any application which dynamically loads a library never unload it?

What is the general case that this would not be fixed in? If a multi-threaded app uses SSL and doesn't load its own callbacks then it is in violation as libcryto states that certain callbacks must be set. If it doesn't use SSL then it's never going to have a problem as the callbacks will never be called after _ssl.so is unloaded. So the remaining case is a single threaded app that doesn't load the callbacks because it's single-threaded. But in that case it should be using a single threaded python library or it is in violation as it can no longer be considered a single-threaded app, and therefore should have loaded its own callbacks.

> You you suggest that any application which
> dynamically loads a library never unload it?

Correct. Library unloading just don't work. Trying it anyway
is an endless uphill battle.

Okay, what if I attack this problem from a "it's not thread-safe" point of view? If the callbacks are already loaded, then who knows what state the locks are in. If you replace the locking_callback while a thread already has the lock, and another thread comes in and tries to lock, it will succeed immediately, and two threads will be in the critical section.

Attaching a patch for how I think this should work.

From Bill via email:
Hmmm, well, is there a standard way to unload Python?  I could put a
__del__ method on the module which would remove the callbacks, I
suppose.  I just never heard of "unloading" a module before.

There is no way to unload a module, at least not until Python 3. Losing all references to the module won't help.

Yea, I've given up on getting this fixed based on the crash. Now I'm going for it not being thread safe.

The patch is not thread-safe. If two threads where to do the same thing simultaneously, they might (simultaneously) both determine that the lock is not installed, and then one would overwrite the lock of the other, leading exactly to the situation that the patch is meant to prevent.

For an app that makes use of SSL itself it better set the callbacks before spawning threads or it's going to be in trouble anyway. For an app not making use of SSL my patch doesn't make the situation any worse. That sounds like an overall gain to me.

Bill, what do you think?

Martin, I'm thinking that the module object has a __del__ method, and we could un-register the callbacks there.  But I don't know if that method would ever get called.  How does the act of "unloading a library" interact with the initialized Python interpreter?

Simply unloading the callbacks wouldn't be wise. Callbacks are necessary for proper thread safety with libcrypto (man pages says random crashing could occur without them). So setting them to NULL could cause random crashing which is even worse than what's there now. Restoring the existing callbacks is one option. It's also not thread safe to be chaing the callbacks all the time. Is it critical that Python install its own callbacks over whatever the app has provided?

Bill: unloading the shared probably means he does dlclose() (after having done dlopen initially). Furthermore, it probably means he does that on libpythonxy.so. I'm a bit puzzled that it also affects _ssl.so (unless he dlcloses that as well) - Python, on its own, will never dlclose any library.

In any case, calling dlclose to unload a shared library has no effect on Python whatsoever. No callback is triggered, and no interpreter code gets executed in that process.

You are correct, dlclose is called on libpythonXY.so and all .so modules loaded by it.

As for the module's __del__: that is already implemented in moduleobject.c:module_dealloc. It doesn't give the module implementation a hook to execute custom code. In Py3k, you can implemenent the m_free hook of the PyModuleDef.

I don't think asking Python extensions to be safe in the face of brutal DLL unloading is wise. I don't think any of our C extensions tries to respect such an use case. We could still try to support the use case of some application registering its own callbacks before importing _ssl.

As for the patch, it looks wrong since CRYPTO_get_locking_callback() is compared to NULL a second time after the callback has been set to something non-NULL.

OpenSSL 1.1.0 comes with its own locking code. I'm not going to change the module for < 1.1.0.