From 86d226ae5a3607b8bbde2487f821b8f8807da1c8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 8 Sep 2018 18:20:36 -0400 Subject: [PATCH] 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 --- src/backend/libpq/auth-scram.c | 4 ++-- src/common/saslprep.c | 11 ++++++++--- src/interfaces/libpq/fe-auth-scram.c | 4 ++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 8fbad53fa8..e997c94600 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 271021550a..4cf574fed8 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 1d9c937118..603ef4c002 100644 --- 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; -- 2.40.0