database/postgres
database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-04-29 13:56:47 +03:00
Code

Don't reference out-of-bounds array elements in brin_minmax_multi.c

Browse Source 
The primary fix here is to fix has_matching_range() so it does not
reference ranges->values[-1] when nranges == 0.  Similar problems existed
in AssertCheckRanges() too.  It does not look like any of these problems
could lead to a crash as the array in question is at the end of the Ranges
struct, and values[-1] is memory that belongs to other fields in the
struct.  However, let's get rid of these rather unsafe coding practices.

In passing, I (David) adjusted some comments to try to make it more clear
what some of the fields are for in the Ranges struct.  I had to study the
code to find out what nsorted was for as I couldn't tell from the
comments.

Author: Ranier Vilela
Discussion: https://postgr.es/m/CAEudQAqJQzPitufX-jR=YUbJafpCDAKUnwgdbX_MzSc93wuvdw@mail.gmail.com
Backpatch-through: 14, where multi-range brin was added.
This commit is contained in:
David Rowley 2022-09-13 11:04:37 +12:00
parent 7d7d72c195
commit 4d5d35858c
1 changed files with 90 additions and 86 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

176
src/backend/access/brin/brin_minmax_multi.c
View File

@ -142,19 +142,23 @@ typedef struct MinMaxMultiOptions
* The Ranges struct stores the boundary values in a single array, but we * The Ranges struct stores the boundary values in a single array, but we
* treat regular and single-point ranges differently to save space. For * treat regular and single-point ranges differently to save space. For
* regular ranges (with different boundary values) we have to store both * regular ranges (with different boundary values) we have to store both
* values, while for "single-point ranges" we only need to save one value. * the lower and upper bound of the range, while for "single-point ranges"
* we only need to store a single value.
* *
* The 'values' array stores boundary values for regular ranges first (there * The 'values' array stores boundary values for regular ranges first (there
* are 2*nranges values to store), and then the nvalues boundary values for * are 2*nranges values to store), and then the nvalues boundary values for
* single-point ranges. That is, we have (2*nranges + nvalues) boundary * single-point ranges. That is, we have (2*nranges + nvalues) boundary
* values in the array. * values in the array.
* *
* +---------------------------------+-------------------------------+ * +-------------------------+----------------------------------+
* | ranges (sorted pairs of values) | sorted values (single points) | * | ranges (2 * nranges of) | single point values (nvalues of) |
* +---------------------------------+-------------------------------+ * +-------------------------+----------------------------------+
* *
* This allows us to quickly add new values, and store outliers without * This allows us to quickly add new values, and store outliers without
* making the other ranges very wide. * having to widen any of the existing range values.
*
* 'nsorted' denotes how many of 'nvalues' in the values[] array are sorted.
* When nsorted == nvalues, all single point values are sorted.
* *
* We never store more than maxvalues values (as set by values_per_range * We never store more than maxvalues values (as set by values_per_range
* reloption). If needed we merge some of the ranges. * reloption). If needed we merge some of the ranges.
@ -173,10 +177,10 @@ typedef struct Ranges
FmgrInfo *cmp; FmgrInfo *cmp;
/* (2*nranges + nvalues) <= maxvalues */ /* (2*nranges + nvalues) <= maxvalues */
int nranges; /* number of ranges in the array (stored) */ int nranges; /* number of ranges in the values[] array */
int nsorted; /* number of sorted values (ranges + points) */ int nsorted; /* number of nvalues which are sorted */
int nvalues; /* number of values in the data array (all) */ int nvalues; /* number of point values in values[] array */
int maxvalues; /* maximum number of values (reloption) */ int maxvalues; /* number of elements in the values[] array */
/* /*
* We simply add the values into a large buffer, without any expensive * We simply add the values into a large buffer, without any expensive
@ -318,102 +322,99 @@ AssertCheckRanges(Ranges *ranges, FmgrInfo *cmpFn, Oid colloid)
* Check that none of the values are not covered by ranges (both sorted * Check that none of the values are not covered by ranges (both sorted
* and unsorted) * and unsorted)
*/ */
for (i = 0; i < ranges->nvalues; i++) if (ranges->nranges > 0)
{ {
Datum compar; for (i = 0; i < ranges->nvalues; i++)
int start,
end;
Datum minvalue,
maxvalue;
Datum value = ranges->values[2 * ranges->nranges + i];
if (ranges->nranges == 0)
break;
minvalue = ranges->values[0];
maxvalue = ranges->values[2 * ranges->nranges - 1];
/*
* Is the value smaller than the minval? If yes, we'll recurse to the
* left side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
/* smaller than the smallest value in the first range */
if (DatumGetBool(compar))
continue;
/*
* Is the value greater than the maxval? If yes, we'll recurse to the
* right side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
/* larger than the largest value in the last range */
if (DatumGetBool(compar))
continue;
start = 0; /* first range */
end = ranges->nranges - 1; /* last range */
while (true)
{ {
int midpoint = (start + end) / 2; Datum compar;
int start,
end;
Datum minvalue = ranges->values[0];
Datum maxvalue = ranges->values[2 * ranges->nranges - 1];
Datum value = ranges->values[2 * ranges->nranges + i];
/* this means we ran out of ranges in the last step */
if (start > end)
break;
/* copy the min/max values from the ranges */
minvalue = ranges->values[2 * midpoint];
maxvalue = ranges->values[2 * midpoint + 1];
/*
* Is the value smaller than the minval? If yes, we'll recurse to
* the left side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue); compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
/* smaller than the smallest value in this range */
if (DatumGetBool(compar))
{
end = (midpoint - 1);
continue;
}
/* /*
* Is the value greater than the minval? If yes, we'll recurse to * If the value is smaller than the lower bound in the first range
* the right side of range array. * then it cannot possibly be in any of the ranges.
*/ */
if (DatumGetBool(compar))
continue;
compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value); compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
/* larger than the largest value in this range */ /*
* Likewise, if the value is larger than the upper bound of the
* final range, then it cannot possibly be inside any of the
* ranges.
*/
if (DatumGetBool(compar)) if (DatumGetBool(compar))
{
start = (midpoint + 1);
continue; continue;
}
/* hey, we found a matching range */ /* bsearch the ranges to see if 'value' fits within any of them */
Assert(false); start = 0; /* first range */
end = ranges->nranges - 1; /* last range */
while (true)
{
int midpoint = (start + end) / 2;
/* this means we ran out of ranges in the last step */
if (start > end)
break;
/* copy the min/max values from the ranges */
minvalue = ranges->values[2 * midpoint];
maxvalue = ranges->values[2 * midpoint + 1];
/*
* Is the value smaller than the minval? If yes, we'll recurse
* to the left side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, value, minvalue);
/* smaller than the smallest value in this range */
if (DatumGetBool(compar))
{
end = (midpoint - 1);
continue;
}
/*
* Is the value greater than the minval? If yes, we'll recurse
* to the right side of range array.
*/
compar = FunctionCall2Coll(cmpFn, colloid, maxvalue, value);
/* larger than the largest value in this range */
if (DatumGetBool(compar))
{
start = (midpoint + 1);
continue;
}
/* hey, we found a matching range */
Assert(false);
}
} }
} }
/* and values in the unsorted part must not be in sorted part */ /* and values in the unsorted part must not be in the sorted part */
for (i = ranges->nsorted; i < ranges->nvalues; i++) if (ranges->nsorted > 0)
{ {
compare_context cxt; compare_context cxt;
Datum value = ranges->values[2 * ranges->nranges + i];
if (ranges->nsorted == 0)
break;
cxt.colloid = ranges->colloid; cxt.colloid = ranges->colloid;
cxt.cmpFn = ranges->cmp; cxt.cmpFn = ranges->cmp;
Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges], for (i = ranges->nsorted; i < ranges->nvalues; i++)
ranges->nsorted, sizeof(Datum), {
compare_values, (void *) &cxt) == NULL); Datum value = ranges->values[2 * ranges->nranges + i];
Assert(bsearch_arg(&value, &ranges->values[2 * ranges->nranges],
ranges->nsorted, sizeof(Datum),
compare_values, (void *) &cxt) == NULL);
}
} }
#endif #endif
} }
@ -924,8 +925,8 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
{ {
Datum compar; Datum compar;
Datum minvalue = ranges->values[0]; Datum minvalue;
Datum maxvalue = ranges->values[2 * ranges->nranges - 1]; Datum maxvalue;
FmgrInfo *cmpLessFn; FmgrInfo *cmpLessFn;
FmgrInfo *cmpGreaterFn; FmgrInfo *cmpGreaterFn;
@ -937,6 +938,9 @@ has_matching_range(BrinDesc *bdesc, Oid colloid, Ranges *ranges,
if (ranges->nranges == 0) if (ranges->nranges == 0)
return false; return false;
minvalue = ranges->values[0];
maxvalue = ranges->values[2 * ranges->nranges - 1];
/* /*
* Otherwise, need to compare the new value with boundaries of all the * Otherwise, need to compare the new value with boundaries of all the
* ranges. First check if it's less than the absolute minimum, which is * ranges. First check if it's less than the absolute minimum, which is