public inbox for [email protected]
help / color / mirror / Atom feedFrom: Michael Paquier <[email protected]>
To: John Naylor <[email protected]>
Cc: Postgres hackers <[email protected]>
Subject: Re: Non-compliant SASLprep implementation for ASCII characters
Date: Thu, 19 Mar 2026 13:25:52 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <CANWCAZbgyvx66qfngNvW0n+uEv3=Bak1X5GLPr4sZApuHDPV-g@mail.gmail.com>
References: <[email protected]>
<CANWCAZbgyvx66qfngNvW0n+uEv3=Bak1X5GLPr4sZApuHDPV-g@mail.gmail.com>
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().
>
> It's more natural to say "wrappers around", at least that's what comes to me.
Fixed.
> + if (unlikely(utf8_len == 0))
>
> 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));
>
> 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");
>
> 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
From a806e48445ee7d9d75dbe70e0da76b703650faa4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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 = NULL;
- /*
- * Quick check if the input is pure ASCII. An ASCII string requires no
- * further processing.
- */
- if (pg_is_ascii(input))
- {
- *output = 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/src/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
----------+------+-------------------
- <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)
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_saslprep;');
# 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 = $node->safe_psql(
'postgres', qq[SELECT * FROM test_saslprep_ranges()
WHERE status = 'SUCCESS' AND res IN (NULL, '')
]);
-is($result, 'U+0000|SUCCESS|\x00|\x', "valid codepoints returning an empty password");
+is($result, '', "valid codepoints returning an empty password");
$node->stop;
done_testing();
--
2.53.0
Attachments:
[text/plain] v3-0001-Make-implementation-of-SASLprep-compliant-for-ASC.patch (5.1K, 2-v3-0001-Make-implementation-of-SASLprep-compliant-for-ASC.patch)
download | inline diff:
From a806e48445ee7d9d75dbe70e0da76b703650faa4 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email protected]>
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 = NULL;
- /*
- * Quick check if the input is pure ASCII. An ASCII string requires no
- * further processing.
- */
- if (pg_is_ascii(input))
- {
- *output = 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/src/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
----------+------+-------------------
- <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)
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_saslprep;');
# 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 = $node->safe_psql(
'postgres', qq[SELECT * FROM test_saslprep_ranges()
WHERE status = 'SUCCESS' AND res IN (NULL, '')
]);
-is($result, 'U+0000|SUCCESS|\x00|\x', "valid codepoints returning an empty password");
+is($result, '', "valid codepoints returning an empty password");
$node->stop;
done_testing();
--
2.53.0
[application/pgp-signature] signature.asc (833B, 3-signature.asc)
download
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Non-compliant SASLprep implementation for ASCII characters
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox