Thread: Assert failure in base_yyparse
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.
Attachment
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
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
> 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
Attachment
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
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
Attachment
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/
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