Thread: BUG #17152: ERROR: AddressSanitizer: SEGV on iso-8859-1 address
The following bug has been logged on the website:
Bug reference: 17152
Logged by: Zhiyong Wu
Email address: 253540651@qq.com
PostgreSQL version: 14beta2
Operating system: Linux version 5.13.0-1-MANJARO (builduser@LEGION)
Description:
PoC:
CREATE TEMP TABLE v0 ( v2 SMALLINT NOT NULL DEFAULT - - 90 , DATA TEXT , v1
REAL CONSTRAINT XMLFOREST NULL ) ;
INSERT INTO v0 VALUES ( - - - - 0 , - - - - -1 ) , ( - - ( ( ( SELECT (
SELECT LEAST ( v1 ) x FROM v0 WHERE - - - 43 >= v1 ) FROM v0 AS v2 ( OVERLAY
, v2 , v1 ) ) ) UNION SELECT - - - 22 ) , - - - - - - 2147483647 ) , ( - - -
-128 , - - - -2147483648 ) , ( - - - - 36 , - - - - - - - -128 ) , ( - - - -
9 , - - - - - -128 ) ON CONFLICT DO NOTHING ;
;
SELECT - - 11 + v2 AS x FROM v0 WHERE v2 = ( SELECT LEAST ( ( ( ( SELECT -
127 FROM ( SELECT 0 FROM ( VALUES ( - 16 ) , ( -2147483648 ) , ( - - - - -1
) ) v2 ( v2 ) GROUP BY ( + - - 72 ) / - - 18 ) AS SMALLINT ) ) UNION SELECT
MODE ( ) WITHIN GROUP ( ORDER BY v2 DESC ) FILTER ( WHERE MODE ( ) WITHIN
GROUP ( ORDER BY v1 = CASE WHEN v1 IS NULL THEN v1 ELSE - - 91 END DESC ) )
NULL ) ) FROM v0 ) ;
COMMIT TRANSACTION ;
DELETE FROM v0 WHERE v2 = - - - - - - 38 ;
;
Asan Report:
AddressSanitizer:DEADLYSIGNAL
=================================================================
==52==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc
0x000000c6428a bp 0x7ffcd1914310 sp 0x7ffcd1914040 T0)
==52==The signal is caused by a READ memory access.
==52==Hint: address points to the zero page.
#0 0xc64289 in ExecInterpExpr
/home/postgres/postgres/bld/../src/backend/executor/execExprInterp.c:1532:20
#1 0xce2658 in ExecEvalExprSwitchContext
/home/postgres/postgres/bld/../src/include/executor/executor.h:339:13
#2 0xce2658 in advance_aggregates
/home/postgres/postgres/bld/../src/backend/executor/nodeAgg.c:842
#3 0xce2658 in agg_retrieve_direct
/home/postgres/postgres/bld/../src/backend/executor/nodeAgg.c:2450
#4 0xce2658 in ExecAgg
/home/postgres/postgres/bld/../src/backend/executor/nodeAgg.c:2175
#5 0xd80380 in ExecProcNode
/home/postgres/postgres/bld/../src/include/executor/executor.h:257:9
#6 0xd80380 in ExecSetParamPlan
/home/postgres/postgres/bld/../src/backend/executor/nodeSubplan.c:1118
#7 0xc66f2b in ExecEvalParamExec
/home/postgres/postgres/bld/../src/backend/executor/execExprInterp.c:2414:3
#8 0xc66f2b in ExecInterpExpr
/home/postgres/postgres/bld/../src/backend/executor/execExprInterp.c:1062
#9 0xcb09f2 in ExecEvalExprSwitchContext
/home/postgres/postgres/bld/../src/include/executor/executor.h:339:13
#10 0xcb09f2 in ExecQual
/home/postgres/postgres/bld/../src/include/executor/executor.h:408
#11 0xcb09f2 in ExecScan
/home/postgres/postgres/bld/../src/backend/executor/execScan.c:227
#12 0xc89648 in ExecProcNode
/home/postgres/postgres/bld/../src/include/executor/executor.h:257:9
#13 0xc89648 in ExecutePlan
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:1551
#14 0xc89648 in standard_ExecutorRun
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:361
#15 0xc89061 in ExecutorRun
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:305:3
#16 0x13ca6af in PortalRunSelect
/home/postgres/postgres/bld/../src/backend/tcop/pquery.c:919:4
#17 0x13c974d in PortalRun
/home/postgres/postgres/bld/../src/backend/tcop/pquery.c:763:18
#18 0x13c52d5 in exec_simple_query
/home/postgres/postgres/bld/../src/backend/tcop/postgres.c:1214:10
#19 0x13be613 in PostgresMain
/home/postgres/postgres/bld/../src/backend/tcop/postgres.c
#20 0xe073fd in main
/home/postgres/postgres/bld/../src/backend/main/main.c:205:3
#21 0x7f61369f6bf6 in __libc_start_main
/build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
#22 0x499889 in _start (/usr/local/pgsql/bin/postgres+0x499889)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
/home/postgres/postgres/bld/../src/backend/executor/execExprInterp.c:1532:20
in ExecInterpExpr
==52==ABORTING
On Wed, Aug 18, 2021 at 02:56:00AM +0000, PG Bug reporting form wrote: > The following bug has been logged on the website: > > Bug reference: 17152 > Logged by: Zhiyong Wu > Email address: 253540651@qq.com > PostgreSQL version: 14beta2 > Operating system: Linux version 5.13.0-1-MANJARO (builduser@LEGION) > Description: > > PoC: > CREATE TEMP TABLE v0 ( v2 SMALLINT NOT NULL DEFAULT - - 90 , DATA TEXT , v1 > REAL CONSTRAINT XMLFOREST NULL ) ; > INSERT INTO v0 VALUES ( - - - - 0 , - - - - -1 ) , ( - - ( ( ( SELECT ( > SELECT LEAST ( v1 ) x FROM v0 WHERE - - - 43 >= v1 ) FROM v0 AS v2 ( OVERLAY > , v2 , v1 ) ) ) UNION SELECT - - - 22 ) , - - - - - - 2147483647 ) , ( - - - > -128 , - - - -2147483648 ) , ( - - - - 36 , - - - - - - - -128 ) , ( - - - - > 9 , - - - - - -128 ) ON CONFLICT DO NOTHING ; > ; > SELECT - - 11 + v2 AS x FROM v0 WHERE v2 = ( SELECT LEAST ( ( ( ( SELECT - > 127 FROM ( SELECT 0 FROM ( VALUES ( - 16 ) , ( -2147483648 ) , ( - - - - -1 > ) ) v2 ( v2 ) GROUP BY ( + - - 72 ) / - - 18 ) AS SMALLINT ) ) UNION SELECT > MODE ( ) WITHIN GROUP ( ORDER BY v2 DESC ) FILTER ( WHERE MODE ( ) WITHIN > GROUP ( ORDER BY v1 = CASE WHEN v1 IS NULL THEN v1 ELSE - - 91 END DESC ) ) > NULL ) ) FROM v0 ) ; > COMMIT TRANSACTION ; > DELETE FROM v0 WHERE v2 = - - - - - - 38 ; > ; I can also confirm this failure on git master. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com If only the physical world exists, free will is an illusion.
PG Bug reporting form <noreply@postgresql.org> writes:
> [ unreadable example ]
This can be boiled down to
CREATE TEMP TABLE v0 ( v2 int );
INSERT INTO v0 VALUES (1);
SELECT (
SELECT
MODE ( ) WITHIN GROUP ( ORDER BY v2 )
FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
)
FROM v0;
It seems fairly clear to me that this ought to be rejected as invalid,
because we don't allow aggregates within the arguments of aggregates.
That does happen without the sub-select:
regression=# SELECT
regression-# MODE ( ) WITHIN GROUP ( ORDER BY v2 )
regression-# FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
regression-# FROM v0;
ERROR: aggregate functions are not allowed in FILTER
LINE 3: FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 )...
^
So somehow the fact that outer references are involved is misleading that
error check. I've not traced it further than that.
regards, tom lane
I wrote:
> So somehow the fact that outer references are involved is misleading that
> error check. I've not traced it further than that.
Actually, it's simpler than that. The patch that added FILTER
taught check_agg_arguments to check the FILTER argument like this:
(void) expression_tree_walker((Node *) filter,
check_agg_arguments_walker,
(void *) &context);
which was a blind copy-and-paste of what it does for the "args"
list. However, that coding for "args" includes an undocumented
optimization: "args" is a List, and check_agg_arguments_walker
doesn't care about Lists, so it's okay to recurse directly to
expression_tree_walker without invoking the walker on the top-level
List node. This is completely NOT okay for the "filter" argument,
which could be directly a Var or Aggref. Hence, the check completely
misses a top-level Var or Aggref in FILTER.
It would be enough to convert this one call to
(void) check_agg_arguments_walker((Node *) filter, &context);
but in the attached I just converted all three of check_agg_arguments'
invocations to look that way. The few nanoseconds shaved by the
other coding don't justify the risk of more copy-and-paste bugs.
An interesting nearby issue is that check_agglevels_and_constraints is
not really handling this correctly either. It supposes that once it's
identified the agglevelsup for an Aggref node, it can look that many
levels up in the parse stack to see whether it's inside a relevant
FILTER expression. This is wrong, because we've not yet determined
the semantic level of the surrounding aggregate if any, so we've
certainly not set p_expr_kind in the relevant outer pstate level.
That explains why you get
regression=# CREATE TEMP TABLE v0 ( v2 int );
CREATE TABLE
regression=#
SELECT
MODE ( ) WITHIN GROUP ( ORDER BY v2 )
FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
FROM v0;
ERROR: aggregate functions are not allowed in FILTER
LINE 3: FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 )...
^
while (after this patch) the outer-level case gives
regression=# SELECT (
SELECT
MODE ( ) WITHIN GROUP ( ORDER BY v2 )
FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 ) )
)
FROM v0;
ERROR: aggregate function calls cannot be nested
LINE 4: FILTER ( WHERE MODE ( ) WITHIN GROUP ( ORDER BY v2 > 0 )...
^
check_agglevels_and_constraints doesn't complain at the lower
Aggref, so it's left to the upper one to whine.
I'm not too concerned about this cosmetic difference, but I wonder
if there are any cases where check_agglevels_and_constraints will
throw an error when it shouldn't. Right offhand I can't see any,
but I may just lack imagination today.
Regardless of that, we certainly need the attached patch (and,
probably, some more regression tests).
regards, tom lane
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 24268eb502..7d829a05a9 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -627,13 +627,8 @@ check_agg_arguments(ParseState *pstate,
context.min_agglevel = -1;
context.sublevels_up = 0;
- (void) expression_tree_walker((Node *) args,
- check_agg_arguments_walker,
- (void *) &context);
-
- (void) expression_tree_walker((Node *) filter,
- check_agg_arguments_walker,
- (void *) &context);
+ (void) check_agg_arguments_walker((Node *) args, &context);
+ (void) check_agg_arguments_walker((Node *) filter, &context);
/*
* If we found no vars nor aggs at all, it's a level-zero aggregate;
@@ -680,9 +675,7 @@ check_agg_arguments(ParseState *pstate,
{
context.min_varlevel = -1;
context.min_agglevel = -1;
- (void) expression_tree_walker((Node *) directargs,
- check_agg_arguments_walker,
- (void *) &context);
+ (void) check_agg_arguments_walker((Node *) directargs, &context);
if (context.min_varlevel >= 0 && context.min_varlevel < agglevel)
ereport(ERROR,
(errcode(ERRCODE_GROUPING_ERROR),
I wrote:
> I'm not too concerned about this cosmetic difference, but I wonder
> if there are any cases where check_agglevels_and_constraints will
> throw an error when it shouldn't. Right offhand I can't see any,
> but I may just lack imagination today.
After further study I've more or less convinced myself that that
isn't an issue. We may consider these cases:
1. We are parsing an aggregate call that's inside a FILTER clause
of the current query level.
1a. If we identify the aggregate as being of the current query level,
we will look at the current pstate, see p_expr_kind ==
EXPR_KIND_FILTER, and throw an error. That is correct, because the
existence of this aggregate would constrain the surrounding aggregate
to also have the current query level as semantic level.
1b. If we identify the aggregate as being of some outer query level,
we will look at that outer pstate and probably not see
EXPR_KIND_FILTER (see #2 for the case where we do see that). In
this case check_agglevels_and_constraints will not throw an error,
which is fine because we can't yet know the semantic levels of
surrounding aggregate(s). It's left to check_agg_arguments, when
it checks the surrounding aggregate, to throw an error if needed.
2. We are parsing an aggregate call that's inside a FILTER clause
of some outer query level (i.e., the current aggregate is inside
a sub-SELECT inside FILTER).
2a. If we identify the aggregate as being of that same outer
query level, we will see EXPR_KIND_FILTER and throw error.
As in #1a, the error is correct since we'd ultimately assign
that outer aggregate to its own syntactic level. (It clearly
can't have a semantic level lower than its syntactic level,
and again the existence of this aggregate within its arguments
prevents it from being assigned to any higher level.)
2b. If we identify the aggregate as being of some other query level
where there is not an open FILTER clause, we will not throw error.
As in #1b, it's up to check_agg_arguments to do so if needed.
So I think the patch I presented is sufficient. I added some
test cases and pushed it.
Thanks for the report!
regards, tom lane