On 03.07.25 18:01, Daniel Gustafsson wrote:
On 2 Jul 2025, at 13:44, Peter Eisentraut <[email protected]> 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.)
