Horton

Thread

Commits

  1. Fix Assert failure in XMLTABLE parser

  1. Assert failure in base_yyparse

    Evgeniy Gorbanev <gorbanyoves@basealt.ru> — 2025-03-28T07:39:55Z

    Hello.

Got an assert failure when fuzzing the raw_parser function.
The query to reproduce:
SELECT xmltable.* FROM xmltest2, LATERAL xmltable('/d/r' PASSING x 
COLUMNS a int PATH '' || lower(_path) is_not_null|| 'c');

If I understand correctly, is_not_null is considered as a valid keyword 
in xmltable, but it gets the type T_A_Expr.

Postgres output:
$ ./postgres -D data
2025-03-28 12:19:19.945 +06 [53058] LOG:  starting PostgreSQL 18devel on 
x86_64-pc-linux-gnu, compiled by x86_64-alt-linux-gcc (GCC) 10.3.1 
20210703 (ALT Sisyphus 10.3.1-alt2), 64-bit
2025-03-28 12:19:19.946 +06 [53058] LOG:  listening on IPv4 address 
"127.0.0.1", port 5432
2025-03-28 12:19:20.032 +06 [53058] LOG:  listening on Unix socket 
"/tmp/.s.PGSQL.5432"
2025-03-28 12:19:20.281 +06 [53064] LOG:  database system was shut down 
at 2025-03-28 12:18:37 +06
2025-03-28 12:19:20.342 +06 [53058] LOG:  database system is ready to 
accept connections
TRAP: failed Assert("ptr == NULL || nodeTag(ptr) == type"), File: 
"../../../src/include/nodes/nodes.h", Line: 177, PID: 53069
postgres: user postgres 127.0.0.1(56762) 
idle(ExceptionalCondition+0x141)[0x5561768c0fb6]
postgres: user postgres 127.0.0.1(56762) idle(+0x585784)[0x556175e03784]
postgres: user postgres 127.0.0.1(56762) 
idle(base_yyparse+0x30578)[0x556175e33f83]
postgres: user postgres 127.0.0.1(56762) 
idle(raw_parser+0xf5)[0x556175ec0f47]
postgres: user postgres 127.0.0.1(56762) 
idle(pg_parse_query+0x63)[0x556176550a67]
postgres: user postgres 127.0.0.1(56762) idle(+0xcd3bcf)[0x556176551bcf]
postgres: user postgres 127.0.0.1(56762) 
idle(PostgresMain+0x14ef)[0x55617655ca54]
postgres: user postgres 127.0.0.1(56762) idle(+0xccc207)[0x55617654a207]
postgres: user postgres 127.0.0.1(56762) 
idle(postmaster_child_launch+0x2ae)[0x5561763ad6e8]
postgres: user postgres 127.0.0.1(56762) idle(+0xb3bc75)[0x5561763b9c75]
postgres: user postgres 127.0.0.1(56762) idle(+0xb3686b)[0x5561763b486b]
postgres: user postgres 127.0.0.1(56762) 
idle(PostmasterMain+0x288f)[0x5561763b3bdb]
postgres: user postgres 127.0.0.1(56762) idle(main+0x5dc)[0x55617616dfbd]
/lib64/libc.so.6(__libc_start_main+0xcd)[0x7f36ef05fefd]
postgres: user postgres 127.0.0.1(56762) idle(_start+0x2a)[0x556175b4a39a]
2025-03-28 12:19:24.020 +06 [53058] LOG:  client backend (PID 53069) was 
terminated by signal 6: Aborted
2025-03-28 12:19:24.020 +06 [53058] DETAIL:  Failed process was running: 
SELECT xmltable.* FROM xmltest2, LATERAL xmltable('/d/r' PASSING x 
COLUMNS a int PATH '' || lower(_path) is_not_null|| 'c');
2025-03-28 12:19:24.020 +06 [53058] LOG:  terminating any other active 
server processes
2025-03-28 12:19:24.021 +06 [53058] LOG:  all server processes 
terminated; reinitializing
2025-03-28 12:19:24.220 +06 [53080] LOG:  database system was 
interrupted; last known up at 2025-03-28 12:19:20 +06
2025-03-28 12:19:28.268 +06 [53080] LOG:  database system was not 
properly shut down; automatic recovery in progress
2025-03-28 12:19:28.296 +06 [53080] LOG:  invalid record length at 
0/17864A0: expected at least 24, got 0
2025-03-28 12:19:28.296 +06 [53080] LOG:  redo is not required
2025-03-28 12:19:28.414 +06 [53081] LOG:  checkpoint starting: 
end-of-recovery immediate wait
2025-03-28 12:19:28.561 +06 [53081] LOG:  checkpoint complete: wrote 0 
buffers (0.0%), wrote 3 SLRU buffers; 0 WAL file(s) added, 0 removed, 0 
recycled; write=0.014 s, sync=0.004 s, total=0.183 s; sync files=2, 
longest=0.002 s, average=0.002 s; distance=0 kB, estimate=0 kB; 
lsn=0/17864A0, redo lsn=0/17864A0
2025-03-28 12:19:28.618 +06 [53058] LOG:  database system is ready to 
accept connections
^C2025-03-28 12:19:30.116 +06 [53058] LOG:  received fast shutdown request
2025-03-28 12:19:30.259 +06 [53058] LOG:  aborting any active transactions
2025-03-28 12:19:30.313 +06 [53081] LOG:  shutting down
2025-03-28 12:19:30.464 +06 [53081] LOG:  checkpoint starting: shutdown 
immediate
2025-03-28 12:19:30.515 +06 [53081] LOG:  checkpoint complete: wrote 0 
buffers (0.0%), wrote 0 SLRU buffers; 0 WAL file(s) added, 0 removed, 0 
recycled; write=0.001 s, sync=0.001 s, total=0.202 s; sync files=0, 
longest=0.000 s, average=0.000 s; distance=0 kB, estimate=0 kB; 
lsn=0/1786518, redo lsn=0/1786518
2025-03-28 12:19:30.665 +06 [53058] LOG:  database system is shut down

