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

On Tue, Nov 21, 2023 at 12:14:16PM +0300, Aleksander Alekseev wrote:

Thanks for the report, Willi, and the test case! Thanks Aleksander
for the reply.

> I wonder if we should just document that libpq is thread safe as of PG
> v??? and deprecate PQisthreadsafe() at some point. Currently the
> documentation gives an impression that the library may or may not be
> thread safe depending on the circumstances.

Because --disable-thread-safe has been removed recently in
68a4b58eca03. The routine could be marked as deprecated on top of
saying that it always returns 1 for 17~.

> Please add the patch to the nearest open commit fest [2]. The patch
> will be automatically picked up by cfbot [3] and tested on different
> platforms. Also this way it will not be lost among other patches.
>
> The code looks OK but I would appreciate a second opinion from cfbot.
> Also maybe a few comments before my_BIO_methods_init_mutex and/or
> pthread_mutex_lock would be appropriate. Personally I am inclined to
> think that the automatic test in this particular case is redundant.

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. 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?
--
Michael