On Mon, May 23, 2022 at 8:51 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > > Daniel Gustafsson <dan...@yesql.se> writes: > >> On 22 May 2022, at 08:41, Gurjeet Singh <gurj...@singh.im> wrote: > >> The initialization in PostmasterMain() blindly turns on LoadedSSL, > >> irrespective of the outcome of secure_initialize(). > > > This call is invoked with isServerStart set to true so any error in > > secure_initialize should error out with ereport FATAL (in be_tls_init()). > > That > > could be explained in a comment though, which is currently isn't. > > The comments for secure_initialize() and be_tls_init() both explain > this already.
The comments above secure_initialize() do, but there are no comments above be_tls_init(), and nothing in there attempts to explain the FATAL vs. LOG difference. I think it doesn't hurt, and may actively help, if we add a comment at the call-site just alluding to the fact that the function call will not return in case of an error, and that it's safe to assume certain state has been initialized if the function returns. The comments *inside* be_tls_init() attempt to explain allocation of a new SSL_context, but end up making it sound like it's an optimization to prevent memory leak. In the patch proposed a few minutes ago in this thread, I have tried to explain above be_tls_init() the error handling behavior, as well as the reason to retain the active SSL_context, if any. > It's not great that be_tls_init() implements two different error > handling behaviors, perhaps. One could imagine separating those. > But we've pretty much bought into such messes with the very fact > that elog/ereport sometimes return and sometimes not. I don't find the dual mode handling startling; I feel it's common in Postgres code, but it's been a while since I've touched it. What I would love to see improved around ereport() calls in SSL functions would be to shrink the 'ereport(); goto error;' pattern into one statement, so that we don't introduce an accidental "goto fail" bug [1]. [1]: https://nakedsecurity.sophos.com/2014/02/24/anatomy-of-a-goto-fail-apples-ssl-bug-explained-plus-an-unofficial-patch/ Best regards, Gurjeet http://Gurje.et
