Обсуждение: Re: 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 
<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?

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

--
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
<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.)

> 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,
forexample: 

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.
Wecould just make errbuf thread-local and be done, without having to change the API.  (This is how glibc's strerror()
worksinternally.) 

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,
forexample:
 
> 
> 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.
Wecould just make errbuf thread-local and be done, without having to change the API.  (This is how glibc's strerror()
worksinternally.)
 
> 
> 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