Re: libpq's multi-threaded SSL callback handling is busted

Peter Eisentraut writes:
> On 4/2/15 4:32 AM, Jan Urbański wrote:
>>
>> Peter Eisentraut writes:
>>> I don't think this patch would actually fix the problem that was
>>> described after the original bug report
>>> (http://www.postgresql.org/message-id/5436991B.5020708@vmware.com)
>>> namely that another thread acquires a lock while the libpq callbacks are
>>> set and then cannot release the lock if libpq has been shut down in the
>>> meantime.
>>
>> I did test both the Python and the PHP repro scripts and the patch fixed both
>> the deadlock and the segfault.
>>
>> What happens is that Python (for instance) stops over the callback
>> unconditionally. So when libpq gets unloaded, it sees that the currently set
>> callback is no the one it originally set and refrains from NULLing it.
>
> So this works because Python is just as broken as libpq right now. What
> happens if Python is fixed as well? Then we'll have the problem I
> described above.

Well, not really, libpq sets and unsets the callbacks every time an SSL
connection is opened and closed. Python sets the callbacks once when the ssl
module is imported and never touches them again.

Arguably, it should set them even earlier, but it's still saner than
flip-flopping them. AFAIK you can't "unload" Python, so setting the callback
and keeping it there is a sound strategy.

The change I'm proposing is not to set the callback in libpq if one is already
set and not remove it if the one that's set is not libpq's. That's as sane as
it gets.

With multiple libraries wanting to use OpenSSL in threaded code, the mechanism
seems to be "last one wins". It doesn't matter *who's* callbacks are used, as
long as they're there and they stay there and that's Python's stance. This
doesn't work if you want to be able to unload the library, so the next best
thing is not touching the callback if someone else set his in the meantime.

What's broken is OpenSSL for offering such a bad way of dealing with
concurrency.

To reiterate: I recognise that not handling the callbacks is not the right
answer. But not stomping on callbacks others might have set sounds like a
minimal and safe improvement.

Cheers,
Jan