pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedAssert failure with ICU support
8+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-04-21 c04c6c5d6f 109363de0a dde926b0f6 79a66c617a 5bcb15b816 106a1bf824 Avoid character classification in regex escape parsing.
* Assert failure with ICU support
@ 2023-04-19 10:30 Richard Guo <guofenglinux@gmail.com>
0 siblings, 2 replies; 8+ messages in thread
From: Richard Guo @ 2023-04-19 10:30 UTC (permalink / raw)
To: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
I happened to run into an assert failure by chance with ICU support.
Here is the query:
SELECT '1' SIMILAR TO '\൧';
The failure happens in lexescape(),
default:
assert(iscalpha(c));
FAILW(REG_EESCAPE); /* unknown alphabetic escape */
break;
Without ICU support, the same query just gives an error.
# SELECT '1' SIMILAR TO '\൧';
ERROR: invalid regular expression: invalid escape \ sequence
FWIW, I googled a bit and '൧' seems to be number 1 in Malayalam.
Thanks
Richard
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-19 15:42 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 0 replies; 8+ messages in thread
From: Tom Lane @ 2023-04-19 15:42 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Richard Guo <guofenglinux@gmail.com> writes:
> I happened to run into an assert failure by chance with ICU support.
> Here is the query:
> SELECT '1' SIMILAR TO '\൧';
> The failure happens in lexescape(),
> default:
> assert(iscalpha(c));
> FAILW(REG_EESCAPE); /* unknown alphabetic escape */
> break;
> Without ICU support, the same query just gives an error.
Interesting.
> FWIW, I googled a bit and '൧' seems to be number 1 in Malayalam.
The code in lexescape() is assuming that if "c" passes
iscalnum(), then either it's '0'-'9' or it passes iscalpha().
This is clearly wrong in Unicode-land, which has non-ASCII digits.
I imagine you can find libc locales where this fails, not only ICU.
I think the question here is what we want to do with such cases:
throw a regex syntax error, or just return the character as-is?
The fine manual says that if the character after '\' is
alphanumeric, it's an escape, and otherwise the character is
quoted literally. But how shall we interpret "alphanumeric"?
I'm kind of inclined to the idea that anything that's not ASCII
should be considered to be literally quoted by '\', rather than
being an erroneous regex escape. Maybe I'm too English-centric.
But I don't like the idea that what is a valid regex should vary
depending on locale.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-19 20:31 Jeff Davis <pgsql@j-davis.com>
parent: Richard Guo <guofenglinux@gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2023-04-19 20:31 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Wed, 2023-04-19 at 18:30 +0800, Richard Guo wrote:
> I happened to run into an assert failure by chance with ICU support.
> Here is the query:
>
> SELECT '1' SIMILAR TO '\൧';
>
> The failure happens in lexescape(),
>
> default:
> assert(iscalpha(c));
> FAILW(REG_EESCAPE); /* unknown alphabetic escape */
> break;
>
> Without ICU support, the same query just gives an error.
>
> # SELECT '1' SIMILAR TO '\൧';
> ERROR: invalid regular expression: invalid escape \ sequence
>
> FWIW, I googled a bit and '൧' seems to be number 1 in Malayalam.
Thank you for the report and analysis! The problem exists all the way
back if you do:
SELECT '1' COLLATE "en-US-x-icu" SIMILAR TO '\൧';
The root cause (which you allude to) is that the code makes the
assumption that digits only include 0-9, but u_isdigit('൧') == true,
which violates that assumption.
For Linux[1] specifically, it seems that the assumption should hold for
iswdigit(). But looking here[2], it seems that the behavior of
iswdigit() depends on the locale and I don't think it's correct to make
that assumption.
I did some experimentation on ICU and I found (pseudocode -- the real
code needs to create a UChar32 from an encoded string first):
char name: MALAYALAM DIGIT ONE
u_isalnum('൧'): true
u_isalpha('൧'): false
u_isdigit('൧'): true
u_charType('൧') == U_DECIMAL_DIGIT_NUMBER: true
u_hasBinaryProperty('൧', UCHAR_POSIX_XDIGIT): true
u_hasBinaryProperty('൧', UCHAR_POSIX_ALNUM): true
The docs[3] for ICU say:
"There are also functions that provide easy migration from C/POSIX
functions like isblank(). Their use is generally discouraged because
the C/POSIX standards do not define their semantics beyond the ASCII
range, which means that different implementations exhibit very
different behavior. Instead, Unicode properties should be used
directly."
We should probably just check that it's plain ASCII.
Unfortunately I would not be surprised if there are more bugs similar
to this one.
Regards,
Jeff Davis
[1] https://man7.org/linux/man-pages/man3/iswdigit.3.html
[2]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/iswdigit.html
[3]
https://unicode-org.github.io/icu-docs/apidoc/dev/icu4c/uchar_8h.html#details
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-19 20:45 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2023-04-19 20:45 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Jeff Davis <pgsql@j-davis.com> writes:
> We should probably just check that it's plain ASCII.
That was about the same conclusion I came to. More or less
c = *v->now++;
- if (!iscalnum(c))
+ if (!isascii(c) || !iscalnum(c))
RETV(PLAIN, c);
although I'm not sure if it's a good idea to apply isascii()
to something that's wider than char. It might be advisable,
if ugly, to write
+ if (c >= 0x100 || !iscalnum(c))
I'm also inclined to remove that "assert(iscalpha(c))" in
the switch. That's not checking sanity of our own code,
just consistency of the <wctype.h> functions; and as this
episode shows it's more trouble than it's worth.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-20 19:47 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2023-04-20 19:47 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Wed, 2023-04-19 at 16:45 -0400, Tom Lane wrote:
> + if (c >= 0x100 || !iscalnum(c))
I'm curious why you say >= 0x100 rather than >= 0x80?
What's the purpose of the error? Is it to catch mistakes, or is it to
reserve room for adding new escape sequences in the future?
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-20 19:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 1 reply; 8+ messages in thread
From: Tom Lane @ 2023-04-20 19:53 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Jeff Davis <pgsql@j-davis.com> writes:
> On Wed, 2023-04-19 at 16:45 -0400, Tom Lane wrote:
>> + if (c >= 0x100 || !iscalnum(c))
> I'm curious why you say >= 0x100 rather than >= 0x80?
Right, should be 0x80, my thinko.
> What's the purpose of the error? Is it to catch mistakes, or is it to
> reserve room for adding new escape sequences in the future?
As I read it, it's meant to leave room for defining more escapes.
If we allowed \x for any non-currently-defined "x" to just be "x",
then there would be a compatibility problem if we wanted to make
it mean something else. But I think it's sufficient to reserve
the ASCII letters for that purpose.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Assert failure with ICU support
@ 2023-04-20 23:30 Jeff Davis <pgsql@j-davis.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 8+ messages in thread
From: Jeff Davis @ 2023-04-20 23:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
On Thu, 2023-04-20 at 15:53 -0400, Tom Lane wrote:
> As I read it, it's meant to leave room for defining more escapes.
> If we allowed \x for any non-currently-defined "x" to just be "x",
> then there would be a compatibility problem if we wanted to make
> it mean something else. But I think it's sufficient to reserve
> the ASCII letters for that purpose.
Sounds good, patch attached.
Regards,
Jeff Davis
Attachments:
0001-Avoid-character-classification-in-regex-escape-parsi.patch (text/x-patch, 1.6K) download
From b933b04d68cdac098c7eeb32b88707f3be385fe6 Mon Sep 17 00:00:00 2001
From: Jeff Davis <jeff@j-davis.com>
Date: Thu, 20 Apr 2023 13:17:09 -0700
Subject: [PATCH] Avoid character classification in regex escape parsing.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
For regex escape sequences, just test directly for the relevant ASCII
characters rather than using locale-sensitive character
classification.
This fixes an assertion failure when a locale considers a non-ASCII
character, such as "൧", to be a digit.
Reported-by: Richard Guo
Discussion: https://postgr.es/m/CAMbWs49Q6UoKGeT8pBkMtJGJd+16CBFZaaWUk9Du+2ERE5g_YA@mail.gmail.com
---
src/backend/regex/regc_lex.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/src/backend/regex/regc_lex.c b/src/backend/regex/regc_lex.c
index 4780d79f09..38c09b1123 100644
--- a/src/backend/regex/regc_lex.c
+++ b/src/backend/regex/regc_lex.c
@@ -613,7 +613,11 @@ lexescape(struct vars *v)
assert(!ATEOS());
c = *v->now++;
- if (!iscalnum(c))
+
+ /* if it's not alphanumeric ASCII, treat it as a plain character */
+ if (!('a' <= c && c <= 'z') &&
+ !('A' <= c && c <= 'Z') &&
+ !('0' <= c && c <= '9'))
RETV(PLAIN, c);
NOTE(REG_UNONPOSIX);
@@ -755,8 +759,11 @@ lexescape(struct vars *v)
RETV(PLAIN, c);
break;
default:
- assert(iscalpha(c));
- FAILW(REG_EESCAPE); /* unknown alphabetic escape */
+ /*
+ * Throw an error for unrecognized ASCII alpha escape sequences,
+ * which reserves them for future use if needed.
+ */
+ FAILW(REG_EESCAPE);
break;
}
assert(NOTREACHED);
--
2.34.1
* Re: Assert failure with ICU support
@ 2023-04-20 23:31 Tom Lane <tgl@sss.pgh.pa.us>
parent: Jeff Davis <pgsql@j-davis.com>
0 siblings, 0 replies; 8+ messages in thread
From: Tom Lane @ 2023-04-20 23:31 UTC (permalink / raw)
To: Jeff Davis <pgsql@j-davis.com>; +Cc: Richard Guo <guofenglinux@gmail.com>; PostgreSQL mailing lists <pgsql-bugs@lists.postgresql.org>
Jeff Davis <pgsql@j-davis.com> writes:
> Sounds good, patch attached.
WFM.
regards, tom lane
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-04-20 23:31 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-04-19 10:30 Assert failure with ICU support Richard Guo <guofenglinux@gmail.com>
2023-04-19 15:42 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-19 20:31 ` Jeff Davis <pgsql@j-davis.com>
2023-04-19 20:45 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-20 19:47 ` Jeff Davis <pgsql@j-davis.com>
2023-04-20 19:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-04-20 23:30 ` Jeff Davis <pgsql@j-davis.com>
2023-04-20 23:31 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox