From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Peter Eisentraut <peter@eisentraut.org>
Subject: ACL identifier quoting has some issues
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3792717.1751492066.0@sss.pgh.pa.us>
Date: Wed, 02 Jul 2025 17:36:12 -0400
Message-ID: <3792884.1751492172@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/3792884.1751492172%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3792717.1751492066.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

Peter pointed out at [1] that acl.c's getid() behaves oddly
when presented with a string of just two double quotes ("").
If that has any sane interpretation it's as an empty string,
but what you got was a single double quote.

While looking at this I realized that there's another problem:
if the string contains any non-ASCII characters then we will
blindly apply isalnum() to byte(s) with the high bit set,
which will have encoding-dependent, locale-dependent,
and perhaps platform-dependent results.  This could easily
result in putid() electing not to quote some string that,
later in some other environment, getid() will decide is not
a valid identifier, causing dump/reload or similar failures.

So I think we need to apply and back-patch something like
the attached.  Here I've opined that any non-ASCII is safe.
We could invert that and decide that any non-ASCII is unsafe,
but that seems more likely to break existing dumps than this
choice is.

			regards, tom lane

[1] https://www.postgresql.org/message-id/ee96443a-72f3-4a12-8ba7-326069fd=
1c14%40eisentraut.org


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="fix-ACL-quoting-logic.patch";
	charset="us-ascii"
Content-ID: <3792717.1751492066.2@sss.pgh.pa.us>
Content-Description: fix-ACL-quoting-logic.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index ca3c5ee3df3..bd2435198e1 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -134,6 +134,16 @@ static AclResult pg_role_aclcheck(Oid role_oid, Oid r=
oleid, AclMode mode);
 static void RoleMembershipCacheCallback(Datum arg, int cacheid, uint32 ha=
shvalue);
 =

 =

+/*
+ * Test whether an identifier char can be left unquoted in ACLs
+ */
+static inline bool
+is_safe_acl_char(unsigned char c)
+{
+	/* We don't trust isalnum() to deliver consistent results for non-ASCII =
*/
+	return IS_HIGHBIT_SET(c) || isalnum(c) || c =3D=3D '_';
+}
+
 /*
  * getid
  *		Consumes the first alphanumeric string (identifier) found in string
@@ -162,18 +172,20 @@ getid(const char *s, char *n, Node *escontext)
 	/* This code had better match what putid() does, below */
 	for (;
 		 *s !=3D '\0' &&
-		 (isalnum((unsigned char) *s) ||
-		  *s =3D=3D '_' ||
-		  *s =3D=3D '"' ||
-		  in_quotes);
+		 (in_quotes || is_safe_acl_char(*s) || *s =3D=3D '"');
 		 s++)
 	{
 		if (*s =3D=3D '"')
 		{
+			if (!in_quotes)
+			{
+				in_quotes =3D true;
+				continue;
+			}
 			/* safe to look at next char (could be '\0' though) */
 			if (*(s + 1) !=3D '"')
 			{
-				in_quotes =3D !in_quotes;
+				in_quotes =3D false;
 				continue;
 			}
 			/* it's an escaped double quote; skip the escaping char */
@@ -210,7 +222,7 @@ putid(char *p, const char *s)
 	for (src =3D s; *src; src++)
 	{
 		/* This test had better match what getid() does, above */
-		if (!isalnum((unsigned char) *src) && *src !=3D '_')
+		if (!is_safe_acl_char(*src))
 		{
 			safe =3D false;
 			break;

------- =_aaaaaaaaaa0--