Prepared a patch to fix it, but there may be a better solution.

  2. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-03-28T09:05:48Z

    On Fri, Mar 28, 2025 at 4:40 PM Евгений Горбанев <gorbanyoves@basealt.ru> wrote:
> Got an assert failure when fuzzing the raw_parser function.
> The query to reproduce:
> SELECT xmltable.* FROM xmltest2, LATERAL xmltable('/d/r' PASSING x
> COLUMNS a int PATH '' || lower(_path) is_not_null|| 'c');
>
> If I understand correctly, is_not_null is considered as a valid keyword
> in xmltable, but it gets the type T_A_Expr.

> Prepared a patch to fix it, but there may be a better solution.

Nice catch.  Yeah, is_not_null is a valid column option in xmltable.
In you example, the value of the is_not_null option is "|| 'c'", which
is interpreted as an A_Expr.

I wonder if the value's type should be checked earlier, rather than at
the last minute.

Also, I think IsA is a better choice for checking the node type.

Thanks
Richard

  3. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-03-28T09:34:48Z

    On Fri, Mar 28, 2025 at 6:05 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Nice catch.  Yeah, is_not_null is a valid column option in xmltable.
> In you example, the value of the is_not_null option is "|| 'c'", which
> is interpreted as an A_Expr.
>
> I wonder if the value's type should be checked earlier, rather than at
> the last minute.

Hmm, I wonder if we should allow the use of the 'is_not_null' keyword
in xmltable.  According to the doc, it seems that users should declare
NULL or NOT NULL for a column by specifying [NOT NULL | NULL] for the
column.

Thanks
Richard

  4. Re: Assert failure in base_yyparse

    Evgeniy Gorbanev <gorbanyoves@basealt.ru> — 2025-03-28T10:12:37Z

    > Also, I think IsA is a better choice for checking the node type.

Agree, IsA is better. Fixed in the patch.


> On Fri, Mar 28, 2025 at 6:05 PM Richard Guo <guofenglinux@gmail.com> wrote:
> Hmm, I wonder if we should allow the use of the 'is_not_null' keyword
> in xmltable.  According to the doc, it seems that users should declare
> NULL or NOT NULL for a column by specifying [NOT NULL | NULL] for the
> column.
>
> Thanks
> Richard

If you replace is_not_null with NOT NULL in the query, everything works 
correctly.
It seems that is_not_null is an incorrect keyword and it should not be 
used, but I don't understand how it gets here.

Best regards,
Evgeniy

  5. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-04-14T08:45:16Z

    On Fri, Mar 28, 2025 at 7:12 PM Евгений Горбанев <gorbanyoves@basealt.ru> wrote:
> If you replace is_not_null with NOT NULL in the query, everything works
> correctly.
> It seems that is_not_null is an incorrect keyword and it should not be
> used, but I don't understand how it gets here.

