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

On 2015-02-09 18:17:14 +0100, Jan Urbański wrote:
> We added unsetting the locking callback in
> 4e816286533dd34c10b368487d4079595a3e1418 due to this bug report:
> http://www.postgresql.org/message-id/48620925.6070806@pws.com.au
>
> Indeed, commenting out the CRYPTO_set_locking_callback(NULL) call in
> fe-secure-openssl.c gets rid of the deadlock. However, it makes php segfault
> with the (attached) reproduction script from the original 2008 bug report. If
> your php.ini loads both the pgsql and curl extensions, reproduce the segfault with:
>
> php -f pg_segfault.php
>
> The most difficult part about fixing this bug is to determine *who's at
> fault*. I now lean towards the opinion that we shouldn't be messing with
> OpenSSL callbacks *at all*.
>
> First of all, the current behaviour is crazy. We're setting and unsetting the
> locking callback every time a connection is made/closed, which is not how
> OpenSSL is supposed to be used. The *application* using libpq should set a
> callback before it starts threads, it's no business of the library's.

I don't buy this argument at all. Delivering a libpq that's not
threadsafe in some circumstances is a really bad idea.

> The old behaviour was slightly less insane (set callbacks first time we're
> engaging OpenSSL code, never touch them again). The real sane solution is to
> leave it up to the application.

The real solution would be for openssl to do it itself.

> I posit we should remove all CRYPTO_set_*_callback functions and associated
> cruft from libpq. This unfortunately means that multi-threaded applications
> using libpq and SSL will break if they haven't been setting their own callbacks
> (if they have, well, tough luck! libpq will just stomp over them the first time
> it connects to Postgres, but at least *some* callbacks are left present after
> that).

I think there's no chance in hell we can do that. Breaking a noticeable
amount of libpq users in a minor upgrade is imo a cure *FAR* worse than
the cure.

> Note that the PHP pgsql extension doesn't set the OpenSSL callbacks, because
> libpq was setting them on its own. If we remove the callback handling from
> libpq, PHP will need to add them. By the way, the MySQL extension for PHP also
> does not set those callbacks.

I think this shows pretty clearly how insane it would be to change this
in a minor release. Do you really expect people to update libpq and php
in unison. After a minor release?

Note that we *already* provide applications with the ability to set the
callbacks themselves and prevent us from doing so: PQinitSSL(false).

> Let me reiterate: I now believe the callbacks should be set by the application,
> libraries should not touch them, since they don't know what will they be
> stomping on. If the application is run through a VM like Python or PHP, it's
> the VM that should make sure the callbacks are set.

I fail to see why it's any better to do it in the VM. That relies on
either always loading the VM's openssl module, even if you don't
actually need it because the library you use deals with openssl. It
prevents other implementations of openssl in the VM.

What I think we should do is the following:

* Emphasize the fact that it's important to use PQinitSSL(false) if you
did things yourself.
* If there's already callbacks set: Remember that fact and don't
overwrite. In the next major version: warn.
* Assert or something if the callback when unregistering isn't the same
as it was when registering. That's pretty much guaranteed to cause
issues.

> I would very much like to have this change back-patched, since setting and
> resetting the callback makes using libpq in a threaded OpenSSL-enabled app
> arguably less safe than if it didn't use any locking.

Meh^2

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services