Feedback-ID: i0fe9450f:Fastmail
Date: Sun, 18 Jun 2023 17:32:15 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Evan Jones <evan.jones@datadoghq.com>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale
 specific
Message-ID: <ZI7BD6jTsn4vdCxe@paquier.xyz>
References: <CA+HWA9awUW0+RV_gO9r1ABZwGoZxPztcJxPy8vMFSTbTfi4jig@mail.gmail.com>
 <ZH8aia7JfdPjTKAY@paquier.xyz>
 <CA+HWA9bET27MxdO4VOgK==w4P_G9H0aStCq+C_DP4YnarAw87g@mail.gmail.com>
 <ZIZkMf6rfCVQaKgO@paquier.xyz>
 <CA+HWA9bTRDf52DHyU+JOoqEALgRGRo5uHUYTFuduoj3cBfer+Q@mail.gmail.com>
 <ZI5i2NCWirDuE/Iq@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="7iSnB+6tWp20VWO2"
Content-Disposition: inline
In-Reply-To: <ZI5i2NCWirDuE/Iq@paquier.xyz>
Archived-At: <https://www.postgresql.org/message-id/ZI7BD6jTsn4vdCxe%40paquier.xyz>
Precedence: bulk


--7iSnB+6tWp20VWO2
Content-Type: multipart/mixed; boundary="QaCgPg+7zdmvoevL"
Content-Disposition: inline


--QaCgPg+7zdmvoevL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sun, Jun 18, 2023 at 10:50:16AM +0900, Michael Paquier wrote:
> The difference between scanner_isspace() and array_isspace() is that
> the former matches with what scan.l stores as rules for whitespace
> characters, but the latter works on values.  For hstore, we want the
> latter, with something that works on values.  To keep the change
> locale to hstore, I think that we should just introduce an
> hstore_isspace() which is a copy of array_isspace.  That's a
> duplication, sure, but I think that we may want to think harder about
> \v in the flex scanner, and that's just a few extra lines for=20
> something that has not changed in 13 years for arrays.  That's also
> easier to think about for stable branches.  If you can send a patch,
> that helps a lot, for sure!

At the end, no need to do that.  I have been able to hack the
attached, that shows the difference of treatment for \v when running
in macOS.  Evan, what do you think?
--
Michael

--QaCgPg+7zdmvoevL
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="0001-More-fixes-for-hstore-and-locales.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 7ede07940011d08f8c0cf05d57b3782f367c0adf Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@paquier.xyz>
Date: Sun, 18 Jun 2023 17:29:37 +0900
Subject: [PATCH] More fixes for hstore and locales

This is not really complete without the treatment of \v like array
values.
---
 contrib/hstore/expected/hstore_utf8.out | 31 +++++++++++++++++++++++++
 contrib/hstore/hstore_io.c              | 30 ++++++++++++++++++++----
 contrib/hstore/sql/hstore_utf8.sql      |  7 ++++++
 3 files changed, 63 insertions(+), 5 deletions(-)

diff --git a/contrib/hstore/expected/hstore_utf8.out b/contrib/hstore/expec=
ted/hstore_utf8.out
index 4405824413..bbc885a181 100644
--- a/contrib/hstore/expected/hstore_utf8.out
+++ b/contrib/hstore/expected/hstore_utf8.out
@@ -34,3 +34,34 @@ SELECT 'key=C4=85foo=3D>value=C4=85'::hstore;
  "key=C4=85foo"=3D>"value=C4=85"
 (1 row)
=20
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=3D>value\u000A'::hstore; -- \n
+     hstore    =20
+----------------
+ "key"=3D>"value"
+(1 row)
+
+SELECT E'key\u0009=3D>value\u0009'::hstore; -- \t
+     hstore    =20
+----------------
+ "key"=3D>"value"
+(1 row)
+
+SELECT E'key\u000D=3D>value\u000D'::hstore; -- \r
+     hstore    =20
+----------------
+ "key"=3D>"value"
+(1 row)
+
+SELECT E'key\u000B=3D>value\u000B'::hstore; -- \v
+     hstore    =20
+----------------
+ "key"=3D>"value"
+(1 row)
+
+SELECT E'key\u000C=3D>value\u000C'::hstore; -- \f
+     hstore    =20
+----------------
+ "key"=3D>"value"
+(1 row)
+
diff --git a/contrib/hstore/hstore_io.c b/contrib/hstore/hstore_io.c
index 999ddad76d..f33b241d54 100644
--- a/contrib/hstore/hstore_io.c
+++ b/contrib/hstore/hstore_io.c
@@ -13,7 +13,6 @@
 #include "lib/stringinfo.h"
 #include "libpq/pqformat.h"
 #include "nodes/miscnodes.h"
-#include "parser/scansup.h"
 #include "utils/builtins.h"
 #include "utils/json.h"
 #include "utils/jsonb.h"
@@ -41,6 +40,7 @@ typedef struct
 	int			plen;
 } HSParser;
=20
+static bool hstore_isspace(char ch);
 static bool hstoreCheckKeyLength(size_t len, HSParser *state);
 static bool hstoreCheckValLength(size_t len, HSParser *state);