It seems what happens is that internally in gram.y (~line 14274), the
DefElem for the not-null option is assigned the name "is_not_null".
As a result, this allows users to explicitly use "is_not_null" as the
option name.  However, the value provided for the is_not_null option
in this way might not be a Boolean as expected, which triggers the
assertion.

I kind of doubt we should allow the use of the "is_not_null" keyword
in the xmltable function.

Hi Álvaro, what do you think about this?

Thanks
Richard

  6. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-05-09T05:54:37Z

    On Mon, Apr 14, 2025 at 5:45 PM Richard Guo <guofenglinux@gmail.com> wrote:
> It seems what happens is that internally in gram.y (~line 14274), the
> DefElem for the not-null option is assigned the name "is_not_null".
> As a result, this allows users to explicitly use "is_not_null" as the
> option name.  However, the value provided for the is_not_null option
> in this way might not be a Boolean as expected, which triggers the
> assertion.
>
> I kind of doubt we should allow the use of the "is_not_null" keyword
> in the xmltable function.
>
> Hi Álvaro, what do you think about this?

Just realized Álvaro has switched to a new email address — re-cc'ing.

Thanks
Richard

  7. Re: Assert failure in base_yyparse

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-09T09:44:24Z

    On 2025-Apr-14, Richard Guo wrote:

> It seems what happens is that internally in gram.y (~line 14274), the
> DefElem for the not-null option is assigned the name "is_not_null".
> As a result, this allows users to explicitly use "is_not_null" as the
> option name.  However, the value provided for the is_not_null option
> in this way might not be a Boolean as expected, which triggers the
> assertion.
> 
> I kind of doubt we should allow the use of the "is_not_null" keyword
> in the xmltable function.
> 
> Hi Álvaro, what do you think about this?

Hello Richard, sorry that I failed to notice this earlier.

I agree that blocking the index from using the option name that xmltable
parsing uses internally is okay.  Maybe we can rename it to something
like "__pg__is_not_null" or something like that, which would reduce the
chances of troubling people; the existing name sounds too much like a
valid name that users could want to use.

Also, maybe rather than just "syntax error" we could say something like
"option name XYZ cannot be used in XMLTABLE".

I wonder if we have any other names used by the parser that can cause
this kind of problem.  In a quick look through gram.y I didn't find any
other place that would fabricate a name and also accept arbitrary
user-specified names to use, so this seems to be the only place affected
by this particular bug.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  8. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-05-14T07:06:12Z

    On Fri, May 9, 2025 at 6:50 PM Alvaro Herrera <alvherre@kurilemu.de> wrote:
> I agree that blocking the index from using the option name that xmltable
> parsing uses internally is okay.  Maybe we can rename it to something
> like "__pg__is_not_null" or something like that, which would reduce the
> chances of troubling people; the existing name sounds too much like a
> valid name that users could want to use.
>
> Also, maybe rather than just "syntax error" we could say something like
> "option name XYZ cannot be used in XMLTABLE".

Agreed on both points.

Evgeniy's patch checks the type of the option argument at the last
moment and raises an error if it's not Boolean.  I think it might be
better to check whether the user-supplied name collides with the
internally reserved name earlier in the process.  According to the
doc, users should specify column nullability using [NOT NULL | NULL]
in the column definition, rather than explicitly setting an
"is_not_null" option.

Attached is a patch that implements this.  It also renames the
internally used option name and updates the error message.

> I wonder if we have any other names used by the parser that can cause
> this kind of problem.  In a quick look through gram.y I didn't find any
> other place that would fabricate a name and also accept arbitrary
> user-specified names to use, so this seems to be the only place affected
> by this particular bug.

Yeah.  Thanks for checking.

Thanks
Richard

  9. Re: Assert failure in base_yyparse

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-05-14T16:20:00Z

    On 2025-May-14, Richard Guo wrote:

> Attached is a patch that implements this.  It also renames the
> internally used option name and updates the error message.

LGTM.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

  10. Re: Assert failure in base_yyparse

    Richard Guo <guofenglinux@gmail.com> — 2025-05-15T08:55:08Z

    On Thu, May 15, 2025 at 1:20 AM Alvaro Herrera <alvherre@kurilemu.de> wrote:
> On 2025-May-14, Richard Guo wrote:
> > Attached is a patch that implements this.  It also renames the
> > internally used option name and updates the error message.

> LGTM.

Thanks.  I've pushed this patch and backpatched it through v15.  I
didn't backpatch it to v14 or v13, as those branches don't use boolVal
to retrieve the value of the "is_not_null" option, so the assertion
failure doesn't occur there.

Thanks
Richard