From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Jeremy Evans <code@jeremyevans.net>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #15367: Crash in pg_fe_scram_free when using foreign tables
In-reply-to: <20180907204403.GC32058@paquier.xyz>
References: <153626613985.23143.4743626885618266803@wrigleys.postgresql.org>
 <20180906215836.GP2726@paquier.xyz>
 <20180906222843.GF17425@jeremyevans.local>
 <20180907165103.GH17425@jeremyevans.local>
 <20180907175518.GI17425@jeremyevans.local>
 <20180907181522.GB32058@paquier.xyz> <20016.1536351535@sss.pgh.pa.us>
 <20180907204403.GC32058@paquier.xyz>
Comments: In-reply-to Michael Paquier <michael@paquier.xyz>
	message dated "Fri, 07 Sep 2018 13:44:03 -0700"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <16359.1536407524.0@sss.pgh.pa.us>
Date: Sat, 08 Sep 2018 07:56:23 -0400
Message-ID: <16558.1536407783@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <16359.1536407524.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="make-pg-saslprep-more-bulletproof.patch";
	charset="us-ascii"
Content-ID: <16359.1536407524.2@sss.pgh.pa.us>
Content-Description: make-pg-saslprep-more-bulletproof.patch
Content-Transfer-Encoding: quoted-printable

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 inpu=
tlen,
 char *
 pg_be_scram_build_verifier(const char *password)
 {
-	char	   *prep_password =3D 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, cons=
t char *password,
 	uint8		stored_key[SCRAM_KEY_LEN];
 	uint8		server_key[SCRAM_KEY_LEN];
 	uint8		computed_key[SCRAM_KEY_LEN];
-	char	   *prep_password =3D 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 =3D 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 =3D pg_utf8_string_len(input);
 	if (input_size < 0)
-	{
-		*output =3D NULL;
 		return SASLPREP_INVALID_UTF8;
-	}
 =

 	input_chars =3D ALLOC((input_size + 1) * sizeof(pg_wchar));
 	if (!input_chars)
@@ -1246,6 +1246,11 @@ pg_saslprep(const char *input, char **output)
 	result =3D ALLOC(result_size + 1);
 	if (!result)
 		goto oom;
+
+	/*
+	 * There are no error exits below here, so the error exit paths don't ne=
ed
+	 * to worry about possibly freeing "result".
+	 */
 	p =3D (unsigned char *) result;
 	for (wp =3D output_chars; *wp; wp++)
 	{
diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/f=
e-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 =3D NULL;
+	char	   *prep_password;
 	pg_saslprep_rc rc;
 	char		saltbuf[SCRAM_DEFAULT_SALT_LEN];
 	char	   *result;

------- =_aaaaaaaaaa0--