=20
@@ -82,6 +82,26 @@ prseof(HSParser *state)
 	return false;
 }
=20
+/*
+ * hstore_isspace() --- a non-locale-dependent isspace()
+ *
+ * We used to use isspace() for parsing hstore values, but that has
+ * undesirable results: a hstore value might be silently interpreted
+ * differently depending on the locale setting.  Now we just hard-wire
+ * the traditional ASCII definition of isspace().
+ */
+static bool
+hstore_isspace(char ch)
+{
+	if (ch =3D=3D ' ' ||
+		ch =3D=3D '\t' ||
+		ch =3D=3D '\n' ||
+		ch =3D=3D '\r' ||
+		ch =3D=3D '\v' ||
+		ch =3D=3D '\f')
+		return true;
+	return false;
+}
=20
 #define GV_WAITVAL 0
 #define GV_INVAL 1
@@ -119,7 +139,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
 			{
 				st =3D GV_WAITESCIN;
 			}
-			else if (!scanner_isspace((unsigned char) *(state->ptr)))
+			else if (!hstore_isspace((unsigned char) *(state->ptr)))
 			{
 				*(state->cur) =3D *(state->ptr);
 				state->cur++;
@@ -142,7 +162,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
 				state->ptr--;
 				return true;
 			}
-			else if (scanner_isspace((unsigned char) *(state->ptr)))
+			else if (hstore_isspace((unsigned char) *(state->ptr)))
 			{
 				return true;
 			}
@@ -256,7 +276,7 @@ parse_hstore(HSParser *state)
 			{
 				PRSEOF;
 			}
-			else if (!scanner_isspace((unsigned char) *(state->ptr)))
+			else if (!hstore_isspace((unsigned char) *(state->ptr)))
 			{
 				PRSSYNTAXERROR;
 			}
@@ -310,7 +330,7 @@ parse_hstore(HSParser *state)
 			{
 				return true;
 			}
-			else if (!scanner_isspace((unsigned char) *(state->ptr)))
+			else if (!hstore_isspace((unsigned char) *(state->ptr)))
 			{
 				PRSSYNTAXERROR;
 			}
diff --git a/contrib/hstore/sql/hstore_utf8.sql b/contrib/hstore/sql/hstore=
_utf8.sql
index face878324..38c9481ee6 100644
--- a/contrib/hstore/sql/hstore_utf8.sql
+++ b/contrib/hstore/sql/hstore_utf8.sql
@@ -17,3 +17,10 @@ SELECT E'key\u0105=3D>value\u0105'::hstore;
 SELECT 'key=C4=85=3D>value=C4=85'::hstore;
 SELECT '=C4=85=3D>=C4=85'::hstore;
 SELECT 'key=C4=85foo=3D>value=C4=85'::hstore;
+
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=3D>value\u000A'::hstore; -- \n
+SELECT E'key\u0009=3D>value\u0009'::hstore; -- \t
+SELECT E'key\u000D=3D>value\u000D'::hstore; -- \r
+SELECT E'key\u000B=3D>value\u000B'::hstore; -- \v
+SELECT E'key\u000C=3D>value\u000C'::hstore; -- \f
--=20
2.40.1


--QaCgPg+7zdmvoevL--

--7iSnB+6tWp20VWO2
Content-Type: application/pgp-signature; name="signature.asc"

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmSOwQ8ACgkQnvQgOdby
QH3P6hAAk4184jjffsufPVs5faNlrGY2uGuEa76QGoQnArUU4tAsugWAwveFqpQv
18w0whozaXBkKLTNf9+bGFgVAvR1QyiAWmSLUEKhrEYAVtVDWiKF61tj4LVrNsLc
aN5pYDw7K8lbG626CM/UMzsO8fpAQm/ngRc2loS4k+f5lOubYy3ExgRJNRCgaVuR
LD4k5Ai66N4JZ741SvSMc1ukRQE+7+cING4qaTE0qmrdwejD1N+ofgOzXr6z7WP4
mHJOtYSoKlhftrjXbKYujK3yqgW3br/69c7mU4h5iAsuEqhNlOE9hH2OGJ4S+Ltp
xRwg2uPsVCAMzYMbirsHxsfPO/6oVzwbrIqfaJlSnQzM+nZYlf/rSB9uoEgbBU2f
WYduVeQgAiNU69uipfVhKTGzWbR6tlhU+lE7BqJLlDBI/A03T/L7vnMgN/Bh1NOZ
HoFQGi31vFD7pdc0jkghxB6SiYMrueSB6sooUhqJ5NwDMPnfPxn2yE0g1+D6nzdf
NM1989IJbEIv7QdeVkRlOOcnLbfBRpEuhafoVTT7cxhM286/9c2dmMl5Rgv+W01Y
AVyAWFyEzJSje0s0olhDuK4WP7jzASsrQ9e2oek+2d0alL6rGRj4NCGroJ55MLce
EXae2LKhxQFv4ItQwlqufYg6+WfF6Xhk7e1quVHwkEnimM7sg2g=
=Z2Ma
-----END PGP SIGNATURE-----

--7iSnB+6tWp20VWO2--