Minor cleanup/future-proofing for pg_saslprep().

tglsfdc · tglsfdc · commit 930b785d40cf · 2018-09-08T18:20:36.000-04:00
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
@@ -382,7 +382,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;
@@ -428,7 +428,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
@@ -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
@@ -621,7 +621,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;

Original file line number	Diff line number	Diff line change
`@@ -621,7 +621,7 @@ verify_server_signature(fe_scram_state *state)`
`621`	`621`	`char *`
`622`	`622`	`pg_fe_scram_build_verifier(const char *password)`
`623`	`623`	`{`
`624`		`- char *prep_password = NULL;`
	`624`	`+ char *prep_password;`
`625`	`625`	`pg_saslprep_rc rc;`
`626`	`626`	`char saltbuf[SCRAM_DEFAULT_SALT_LEN];`
`627`	`627`	`char *result;`