On Fri, Feb 23, 2024 at 2:30 AM Daniel Gustafsson <dan...@yesql.se> wrote:
>
> The attached two patches are smaller refactorings to the SASL exchange and
> init
> codepaths which are required for the OAuthbearer work [0]. Regardless of the
> future of that patchset, these refactorings are nice cleanups and can be
> considered in isolation. Another goal is of course to reduce scope of the
> OAuth patchset to make it easier to review.
Thanks for pulling these out! They look good overall, just a few notes below.
In 0001:
> + * SASL_FAILED: The exchance has failed and the connection should be
s/exchance/exchange/
> - if (final && !done)
> + if (final && !(status == SASL_FAILED || status == SASL_COMPLETE))
Since there's not yet a SASL_ASYNC, I wonder if this would be more
readable if it were changed to
if (final && status == SASL_CONTINUE)
to match the if condition shortly after it.
In 0002, at the beginning of pg_SASL_init, the `password` variable now
has an uninitialized code path. The OAuth patchset initializes it to
NULL:
> +++ b/src/interfaces/libpq/fe-auth.c
> @@ -425,7 +425,7 @@ pg_SASL_init(PGconn *conn, int payloadlen)
> int initialresponselen;
> const char *selected_mechanism;
> PQExpBufferData mechanism_buf;
> - char *password;
> + char *password = NULL;
> SASLStatus status;
>
> initPQExpBuffer(&mechanism_buf);
I'll base the next version of the OAuth patchset on top of these.
Thanks!
--Jacob
