libpq OpenSSL and multithreading

On 27.06.25 19:24, Daniel Gustafsson wrote:

The OpenSSL code in libpq have two issues for multithreading: the verify_cb
callback use a global variable to pass back error detail state and there is one
use of strerror().

Slightly misleading title: This is actually about the *backend* libpq code.

The attached fixes both, with no functional change, in order to pave the way
for multithreading:

* Rather than using a global variable the callback use a new member in the
Port struct for passing the string, and the Port struct is in turn passed as
private data in the SSL object

Couldn't this also be done by making that global variable thread-local?
But getting rid of it is even nicer.

* The strerror call is replaced with a strerror_r call using the already
existing errorbuffer

This one was already discussed some time ago at
</messages/by-id/daa87d79-c044-46c4-8458-8d77241ed7b0@eisentraut.org&gt;:

But the bigger issue is that the use of a static buffer makes
this not thread-safe, so having it use strerror_r to fill that
buffer is just putting lipstick on a pig.

It looks like your patch doesn't address that?

On Wed, Jul 02, 2025 at 01:44:55PM +0200, Peter Eisentraut wrote:

Couldn't this also be done by making that global variable thread-local? But
getting rid of it is even nicer.

Getting rid of it like Daniel is proposing is by putting this
information in the backend Port is much more elegant IMO.
--
Michael

On 2 Jul 2025, at 13:44, Peter Eisentraut <peter@eisentraut.org> wrote:

On 27.06.25 19:24, Daniel Gustafsson wrote:

The OpenSSL code in libpq have two issues for multithreading: the verify_cb
callback use a global variable to pass back error detail state and there is one
use of strerror().

Slightly misleading title: This is actually about the *backend* libpq code.

Point taken, proposed commitmessage has been updated.

The attached fixes both, with no functional change, in order to pave the way
for multithreading:
* Rather than using a global variable the callback use a new member in the
Port struct for passing the string, and the Port struct is in turn passed as
private data in the SSL object

Couldn't this also be done by making that global variable thread-local? But getting rid of it is even nicer.

It absolutely could, and I briefly considered this approach, but I prefer
getting rid of it altogether.

* The strerror call is replaced with a strerror_r call using the already
existing errorbuffer

This one was already discussed some time ago at </messages/by-id/daa87d79-c044-46c4-8458-8d77241ed7b0@eisentraut.org&gt;:

But the bigger issue is that the use of a static buffer makes
this not thread-safe, so having it use strerror_r to fill that
buffer is just putting lipstick on a pig.

It looks like your patch doesn't address that?

Doh, of course. The attached v2 takes a stab at moving from using a static
buffer to a palloced buffer with the caller being responsible for freeing. In
paths which lead to proc_exit we can omit freeing.

--
Daniel Gustafsson

On 03.07.25 18:01, Daniel Gustafsson wrote:

On 2 Jul 2025, at 13:44, Peter Eisentraut <peter@eisentraut.org> wrote:

On 27.06.25 19:24, Daniel Gustafsson wrote:

The OpenSSL code in libpq have two issues for multithreading: the verify_cb
callback use a global variable to pass back error detail state and there is one
use of strerror().

Slightly misleading title: This is actually about the *backend* libpq code.

Point taken, proposed commitmessage has been updated.

The attached fixes both, with no functional change, in order to pave the way
for multithreading:
* Rather than using a global variable the callback use a new member in the
Port struct for passing the string, and the Port struct is in turn passed as
private data in the SSL object

Couldn't this also be done by making that global variable thread-local? But getting rid of it is even nicer.

I suggest that instead of adding the context to the Port structure, make
a separate context struct for this purpose, for example:

struct verify_cb_context
{
const char *cert_errdetail;
};

int
be_tls_open_server(Port *port)
{
static struct verify_cb_context verify_cb_context;

SSL_set_ex_data(port->ssl, 0, &verify_cb_context);

This avoids the "circle motion" your patch describes.

It also avoids questions about whether Port.cert_errdetail is properly
cleaned up whenever be_tls_open_server() exits early.

It absolutely could, and I briefly considered this approach, but I prefer
getting rid of it altogether.

* The strerror call is replaced with a strerror_r call using the already
existing errorbuffer

This one was already discussed some time ago at </messages/by-id/daa87d79-c044-46c4-8458-8d77241ed7b0@eisentraut.org&gt;:

But the bigger issue is that the use of a static buffer makes
this not thread-safe, so having it use strerror_r to fill that
buffer is just putting lipstick on a pig.

It looks like your patch doesn't address that?

Doh, of course. The attached v2 takes a stab at moving from using a static
buffer to a palloced buffer with the caller being responsible for freeing. In
paths which lead to proc_exit we can omit freeing.

This seems like an extremely inconvenient solution, as can be seen by
the amount of changes your patch introduces. We could just make errbuf
thread-local and be done, without having to change the API. (This is
how glibc's strerror() works internally.)

On 1 Sep 2025, at 07:27, Peter Eisentraut <peter@eisentraut.org> wrote:

I suggest that instead of adding the context to the Port structure, make a separate context struct for this purpose, for example:

Fair enough, done in the attached.

This seems like an extremely inconvenient solution, as can be seen by the amount of changes your patch introduces. We could just make errbuf thread-local and be done, without having to change the API. (This is how glibc's strerror() works internally.)

I assume you mean simply leaving it be for now awaiting more thread primitives
to be added to fully support thread local storage? (sidenote; if our thread
local store code will use TLS then be-secure-openssl.c will be challenging to
read =)).

I've left out this portion in the attached and only left the callback private
data change.

--
Daniel Gustafsson

On 22.10.25 10:59, Daniel Gustafsson wrote:

On 1 Sep 2025, at 07:27, Peter Eisentraut <peter@eisentraut.org> wrote:

I suggest that instead of adding the context to the Port structure, make a separate context struct for this purpose, for example:

Fair enough, done in the attached.

This looks good to me. (I would not have the CallbackErr typedef, since
that additional abstraction doesn't buy anything. But it's a small
difference.)

This seems like an extremely inconvenient solution, as can be seen by the amount of changes your patch introduces. We could just make errbuf thread-local and be done, without having to change the API. (This is how glibc's strerror() works internally.)

I assume you mean simply leaving it be for now awaiting more thread primitives
to be added to fully support thread local storage?

Yes

(sidenote; if our thread
local store code will use TLS then be-secure-openssl.c will be challenging to
read =)).

Yes, let's rename it to SSL to avoid this. ;-)

On 29 Oct 2025, at 10:41, Peter Eisentraut <peter@eisentraut.org> wrote:
On 22.10.25 10:59, Daniel Gustafsson wrote:

On 1 Sep 2025, at 07:27, Peter Eisentraut <peter@eisentraut.org> wrote:
I suggest that instead of adding the context to the Port structure, make a separate context struct for this purpose, for example:

Fair enough, done in the attached.

This looks good to me. (I would not have the CallbackErr typedef, since that additional abstraction doesn't buy anything. But it's a small difference.)

Thanks, I removed the typedef and went ahead with this patch.

(sidenote; if our thread
local store code will use TLS then be-secure-openssl.c will be challenging to
read =)).

Yes, let's rename it to SSL to avoid this. ;-)

Touché! =)

--
Daniel Gustafsson