Skip to content

Commit

Permalink
Fix oversights in array manipulation.
Browse files Browse the repository at this point in the history
The nested-arrays code path in ExecEvalArrayExpr() used palloc to
allocate the result array, whereas every other array-creating function
has used palloc0 since 18c0b4e.  This mostly works, but unused bits
past the end of the nulls bitmap may end up undefined.  That causes
valgrind complaints with -DWRITE_READ_PARSE_PLAN_TREES, and could
cause planner misbehavior as cited in 18c0b4e.  There seems no very
good reason why we should strive to avoid palloc0 in just this one case,
so fix it the easy way with s/palloc/palloc0/.

While looking at that I noted that we also failed to check for overflow
of "nbytes" and "nitems" while summing the sizes of the sub-arrays,
potentially allowing a crash due to undersized output allocation.
For "nbytes", follow the policy used by other array-munging code of
checking for overflow after each addition.  (As elsewhere, the last
addition of the array's overhead space doesn't need an extra check,
since palloc itself will catch a value between 1Gb and 2Gb.)
For "nitems", there's no very good reason to sum the inputs at all,
since we can perfectly well use ArrayGetNItems' result instead of
ignoring it.

Per discussion of this bug, also remove redundant zeroing of the
nulls bitmap in array_set_element and array_set_slice.

Patch by Alexander Lakhin and myself, per bug #17858 from Alexander
Lakhin; thanks also to Richard Guo.  These bugs are a dozen years old,
so back-patch to all supported branches.

Discussion: https://postgr.es/m/17858-8fd287fd3663d051@postgresql.org
  • Loading branch information
tglsfdc committed Mar 26, 2023
1 parent 8fd5aa7 commit 11213d4
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 8 deletions.
13 changes: 9 additions & 4 deletions src/backend/executor/execExprInterp.c
Expand Up @@ -2706,7 +2706,7 @@ ExecEvalArrayExpr(ExprState *state, ExprEvalStep *op)
{
/* Must be nested array expressions */
int nbytes = 0;
int nitems = 0;
int nitems;
int outer_nelems = 0;
int elem_ndims = 0;
int *elem_dims = NULL;
Expand Down Expand Up @@ -2801,9 +2801,14 @@ ExecEvalArrayExpr(ExprState *state, ExprEvalStep *op)
subbitmaps[outer_nelems] = ARR_NULLBITMAP(array);
subbytes[outer_nelems] = ARR_SIZE(array) - ARR_DATA_OFFSET(array);
nbytes += subbytes[outer_nelems];
/* check for overflow of total request */
if (!AllocSizeIsValid(nbytes))
ereport(ERROR,
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("array size exceeds the maximum allowed (%d)",
(int) MaxAllocSize)));
subnitems[outer_nelems] = ArrayGetNItems(this_ndims,
ARR_DIMS(array));
nitems += subnitems[outer_nelems];
havenulls |= ARR_HASNULL(array);
outer_nelems++;
}
Expand Down Expand Up @@ -2837,7 +2842,7 @@ ExecEvalArrayExpr(ExprState *state, ExprEvalStep *op)
}

/* check for subscript overflow */
(void) ArrayGetNItems(ndims, dims);
nitems = ArrayGetNItems(ndims, dims);
ArrayCheckBounds(ndims, dims, lbs);

if (havenulls)
Expand All @@ -2851,7 +2856,7 @@ ExecEvalArrayExpr(ExprState *state, ExprEvalStep *op)
nbytes += ARR_OVERHEAD_NONULLS(ndims);
}

result = (ArrayType *) palloc(nbytes);
result = (ArrayType *) palloc0(nbytes);
SET_VARSIZE(result, nbytes);
result->ndim = ndims;
result->dataoffset = dataoffset;
Expand Down
6 changes: 2 additions & 4 deletions src/backend/utils/adt/arrayfuncs.c
Expand Up @@ -2459,8 +2459,7 @@ array_set_element(Datum arraydatum,
{
bits8 *newnullbitmap = ARR_NULLBITMAP(newarray);

/* Zero the bitmap to take care of marking inserted positions null */
MemSet(newnullbitmap, 0, (newnitems + 7) / 8);
/* palloc0 above already marked any inserted positions as nulls */
/* Fix the inserted value */
if (addedafter)
array_set_isnull(newnullbitmap, newnitems - 1, isNull);
Expand Down Expand Up @@ -3076,8 +3075,7 @@ array_set_slice(Datum arraydatum,
bits8 *newnullbitmap = ARR_NULLBITMAP(newarray);
bits8 *oldnullbitmap = ARR_NULLBITMAP(array);

/* Zero the bitmap to handle marking inserted positions null */
MemSet(newnullbitmap, 0, (nitems + 7) / 8);
/* palloc0 above already marked any inserted positions as nulls */
array_bitmap_copy(newnullbitmap, addedbefore,
oldnullbitmap, 0,
itemsbefore);
Expand Down

0 comments on commit 11213d4

Please sign in to comment.