Re: [PATCH] fix race condition in libpq (related to ssl connections)

On Wed, Nov 22, 2023 at 10:43:32AM +0900, Michael Paquier wrote:
> I am not really convinced that we require a second mutex here, as
> there is always a concern with inter-locking changes. I may be
> missing something, of course, but BIO_s_socket() is just a pointer to
> a set of callbacks hidden in bss_sock.c with BIO_meth_new() and
> BIO_get_new_index() assigning some centralized data to handle the
> methods in a controlled way in OpenSSL.

I was looking at the origin of this one, and this is an issue coming
down to 8bb14cdd33de that has removed the ssl_config_mutex taken in
pgtls_open_client() when we called my_SSL_set_fd(). The commit has
accidentally missed that path with the static BIO method where the
mutex mattered.

> We only case about
> initializing once for the sake of libpq's threads, so wouldn't it be
> better to move my_BIO_s_socket() in pgtls_init() where the
> initialization of the BIO methods would be protected by
> ssl_config_mutex? That's basically what Willi means in his first
> message, no?

I've looked at this idea, and finished by being unhappy with the error
handling that we are currently assuming in my_SSL_set_fd() in the
event of an error in the bio method setup, which would be most likely
an OOM, so let's use ssl_config_mutex in my_BIO_s_socket(). Another
thing is that I have minimized the manipulation of my_bio_methods in
the setup routine.

I've been also testing the risks of collusion, and it takes me quite a
a few tries with hundred threads to reproduce the failure even without
any forced sleep, so that seems really hard to reach.

Please find attached a patch (still need to do more checks with older
versions of OpenSSL). Any thoughts or comments?
--
Michael