Minor cleanup/future-proofing for pg_saslprep().

Ensure that pg_saslprep() initializes its output argument to NULL in
all failure paths, and then remove the redundant initialization that
some (not all) of its callers did.  This does not fix any live bug,
but it reduces the odds of future bugs of omission.

Also add a comment about why the existing failure-path coding is
adequate.

Back-patch so as to keep the function's API consistent across branches,
again to forestall future bug introduction.

Patch by me, reviewed by Michael Paquier

Discussion: https://postgr.es/m/16558.1536407783@sss.pgh.pa.us

diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c
index 8fbad53fa82c5d81ca457022f026fc51e2f1aa59..e997c9460010585b1f89d58ce3c976301065d4c0 100644 (file)
--- 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 271021550adf056b567baa912c85a0a5ec7a2828..4cf574fed87ad830bcf8fdb105e37f8b4df0ee44 100644 (file)
--- 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 1d9c93711835a6b76a18841802a622cdb254ed4c..603ef4c00206666c0b69dc4d787a3402dbc32729 100644 (file)
--- a/src/interfaces/libpq/fe-auth-scram.c
+++ b/src/interfaces/libpq/fe-auth-scram.c
@@ -477,7 +477,7 @@ build_client_final_message(fe_scram_state *state)
        printfPQExpBuffer(&conn->errorMessage,
                          "channel binding not supported by this build\n");
        return NULL;
-#endif /* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */
+#endif                         /* HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH */
    }
 #ifdef HAVE_PGTLS_GET_PEER_CERTIFICATE_HASH
    else if (conn->ssl_in_use)
@@ -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;