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
This commit is contained in:
Tom Lane 2022-05-21 13:13:41 -04:00
parent ac1ae477f8
commit a916cb9d5a
4 changed files with 33 additions and 6 deletions

View File

@ -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;

View File

@ -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

View File

@ -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;

View File

@ -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: */