pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedNegative value of numGroups
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2022-05-21 a916cb9d5a Avoid overflow hazard when clamping group counts to "long int".
* Negative value of numGroups
@ 2022-05-13 06:56 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Lepikhov @ 2022-05-13 06:56 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Hi,
Using sqlancer I've found curious vulnerability. In some places of the
code we convert LONG_MAX to double. After value of 2^53 double doesn't
correspond to a long value precisely. So, LONG_MAX can be converted into
'LONG_MAX + 1' double value. And string:
(long) Min(numGroups, (double) LONG_MAX);
can return negative value, if numGroups > LONG_MAX.
Maybe it isn't practical issue right now, but this annoying thing
doesn't allow to pass sqlancer tests sometimes, for example, with options:
--extensions='pg_stat_statements' --oracle HAVING
It is not difficult to fix the problem in-place, of course. But maybe do
better: change the type of numGroups field in any Plan nodes to double
and convert it into specific type right before usage?
--
Regards
Andrey Lepikhov
Postgres Professional
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Negative value of numGroups
@ 2022-05-17 12:59 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Lepikhov @ 2022-05-17 12:59 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
On 13/5/2022 11:56, Andrey Lepikhov wrote:
> (long) Min(numGroups, (double) LONG_MAX);
>
> can return negative value, if numGroups > LONG_MAX.
So, because this problem still breaks sqlancer tests I prepared a patch
(with regression test) that fixes it.
--
regards,
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Safe-calculation-of-the-minimum-number-of-groups-fro.patch (text/plain, 6.1K) download
From 24a2c6664e38448f652a17b96d16ccf58bcf4643 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 17 May 2022 17:48:49 +0500
Subject: [PATCH] Safe calculation of the minimum number of groups from couple
of a long and a double values. Previous implementation returned negative
value in the case of a double > LONG_MAX value.
---
src/backend/executor/nodeSubplan.c | 3 +-
src/backend/optimizer/plan/createplan.c | 8 +++--
src/test/regress/expected/aggregates.out | 41 ++++++++++++++++++++++++
src/test/regress/sql/aggregates.sql | 12 +++++++
4 files changed, 60 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 60d2290030..89dfe09970 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -498,7 +498,8 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
node->havehashrows = false;
node->havenullrows = false;
- nbuckets = (long) Min(planstate->plan->plan_rows, (double) LONG_MAX);
+ nbuckets = (planstate->plan->plan_rows < (double) LONG_MAX) ?
+ (long) planstate->plan->plan_rows : LONG_MAX;
if (nbuckets < 1)
nbuckets = 1;
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index f4cc56039c..fb94d27473 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2724,7 +2724,8 @@ create_setop_plan(PlannerInfo *root, SetOpPath *best_path, int flags)
flags | CP_LABEL_TLIST);
/* Convert numGroups to long int --- but 'ware overflow! */
- numGroups = (long) Min(best_path->numGroups, (double) LONG_MAX);
+ numGroups = (best_path->numGroups < (double) LONG_MAX) ?
+ (long) best_path->numGroups : LONG_MAX;
plan = make_setop(best_path->cmd,
best_path->strategy,
@@ -2761,7 +2762,8 @@ create_recursiveunion_plan(PlannerInfo *root, RecursiveUnionPath *best_path)
tlist = build_path_tlist(root, &best_path->path);
/* Convert numGroups to long int --- but 'ware overflow! */
- numGroups = (long) Min(best_path->numGroups, (double) LONG_MAX);
+ numGroups = (best_path->numGroups < (double) LONG_MAX) ?
+ (long) best_path->numGroups : LONG_MAX;
plan = make_recursive_union(tlist,
leftplan,
@@ -6554,7 +6556,7 @@ make_agg(List *tlist, List *qual,
long numGroups;
/* Reduce to long, but 'ware overflow! */
- numGroups = (long) Min(dNumGroups, (double) LONG_MAX);
+ numGroups = dNumGroups < (double) LONG_MAX ? (long) dNumGroups : LONG_MAX;
node->aggstrategy = aggstrategy;
node->aggsplit = aggsplit;
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 601047fa3d..0037981fc4 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -3049,10 +3049,51 @@ set work_mem to default;
----+----+----
(0 rows)
+CREATE TABLE agg_group_5 AS (
+ SELECT 1 AS x, NULL AS x1 FROM generate_series(1,1e5) x);
+ANALYZE agg_group_5;
+-- Stress test for grouping: exceed number of 2^53 groups to check
+-- long <-> double conversion.
+EXPLAIN (COSTS OFF) SELECT t1.x1 FROM
+ agg_group_5 t1,agg_group_5 t2,agg_group_5 t3,agg_group_5 t4,agg_group_5 t5,
+ agg_group_5 t6,agg_group_5 t7,agg_group_5 t8,agg_group_5 t9
+GROUP BY (t1.x1,t2.x1,t3.x1,t4.x1,t5.x1,t6.x1,t7.x1,t8.x1,t9.x1);
+ QUERY PLAN
+----------------------------------------------------------------------------------------
+ HashAggregate
+ Group Key: t1.x1, t2.x1, t3.x1, t4.x1, t5.x1, t6.x1, t7.x1, t8.x1, t9.x1
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Seq Scan on agg_group_5 t1
+ -> Materialize
+ -> Seq Scan on agg_group_5 t2
+ -> Materialize
+ -> Seq Scan on agg_group_5 t3
+ -> Materialize
+ -> Seq Scan on agg_group_5 t4
+ -> Materialize
+ -> Seq Scan on agg_group_5 t5
+ -> Materialize
+ -> Seq Scan on agg_group_5 t6
+ -> Materialize
+ -> Seq Scan on agg_group_5 t7
+ -> Materialize
+ -> Seq Scan on agg_group_5 t8
+ -> Materialize
+ -> Seq Scan on agg_group_5 t9
+(27 rows)
+
drop table agg_group_1;
drop table agg_group_2;
drop table agg_group_3;
drop table agg_group_4;
+drop table agg_group_5;
drop table agg_hash_1;
drop table agg_hash_2;
drop table agg_hash_3;
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index c6e0d7ba2b..e19d74fef5 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -1351,10 +1351,22 @@ set work_mem to default;
union all
(select * from agg_group_4 except select * from agg_hash_4);
+CREATE TABLE agg_group_5 AS (
+ SELECT 1 AS x, NULL AS x1 FROM generate_series(1,1e5) x);
+ANALYZE agg_group_5;
+
+-- Stress test for grouping: exceed number of 2^53 groups to check
+-- long <-> double conversion.
+EXPLAIN (COSTS OFF) SELECT t1.x1 FROM
+ agg_group_5 t1,agg_group_5 t2,agg_group_5 t3,agg_group_5 t4,agg_group_5 t5,
+ agg_group_5 t6,agg_group_5 t7,agg_group_5 t8,agg_group_5 t9
+GROUP BY (t1.x1,t2.x1,t3.x1,t4.x1,t5.x1,t6.x1,t7.x1,t8.x1,t9.x1);
+
drop table agg_group_1;
drop table agg_group_2;
drop table agg_group_3;
drop table agg_group_4;
+drop table agg_group_5;
drop table agg_hash_1;
drop table agg_hash_2;
drop table agg_hash_3;
--
2.36.0
* Re: Negative value of numGroups
@ 2022-05-17 15:34 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 1 reply; 5+ messages in thread
From: Tom Lane @ 2022-05-17 15:34 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: pgsql-bugs@lists.postgresql.org
Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
> On 13/5/2022 11:56, Andrey Lepikhov wrote:
>> (long) Min(numGroups, (double) LONG_MAX);
>>
>> can return negative value, if numGroups > LONG_MAX.
I see your point, but I don't think that repeating the same finicky
and undocumented coding pattern in multiple places is a future-proof
fix. I'm inclined to think we should invent a function along the
lines of "long clamp_double_to_long(double x)". Given where it's
used, maybe putting it beside clamp_row_est() would be good.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Negative value of numGroups
@ 2022-05-19 09:03 Andrey Lepikhov <a.lepikhov@postgrespro.ru>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 5+ messages in thread
From: Andrey Lepikhov @ 2022-05-19 09:03 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-bugs@lists.postgresql.org
On 5/17/22 20:34, Tom Lane wrote:
> Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
>> On 13/5/2022 11:56, Andrey Lepikhov wrote:
>>> (long) Min(numGroups, (double) LONG_MAX);
>>>
>>> can return negative value, if numGroups > LONG_MAX.
>
> I see your point, but I don't think that repeating the same finicky
> and undocumented coding pattern in multiple places is a future-proof
> fix. I'm inclined to think we should invent a function along the
> lines of "long clamp_double_to_long(double x)". Given where it's
> used, maybe putting it beside clamp_row_est() would be good.
>
> regards, tom lane
Next version of the patch.
As I see, now we trying to use Cardinality type instead of double.
So, I named casting routine as clamp_cardinality_to_long.
--
Regards
Andrey Lepikhov
Postgres Professional
Attachments:
0001-Fix-potential-problem-with-negative-value-of-groups-.patch (text/x-patch, 7.6K) download
From 533001bf070053c62956a6ad9921f7c3b370fb96 Mon Sep 17 00:00:00 2001
From: "Andrey V. Lepikhov" <a.lepikhov@postgrespro.ru>
Date: Tue, 17 May 2022 17:48:49 +0500
Subject: [PATCH] Fix potential problem with negative value of groups number.
Add the clamp_cardinality_to_long routine for safe cast Cardinality to a long
value everywhere in the code.
---
src/backend/executor/nodeSubplan.c | 3 +-
src/backend/optimizer/path/costsize.c | 25 +++++++++++++++
src/backend/optimizer/plan/createplan.c | 6 ++--
src/include/optimizer/optimizer.h | 1 +
src/test/regress/expected/aggregates.out | 41 ++++++++++++++++++++++++
src/test/regress/sql/aggregates.sql | 12 +++++++
6 files changed, 84 insertions(+), 4 deletions(-)
diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 60d2290030..0189dc66ee 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -35,6 +35,7 @@
#include "miscadmin.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
+#include "optimizer/optimizer.h"
#include "utils/array.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -498,7 +499,7 @@ buildSubPlanHash(SubPlanState *node, ExprContext *econtext)
node->havehashrows = false;
node->havenullrows = false;
- nbuckets = (long) Min(planstate->plan->plan_rows, (double) LONG_MAX);
+ nbuckets = clamp_cardinality_to_long(planstate->plan->plan_rows);
if (nbuckets < 1)
nbuckets = 1;
diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index ed98ba7dbd..322b959993 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -215,6 +215,31 @@ clamp_row_est(double nrows)
return nrows;
}
+/*
+ * clamp_cardinality_to_long
+ * Cast a cardinality value to a sane long value.
+ */
+long
+clamp_cardinality_to_long(Cardinality x)
+{
+ long y;
+
+ /* Avoid infinite and NaN values by the same reason as described above. */
+ if (x < 0)
+ return 0;
+ if (isnan(x))
+ return LONG_MAX;
+
+ /*
+ * LONG_MAX value, 2^n-1, hasn't direct representation in double. So, it
+ * can be converted to greater (2^n) or a lesser number which has
+ * representation in double format. So, result of this calculation may have
+ * slight fluency across different compilers, but doesn't matter much.
+ */
+ y = (x < (double) LONG_MAX) ? (long) x : LONG_MAX;
+
+ return y;
+}
/*
* cost_seqscan
diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index f4cc56039c..a7de85a869 100644
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -2724,7 +2724,7 @@ create_setop_plan(PlannerInfo *root, SetOpPath *best_path, int flags)
flags | CP_LABEL_TLIST);
/* Convert numGroups to long int --- but 'ware overflow! */
- numGroups = (long) Min(best_path->numGroups, (double) LONG_MAX);
+ numGroups = clamp_cardinality_to_long(best_path->numGroups);
plan = make_setop(best_path->cmd,
best_path->strategy,
@@ -2761,7 +2761,7 @@ create_recursiveunion_plan(PlannerInfo *root, RecursiveUnionPath *best_path)
tlist = build_path_tlist(root, &best_path->path);
/* Convert numGroups to long int --- but 'ware overflow! */
- numGroups = (long) Min(best_path->numGroups, (double) LONG_MAX);
+ numGroups = clamp_cardinality_to_long(best_path->numGroups);
plan = make_recursive_union(tlist,
leftplan,
@@ -6554,7 +6554,7 @@ make_agg(List *tlist, List *qual,
long numGroups;
/* Reduce to long, but 'ware overflow! */
- numGroups = (long) Min(dNumGroups, (double) LONG_MAX);
+ numGroups = clamp_cardinality_to_long(dNumGroups);
node->aggstrategy = aggstrategy;
node->aggsplit = aggsplit;
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index d40ce2eae1..7be1e5906b 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -95,6 +95,7 @@ extern PGDLLIMPORT double recursive_worktable_factor;
extern PGDLLIMPORT int effective_cache_size;
extern double clamp_row_est(double nrows);
+extern long clamp_cardinality_to_long(Cardinality x);
/* in path/indxpath.c: */
diff --git a/src/test/regress/expected/aggregates.out b/src/test/regress/expected/aggregates.out
index 601047fa3d..0037981fc4 100644
--- a/src/test/regress/expected/aggregates.out
+++ b/src/test/regress/expected/aggregates.out
@@ -3049,10 +3049,51 @@ set work_mem to default;
----+----+----
(0 rows)
+CREATE TABLE agg_group_5 AS (
+ SELECT 1 AS x, NULL AS x1 FROM generate_series(1,1e5) x);
+ANALYZE agg_group_5;
+-- Stress test for grouping: exceed number of 2^53 groups to check
+-- long <-> double conversion.
+EXPLAIN (COSTS OFF) SELECT t1.x1 FROM
+ agg_group_5 t1,agg_group_5 t2,agg_group_5 t3,agg_group_5 t4,agg_group_5 t5,
+ agg_group_5 t6,agg_group_5 t7,agg_group_5 t8,agg_group_5 t9
+GROUP BY (t1.x1,t2.x1,t3.x1,t4.x1,t5.x1,t6.x1,t7.x1,t8.x1,t9.x1);
+ QUERY PLAN
+----------------------------------------------------------------------------------------
+ HashAggregate
+ Group Key: t1.x1, t2.x1, t3.x1, t4.x1, t5.x1, t6.x1, t7.x1, t8.x1, t9.x1
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Nested Loop
+ -> Seq Scan on agg_group_5 t1
+ -> Materialize
+ -> Seq Scan on agg_group_5 t2
+ -> Materialize
+ -> Seq Scan on agg_group_5 t3
+ -> Materialize
+ -> Seq Scan on agg_group_5 t4
+ -> Materialize
+ -> Seq Scan on agg_group_5 t5
+ -> Materialize
+ -> Seq Scan on agg_group_5 t6
+ -> Materialize
+ -> Seq Scan on agg_group_5 t7
+ -> Materialize
+ -> Seq Scan on agg_group_5 t8
+ -> Materialize
+ -> Seq Scan on agg_group_5 t9
+(27 rows)
+
drop table agg_group_1;
drop table agg_group_2;
drop table agg_group_3;
drop table agg_group_4;
+drop table agg_group_5;
drop table agg_hash_1;
drop table agg_hash_2;
drop table agg_hash_3;
diff --git a/src/test/regress/sql/aggregates.sql b/src/test/regress/sql/aggregates.sql
index c6e0d7ba2b..e19d74fef5 100644
--- a/src/test/regress/sql/aggregates.sql
+++ b/src/test/regress/sql/aggregates.sql
@@ -1351,10 +1351,22 @@ set work_mem to default;
union all
(select * from agg_group_4 except select * from agg_hash_4);
+CREATE TABLE agg_group_5 AS (
+ SELECT 1 AS x, NULL AS x1 FROM generate_series(1,1e5) x);
+ANALYZE agg_group_5;
+
+-- Stress test for grouping: exceed number of 2^53 groups to check
+-- long <-> double conversion.
+EXPLAIN (COSTS OFF) SELECT t1.x1 FROM
+ agg_group_5 t1,agg_group_5 t2,agg_group_5 t3,agg_group_5 t4,agg_group_5 t5,
+ agg_group_5 t6,agg_group_5 t7,agg_group_5 t8,agg_group_5 t9
+GROUP BY (t1.x1,t2.x1,t3.x1,t4.x1,t5.x1,t6.x1,t7.x1,t8.x1,t9.x1);
+
drop table agg_group_1;
drop table agg_group_2;
drop table agg_group_3;
drop table agg_group_4;
+drop table agg_group_5;
drop table agg_hash_1;
drop table agg_hash_2;
drop table agg_hash_3;
--
2.34.1
* Re: Negative value of numGroups
@ 2022-05-21 17:15 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andrey Lepikhov <a.lepikhov@postgrespro.ru>
0 siblings, 0 replies; 5+ messages in thread
From: Tom Lane @ 2022-05-21 17:15 UTC (permalink / raw)
To: Andrey Lepikhov <a.lepikhov@postgrespro.ru>; +Cc: pgsql-bugs@lists.postgresql.org
Andrey Lepikhov <a.lepikhov@postgrespro.ru> writes:
> Next version of the patch.
> As I see, now we trying to use Cardinality type instead of double.
> So, I named casting routine as clamp_cardinality_to_long.
Pushed with a bit of rewriting of comments etc. I left out the
test case too, because (a) for me, it did not reveal any difference
between patched and unpatched code, and (b) even if it had, it was
far too expensive to put into the core regression tests.
regards, tom lane
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2022-05-21 17:15 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13 06:56 Negative value of numGroups Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2022-05-17 12:59 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2022-05-17 15:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-05-19 09:03 ` Andrey Lepikhov <a.lepikhov@postgrespro.ru>
2022-05-21 17:15 ` 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