Thread: Assert failure with ICU support

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

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

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

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

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

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

Jeff Davis <pgsql@j-davis.com> writes:
> Sounds good, patch attached.

WFM.

            regards, tom lane