Re: libpq OpenSSL and multithreading

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
<https://www.postgresql.org/message-id/flat/daa87d79-c044-46c4-8458-8d77241ed7b0%40eisentraut.org>:
>>
>>> 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.)