Subject: Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific

From: Michael Paquier <[email-masked]>
Date: Sun, 18 Jun 2023 17:32:15 +0900

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 
> 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
From 7ede07940011d08f8c0cf05d57b3782f367c0adf Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
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/expected/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ąfoo=>valueą'::hstore;
  "keyąfoo"=>"valueą"
 (1 row)
 
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=>value\u000A'::hstore; -- \n
+     hstore     
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u0009=>value\u0009'::hstore; -- \t
+     hstore     
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000D=>value\u000D'::hstore; -- \r
+     hstore     
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000B=>value\u000B'::hstore; -- \v
+     hstore     
+----------------
+ "key"=>"value"
+(1 row)
+
+SELECT E'key\u000C=>value\u000C'::hstore; -- \f
+     hstore     
+----------------
+ "key"=>"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;
 
+static bool hstore_isspace(char ch);
 static bool hstoreCheckKeyLength(size_t len, HSParser *state);
 static bool hstoreCheckValLength(size_t len, HSParser *state);
 
@@ -82,6 +82,26 @@ prseof(HSParser *state)
 	return false;
 }
 
+/*
+ * 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 == ' ' ||
+		ch == '\t' ||
+		ch == '\n' ||
+		ch == '\r' ||
+		ch == '\v' ||
+		ch == '\f')
+		return true;
+	return false;
+}
 
 #define GV_WAITVAL 0
 #define GV_INVAL 1
@@ -119,7 +139,7 @@ get_val(HSParser *state, bool ignoreeq, bool *escaped)
 			{
 				st = GV_WAITESCIN;
 			}
-			else if (!scanner_isspace((unsigned char) *(state->ptr)))
+			else if (!hstore_isspace((unsigned char) *(state->ptr)))
 			{
 				*(state->cur) = *(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=>value\u0105'::hstore;
 SELECT 'keyą=>valueą'::hstore;
 SELECT 'ą=>ą'::hstore;
 SELECT 'keyąfoo=>valueą'::hstore;
+
+-- More patterns that may depend on isspace() and locales, all discarded.
+SELECT E'key\u000A=>value\u000A'::hstore; -- \n
+SELECT E'key\u0009=>value\u0009'::hstore; -- \t
+SELECT E'key\u000D=>value\u000D'::hstore; -- \r
+SELECT E'key\u000B=>value\u000B'::hstore; -- \v
+SELECT E'key\u000C=>value\u000C'::hstore; -- \f
-- 
2.40.1

---

From: Tom Lane <[email-masked]>
Date: Sun, 18 Jun 2023 12:38:12 -0400

> in macOS.  Evan, what do you think?

FWIW, I think the status quo is fine.  Having hstore do something that
is neither its historical behavior nor aligned with the core parser
doesn't seem like a great idea.  I don't buy this argument that
somebody might be depending on the handling of \v in particular.  It's
not any stronger than the argument that they might be depending on,
say, recognizing no-break space (0xA0) in LATIN1, which the old code
did (probably, depending on platform) and scanner_isspace will not.

If anything, the answer for these concerns is that d522b05c8
should not have been back-patched.  But I'm okay with where we are.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Mon, 19 Jun 2023 08:28:43 +0900

On Sun, Jun 18, 2023 at 12:38:12PM -0400, Tom Lane wrote:
> doesn't seem like a great idea.

Okay.  Fine by me.

> I don't buy this argument that

Another thing that I was wondering, though..  Do you think that there
would be an argument in being stricter in the hstore code regarding
the handling of multi-byte characters with some checks based on
IS_HIGHBIT_SET() when parsing the keys and values?
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Sun, 18 Jun 2023 21:10:59 -0400

> IS_HIGHBIT_SET() when parsing the keys and values?

What have you got in mind?  We should already have validated encoding
correctness before the text ever gets to hstore_in, and I'm not clear
what additional checks would be useful.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Tue, 20 Jun 2023 15:02:02 +0900

> what additional checks would be useful.

I was staring at the hstore parsing code and got the impression that
multi-byte character handling could be improved, but looking closer it
seems that I got that wrong.  Apologies for the noise.
--
Michael

---

From: Evan Jones <[email-masked]>
Date: Tue, 20 Jun 2023 09:04:26 -0400

Thanks for the detailed discussion. To confirm that I've understood
everything:

* Michael's proposed patch to add hstore_isspace() would be a potential
fix: it resolves my original bug, and does not change the behavior of '\v'.
* We believe the change to '\v' is not a problem, and may be an improvement
because it now follows the "core" Postgres parser.

In conclusion: we don't need to make an additional change. Thank you all
for investigating!

My one last suggestion: We *could* revert the backpatching if we are
concerned about this change, but I'm not personally sure that is necessary.
As we discussed, this is an unusual corner case in an "extension" type that
many won't even have enabled.

Evan

---

From: Michael Paquier <[email-masked]>
Date: Wed, 21 Jun 2023 09:02:35 +0900

> many won't even have enabled.

As a whole, I'd like to think that this is an improvement even for
stable branches with these weird isspace() handlings, so I'm OK with
the current status in all the branches.  There's an argument about \v,
IMO, but I won't fight hard for it either even if it would be more
consistent with the way array values are handled.
--
Michael

---

From: Tom Lane <[email-masked]>
Date: Tue, 20 Jun 2023 23:39:31 -0400

Michael Paquier <[email-masked]> writes:
> the current status in all the branches.

Sounds like we're all content with that.

> There's an argument about \v,

I'd be okay with adding \v to the set of whitespace characters in
scan.l and scanner_isspace (and other affected places) for v17.
Don't want to back-patch it though.

			regards, tom lane

---

From: Michael Paquier <[email-masked]>
Date: Wed, 21 Jun 2023 15:49:06 +0900

> Don't want to back-patch it though.

Okay.  No idea where this will lead, but for now I have sent a patch
that adds \v to the parser paths where it would be needed, as far as I
checked:
https://www.postgresql.org/message-id/[email-masked]
--
Michael

---

From: Thomas Munro <[email-masked]>
Date: Tue, 10 Oct 2023 16:17:57 +1300

FTR I ran into a benign case of the phenomenon in this thread when
dealing with row types.  In rowtypes.c, we double-quote stuff
containing spaces, but we detect them by passing individual bytes of
UTF-8 sequences to isspace().  Like macOS, Windows thinks that 0xa0 is
a space when you do that, so for example the Korean character '점'
(code point C810, UTF-8 sequence EC A0 90) gets quotes on Windows but
not on Linux.  That confused a migration/diff tool while comparing
Windows and Linux database servers using that representation.  Not a
big deal, I guess no one ever promised that the format was stable
across platforms, and I don't immediately see a way for anything more
serious to go wrong (though I may lack imagination).  It does seem a
bit weird to be using locale-aware tokenising for a machine-readable
format, and then making sure its behaviour is undefined by feeding it
chopped up bytes.

---

From: Evan Jones <[email-masked]>
Date: Tue, 10 Oct 2023 10:51:10 -0400

Thanks for bringing this up! I just looked at the uses if isspace() in that
file. It looks like it is the usual thing: it is allowing leading or
trailing whitespace when parsing values, or for this "needs quoting" logic
on output. The fix would be the same: this *should* be
using scanner_isspace. This has the same disadvantage: it would change
Postgres's results for some inputs that contain these non-ASCII "space"
characters.

Here is a quick demonstration of this issue, showing that the quoting
behavior is different between these two. Mac OS X with the "default" locale
includes quotes because ą includes  0x85 in its UTF-8 encoding:

postgres=# SELECT ROW('keyą');
   row
----------
 ("keyą")
(1 row)

On Mac OS X with the LANG=C environment variable set, it does not include
quotes:

postgres=# SELECT ROW('keyą');
  row
--------
 (keyą)
(1 row)

---

From: Michael Paquier <[email-masked]>
Date: Wed, 11 Oct 2023 08:34:33 +0900

> includes quotes because ą includes  0x85 in its UTF-8 encoding:

Ugh.  rowtypes.c has reminded me as well of gistfuncs.c in pageinspect
where included columns are printed in a ROW-like fashion.  And it also
uses isspace() when we check if double quotes are needed or not.  So
the use of the quotes would equally depend on what macos thinks is
a correct space in this case.
--
Michael