Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables - Mailing list pgsql-bugs

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Sep 07, 2018 at 04:18:55PM -0400, Tom Lane wrote:
>> I had been studying that when the report first arrived.  I'm not really
>> happy with the coding in pg_saslprep(): it leaves garbage in its output
>> parameter in some code paths, and I think it leaks memory in others.

> Agreed.  Do you want to give it a go?  I can see some of those code
> paths?  Likely you spotted more.

Looking closer, there are no leaks, though I think we could use a comment
about that.  And it still makes me itch that pg_saslprep (mostly) doesn't
worry about clearing *output on failure while only three of its four
callers bother to initialize their variables to null.  That's a recipe
for future bugs.  So I propose the attached cleanup.

We're still no closer to an explanation of Jeremy's failure, though
I'm now pretty sure that pg_saslprep itself isn't the issue.

            regards, tom lane

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8fbad53..e997c94 100644
--- a/src/backend/libpq/auth-scram.c
+++ b/src/backend/libpq/auth-scram.c
@@ -453,7 +453,7 @@ pg_be_scram_exchange(void *opaq, char *input, int inputlen,
 char *
 pg_be_scram_build_verifier(const char *password)
 {
-    char       *prep_password = NULL;
+    char       *prep_password;
     pg_saslprep_rc rc;
     char        saltbuf[SCRAM_DEFAULT_SALT_LEN];
     char       *result;
@@ -499,7 +499,7 @@ scram_verify_plain_password(const char *username, const char *password,
     uint8        stored_key[SCRAM_KEY_LEN];
     uint8        server_key[SCRAM_KEY_LEN];
     uint8        computed_key[SCRAM_KEY_LEN];
-    char       *prep_password = NULL;
+    char       *prep_password;
     pg_saslprep_rc rc;

     if (!parse_scram_verifier(verifier, &iterations, &encoded_salt,
diff --git a/src/common/saslprep.c b/src/common/saslprep.c
index 2710215..4cf574f 100644
--- a/src/common/saslprep.c
+++ b/src/common/saslprep.c
@@ -1081,6 +1081,9 @@ pg_saslprep(const char *input, char **output)
     unsigned char *p;
     pg_wchar   *wp;

+    /* Ensure we return *output as NULL on failure */
+    *output = NULL;
+
     /* Check that the password isn't stupendously long */
     if (strlen(input) > MAX_PASSWORD_LENGTH)
     {
@@ -1112,10 +1115,7 @@ pg_saslprep(const char *input, char **output)
      */
     input_size = pg_utf8_string_len(input);
     if (input_size < 0)
-    {
-        *output = NULL;
         return SASLPREP_INVALID_UTF8;
-    }

     input_chars = ALLOC((input_size + 1) * sizeof(pg_wchar));
     if (!input_chars)
@@ -1246,6 +1246,11 @@ pg_saslprep(const char *input, char **output)
     result = ALLOC(result_size + 1);
     if (!result)
         goto oom;
+
+    /*
+     * There are no error exits below here, so the error exit paths don't need
+     * to worry about possibly freeing "result".
+     */
     p = (unsigned char *) result;
     for (wp = output_chars; *wp; wp++)
     {
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c
index 1d9c937..81531f3 100644
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -747,7 +747,7 @@ verify_server_signature(fe_scram_state *state)
 char *
 pg_fe_scram_build_verifier(const char *password)
 {
-    char       *prep_password = NULL;
+    char       *prep_password;
     pg_saslprep_rc rc;
     char        saltbuf[SCRAM_DEFAULT_SALT_LEN];
     char       *result;