Avoid overflow hazard when clamping group counts to "long int".

Several places in the planner tried to clamp a double value to fit
in a "long" by doing
	(long) Min(x, (double) LONG_MAX);
This is subtly incorrect, because it casts LONG_MAX to double and
potentially back again.  If long is 64 bits then the double value
is inexact, and the platform might round it up to LONG_MAX+1
resulting in an overflow and an undesirably negative output.

While it's not hard to rewrite the expression into a safe form,
let's put it into a common function to reduce the risk of someone
doing it wrong in future.

In principle this is a bug fix, but since the problem could only
manifest with group count estimates exceeding 2^63, it seems unlikely
that anyone has actually hit this or will do so anytime soon.  We're
fixing it mainly to satisfy fuzzer-type tools.  That being the case,
a HEAD-only fix seems sufficient.

Andrey Lepikhov

Discussion: https://postgr.es/m/ebbc2efb-7ef9-bf2f-1ada-d6ec48f70e58@postgrespro.ru

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 60d22900304c6f2c825589b31cb6b335099e0e1f..43b36dcd3b4f737b38e4b872f640a9a1f8c6c974 100644 (file)
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -26,7 +26,6 @@
  */
 #include "postgres.h"
 
-#include <limits.h>
 #include <math.h>
 
 #include "access/htup_details.h"
@@ -35,6 +34,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 +498,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 ed98ba7dbd2b917a74e9133173166bb41b588e8a..fcc26b01a4f3163f4e85043f1ef117dd0c6fcd13 100644 (file)
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -71,6 +71,7 @@
 
 #include "postgres.h"
 
+#include <limits.h>
 #include <math.h>
 
 #include "access/amapi.h"
@@ -215,6 +216,32 @@ 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)
+{
+       /*
+        * Just for paranoia's sake, ensure we do something sane with negative or
+        * NaN values.
+        */
+       if (isnan(x))
+               return LONG_MAX;
+       if (x <= 0)
+               return 0;
+
+       /*
+        * If "long" is 64 bits, then LONG_MAX cannot be represented exactly as a
+        * double.  Casting it to double and back may well result in overflow due
+        * to rounding, so avoid doing that.  We trust that any double value that
+        * compares strictly less than "(double) LONG_MAX" will cast to a
+        * representable "long" value.
+        */
+       return (x < (double) LONG_MAX) ? (long) x : LONG_MAX;
+}
+
 
 /*
  * cost_seqscan

diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c
index f4cc56039c2a176c752b3c151f3563ae958b7ea3..76606faa3e4918339769d0f93d33f7ba62d0815f 100644 (file)
--- a/src/backend/optimizer/plan/createplan.c
+++ b/src/backend/optimizer/plan/createplan.c
@@ -16,7 +16,6 @@
  */
 #include "postgres.h"
 
-#include <limits.h>
 #include <math.h>
 
 #include "access/sysattr.h"
@@ -2724,7 +2723,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 +2760,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 +6553,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 d40ce2eae14dc75184c1ba304665777ba35f16b8..7be1e5906b1f56ac0d23a3b6a32864c046f59df2 100644 (file)
--- 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: */