Re: Patch: Don't set LoadedSSL unless secure_initialize succeeds

On Thu, May 26, 2022 at 4:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Gurjeet Singh <gurjeet@singh.im> writes:
> > On Thu, May 26, 2022 at 12:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> so maybe those comments in libpq-be.h
> >> should be moved to their respective functions?  In any case, I'm not
> >> excited about having three separate comments covering the same point.
>
> > By 3 locations, I suppose you're referring to the definition of
> > secure_initialize(), extern declaration of be_tls_init(), and the
> > definition of be_tls_init().
>
> No, I was counting the third comment as the one you proposed to add to
> secure_initialize's caller.

I think a comment at that call-site is definitely warranted. Consider
the code as it right now...

    if (EnableSSL)
    {
        (void) secure_initialize(true);
        LoadedSSL = true;
    }

... as a first-time reader.

Reader> This is an important piece of code, not just because it is
called from PostmasterMain(), early in the startup process, but also
because it deals with SSL; it has 'SSL' and 'secure' plastered all
over it. But wait, they don't care what the outcome of this important
function call is??!! I might not have paid much attention to it, but
the call is adorned with '(void)', which (i) attracts my attention
more, and (ii) why are they choosing to throw away the result of such
important function call?! And then, they declare SSL has been
"Loaded"... somebody committed half-written code! Perhaps they we in a
hurry. Perhaps this is a result of an automatic git-merge gone wrong.
Let me dig through the code and see if I can find a vulnerability.
<Many hours later, after learning about Postgres' weird
ereport/error-handling, startup vs. reload, getting bashed on IRC or
elsewhere> Duh, there's nothing wrong here. <Moves on>.

Now, consider the same code, and the ensuing thought-process of the reader:

    if (EnableSSL)
    {
        /* Any failure during SSL initialization here will result in
FATAL error. */
        (void) secure_initialize(true);
        /* ... so here we know for sure that SSL was successfully
initialized. */
        LoadedSSL = true;
    }

Reader> This is an important piece of code, not just because it is
called from PostmasterMain(), early in the startup process, but also
because it deals with SSL; it has 'SSL' and 'secure' plastered all
over it. But wait, they don't care what the outcome of this important
function call is??!! That's okay, because the explanation in the
comment makes sense. <Learns about special-case handling of FATAL and
above> There's nothing wrong here. <Moves on>.

> I think it's not a great idea to add such
> comments to call sites, as they're very likely to not get maintained
> when somebody adjusts the API of the function.  (We have a hard enough
> time getting people to update the comments directly next to the
> function :-(.)

That's unfortunate. But I think we should continue to strive for more
maintainable, readable, extensible code.

> I think what we ought to do here is just move the oddly-placed comments
> in libpq-be.h to be adjacent to the function definitions, as attached.
> (I just deleted the .h comments for the GSSAPI functions, as they seem
> to have adequate comments in their .c file already.)

Please see if anything from my patches is usable. I did not get
clarity around startup vs. reload handling until my previous email, so
there may not be much of use in my patches. But I think a few words
mentioning the difference in resource (re)initialization during
startup vs reload would add a lot of value.

Best regards,
Gurjeet
http://Gurje.et