Re: [PATCH] Log details for client certificate failures

On Mon, Jul 11, 2022 at 6:09 AM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> I squashed those two together. I also adjusted the error message a bit
> more for project style. (We can put both lines into detail.)

Oh, okay. Log parsers don't have any issues with that?

> I had to read up on this "ex_data" API. Interesting. But I'm wondering
> a bit about how the life cycle of these objects is managed. What
> happens if the allocated error string is deallocated before its
> containing object? Or vice versa?

Yeah, I'm currently leaning heavily on the lack of any memory context
switches here. And I end up leaking out a pointer to the stale stack
of be_tls_open_server(), which is gross -- it works since there are no
other clients, but that could probably come back to bite us.

The ex_data API exposes optional callbacks for new/dup/free (I'm
currently setting those to NULL), so we can run custom code whenever
the SSL* is destroyed. If you'd rather the data have the same lifetime
of the SSL* object, we can switch to malloc/strdup/free (or even
OPENSSL_strdup() in later versions). But since we don't have any use
for the ex_data outside of this function, maybe we should just clear
it before we return, rather than carrying it around.

> How do we ensure we don't
> accidentally reuse the error string when the code runs again? (I guess
> currently it can't?)

The ex_data is associated with the SSL*, not the global SSL_CTX*, so
that shouldn't be an issue. A new SSL* gets created at the start of
be_tls_open_server().

> Maybe we should avoid this and just put the
> errdetail itself into a static variable that we unset once the message
> is printed?

If you're worried about the lifetime of the palloc'd data being too
short, does switching to a static variable help in that case?

--Jacob