Feedback-ID: i0fe9450f:Fastmail
Date: Thu, 19 Mar 2026 13:25:52 +0900
From: Michael Paquier <michael@paquier.xyz>
To: John Naylor <johncnaylorls@gmail.com>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Non-compliant SASLprep implementation for ASCII characters
Message-ID: <abt60Gcc4drBlY1N@paquier.xyz>
References: <aaEJ-El2seZHeFcG@paquier.xyz>
 <CANWCAZbgyvx66qfngNvW0n+uEv3=Bak1X5GLPr4sZApuHDPV-g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="OWQ2F2onObjpvgt0"
Content-Disposition: inline
In-Reply-To: 
 <CANWCAZbgyvx66qfngNvW0n+uEv3=Bak1X5GLPr4sZApuHDPV-g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/abt60Gcc4drBlY1N%40paquier.xyz>
Precedence: bulk


--OWQ2F2onObjpvgt0
Content-Type: multipart/mixed; boundary="sjoVJoQrPjr5b6RX"
Content-Disposition: inline


--sjoVJoQrPjr5b6RX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 18, 2026 at 06:34:03PM +0700, John Naylor wrote:
> Seems sensible to me. I only have minor nitpicks:

Thanks for the review of the module.

> +operation for a single byte as well as a range of these, acting as thin
> +wrappers standing on top of pg_saslprep().
>=20
> It's more natural to say "wrappers around", at least that's what comes to=
 me.

Fixed.

> + if (unlikely(utf8_len =3D=3D 0))
>=20
> The exceptional path only has two lines of code, so it's unclear what
> this hint is trying to do. This module isn't run by default anyway

Removed that.

> + MemSet(nulls, false, sizeof(nulls));
>=20
> Regular "memset" with a 4-byte constant input is easily inline-able by
> the compiler, and I think we should use our homegrown implementation
> only when there is a specific reason for it. (I know there are many
> dozens of uses without a reason already, but...)

Removed that.

> -is($result, 'U+0000|SUCCESS|\x00|\x', "Only nul authorized for all
> valid UTF8 codepoints");
> +is($result, '', "No empty or NULL values for all valid UTF8 codepoints");
>=20
> I don't quite understand "only nul authorized..." -- I understand the
> explanation in your email, but I having difficulty with the way it's
> phrased here. (Although it'll be moot if we go ahead with 0002)

Yes, still better to keep the state of the tree cleaner at all times,
especially if 0002 gets reverted.  I have used a simpler "valid
codepoints returning an empty password".

Applied the result for the module, to have at least the coverage part.
The last piece is refreshed, and attached for now.
--
Michael

--sjoVJoQrPjr5b6RX
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename=v3-0001-Make-implementation-of-SASLprep-compliant-for-ASC.patch
Content-Transfer-Encoding: quoted-printable

=46rom a806e48445ee7d9d75dbe70e0da76b703650faa4 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Thu, 19 Mar 2026 13:18:28 +0900
Subject: [PATCH v3] Make implementation of SASLprep compliant for ASCII
 characters

---
 src/common/saslprep.c                         | 12 ----
 .../test_saslprep/expected/test_saslprep.out  | 66 +++++++++----------
 .../test_saslprep/t/001_saslprep_ranges.pl    |  4 +-
 3 files changed, 34 insertions(+), 48 deletions(-)

diff --git a/src/common/saslprep.c b/src/common/saslprep.c
index 2ad2cefc14fb..38d50dd823c4 100644
--- a/src/common/saslprep.c
+++ b/src/common/saslprep.c
@@ -1061,18 +1061,6 @@ pg_saslprep(const char *input, char **output)
 	/* Ensure we return *output as NULL on failure */
 	*output =3D NULL;
=20
-	/*
-	 * Quick check if the input is pure ASCII.  An ASCII string requires no
-	 * further processing.
-	 */
-	if (pg_is_ascii(input))
-	{
-		*output =3D STRDUP(input);
-		if (!(*output))
-			goto oom;
-		return SASLPREP_SUCCESS;
-	}
-
 	/*
 	 * Convert the input from UTF-8 to an array of Unicode codepoints.
 	 *
diff --git a/src/test/modules/test_saslprep/expected/test_saslprep.out b/sr=
c/test/modules/test_saslprep/expected/test_saslprep.out
index f72dbffa0a11..92f93365343e 100644
--- a/src/test/modules/test_saslprep/expected/test_saslprep.out
+++ b/src/test/modules/test_saslprep/expected/test_saslprep.out
@@ -19,38 +19,38 @@ SELECT
   FROM generate_series(0,127) AS a;
    dat    | byt  |     saslprep     =20
 ----------+------+-------------------
- <NUL>    | \x00 | ("\\x",SUCCESS)
- <CTL_1>  | \x01 | ("\\x01",SUCCESS)
- <CTL_2>  | \x02 | ("\\x02",SUCCESS)
- <CTL_3>  | \x03 | ("\\x03",SUCCESS)
- <CTL_4>  | \x04 | ("\\x04",SUCCESS)
- <CTL_5>  | \x05 | ("\\x05",SUCCESS)
- <CTL_6>  | \x06 | ("\\x06",SUCCESS)
- <CTL_7>  | \x07 | ("\\x07",SUCCESS)
- <CTL_8>  | \x08 | ("\\x08",SUCCESS)
- <CTL_9>  | \x09 | ("\\x09",SUCCESS)
- <CTL_10> | \x0a | ("\\x0a",SUCCESS)
- <CTL_11> | \x0b | ("\\x0b",SUCCESS)
- <CTL_12> | \x0c | ("\\x0c",SUCCESS)
- <CTL_13> | \x0d | ("\\x0d",SUCCESS)
- <CTL_14> | \x0e | ("\\x0e",SUCCESS)
- <CTL_15> | \x0f | ("\\x0f",SUCCESS)
- <CTL_16> | \x10 | ("\\x10",SUCCESS)
- <CTL_17> | \x11 | ("\\x11",SUCCESS)
- <CTL_18> | \x12 | ("\\x12",SUCCESS)
- <CTL_19> | \x13 | ("\\x13",SUCCESS)
- <CTL_20> | \x14 | ("\\x14",SUCCESS)
- <CTL_21> | \x15 | ("\\x15",SUCCESS)
- <CTL_22> | \x16 | ("\\x16",SUCCESS)
- <CTL_23> | \x17 | ("\\x17",SUCCESS)
- <CTL_24> | \x18 | ("\\x18",SUCCESS)
- <CTL_25> | \x19 | ("\\x19",SUCCESS)
- <CTL_26> | \x1a | ("\\x1a",SUCCESS)
- <CTL_27> | \x1b | ("\\x1b",SUCCESS)
- <CTL_28> | \x1c | ("\\x1c",SUCCESS)
- <CTL_29> | \x1d | ("\\x1d",SUCCESS)
- <CTL_30> | \x1e | ("\\x1e",SUCCESS)
- <CTL_31> | \x1f | ("\\x1f",SUCCESS)
+ <NUL>    | \x00 | (,PROHIBITED)
+ <CTL_1>  | \x01 | (,PROHIBITED)
+ <CTL_2>  | \x02 | (,PROHIBITED)
+ <CTL_3>  | \x03 | (,PROHIBITED)
+ <CTL_4>  | \x04 | (,PROHIBITED)
+ <CTL_5>  | \x05 | (,PROHIBITED)
+ <CTL_6>  | \x06 | (,PROHIBITED)
+ <CTL_7>  | \x07 | (,PROHIBITED)
+ <CTL_8>  | \x08 | (,PROHIBITED)
+ <CTL_9>  | \x09 | (,PROHIBITED)
+ <CTL_10> | \x0a | (,PROHIBITED)
+ <CTL_11> | \x0b | (,PROHIBITED)
+ <CTL_12> | \x0c | (,PROHIBITED)
+ <CTL_13> | \x0d | (,PROHIBITED)
+ <CTL_14> | \x0e | (,PROHIBITED)
+ <CTL_15> | \x0f | (,PROHIBITED)
+ <CTL_16> | \x10 | (,PROHIBITED)
+ <CTL_17> | \x11 | (,PROHIBITED)
+ <CTL_18> | \x12 | (,PROHIBITED)
+ <CTL_19> | \x13 | (,PROHIBITED)
+ <CTL_20> | \x14 | (,PROHIBITED)
+ <CTL_21> | \x15 | (,PROHIBITED)
+ <CTL_22> | \x16 | (,PROHIBITED)
+ <CTL_23> | \x17 | (,PROHIBITED)
+ <CTL_24> | \x18 | (,PROHIBITED)
+ <CTL_25> | \x19 | (,PROHIBITED)
+ <CTL_26> | \x1a | (,PROHIBITED)
+ <CTL_27> | \x1b | (,PROHIBITED)
+ <CTL_28> | \x1c | (,PROHIBITED)
+ <CTL_29> | \x1d | (,PROHIBITED)
+ <CTL_30> | \x1e | (,PROHIBITED)
+ <CTL_31> | \x1f | (,PROHIBITED)
           | \x20 | ("\\x20",SUCCESS)
  !        | \x21 | ("\\x21",SUCCESS)
  "        | \x22 | ("\\x22",SUCCESS)
@@ -146,7 +146,7 @@ SELECT
  |        | \x7c | ("\\x7c",SUCCESS)
  }        | \x7d | ("\\x7d",SUCCESS)
  ~        | \x7e | ("\\x7e",SUCCESS)
- <DEL>    | \x7f | ("\\x7f",SUCCESS)
+ <DEL>    | \x7f | (,PROHIBITED)
 (128 rows)
=20
 DROP EXTENSION test_saslprep;
diff --git a/src/test/modules/test_saslprep/t/001_saslprep_ranges.pl b/src/=
test/modules/test_saslprep/t/001_saslprep_ranges.pl
index b353017c0651..4a7cb5aaa588 100644
--- a/src/test/modules/test_saslprep/t/001_saslprep_ranges.pl
+++ b/src/test/modules/test_saslprep/t/001_saslprep_ranges.pl
@@ -25,14 +25,12 @@ $node->safe_psql('postgres', 'CREATE EXTENSION test_sas=
lprep;');
 # Among all the valid UTF-8 codepoint ranges, our implementation of
 # SASLprep should never return an empty password if the operation is
 # considered a success.
-# The only exception is currently the nul character, prohibited in
-# input of CREATE/ALTER ROLE.
 my $result =3D $node->safe_psql(
 	'postgres', qq[SELECT * FROM test_saslprep_ranges()
   WHERE status =3D 'SUCCESS' AND res IN (NULL, '')
 ]);
=20
-is($result, 'U+0000|SUCCESS|\x00|\x', "valid codepoints returning an empty=
 password");
+is($result, '', "valid codepoints returning an empty password");
=20
 $node->stop;
 done_testing();
--=20
2.53.0


--sjoVJoQrPjr5b6RX--

--OWQ2F2onObjpvgt0
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmm7etAACgkQnvQgOdby
QH1Y2Q/+J24sp8xlgIoECv8Ap1Oq12Wmh+pN2GSxAg8Na/Y1IccEeXya1iUE/Dev
MTtc7EGBAJ/i+bHQScO4OOg1xCYZcpACP1jjMG90eb7segv+7+u3rgmLwjnz1kvC
eCcqqoTnkHlmHmvJUagxMl2lHDJaRwL8bJAAwtLG15TJBrRJGLcCC4Q09ioZnEO4
YKa7QFjRcWxSGGGORtnbcqW0g87pTGz/qUL8DhvDbFakOZfL7W7bNbEBPV/TaYxe
2HAz1bzic/NLpt9oCOxFRqKe/Gun5aeDG5UuBQs/5HtC6t+Zub6SwKkFWK+BWzgw
N5G9k4KHaOu31St0xnf0UKmbO9jnINepekm007aOhXvvHLs17KbH/Kp380SAcLs9
v5zVoUDw9fdtAqWE3ahuVy86f+0VfkF61GAh5OGZ2NhWFRnpcFRtysfkq9AhVzUa
5OyxL4F3QoKJrJZ8eTzYqJUvLlk+b+vTRww/VNCgqQuat3Q/EgbkRw0AVtNbGXaP
t5+QNxb9cl/F0i60pQIqMDo2Cf7VTZeq/gZn3qM0h+mHfxPIKIzhF8KZE1adWqJE
Oar19bJ+oNAnMy/xOai78+t0Ol6eH539G9OLgMjHtRhWEfJ0KgqG3BcrtP9SqiM5
kUrFgfb18CC4t7A2KzJVyOeunh+dxlnY2KWBQbHQMykInrXrYsE=
=ZPNV
-----END PGP SIGNATURE-----

--OWQ2F2onObjpvgt0--