Re: Fix possible bogus array out of bonds (src/backend/access/brin/brin_minmax_multi.c)

On Sat, 27 Aug 2022 at 01:29, Ranier Vilela <ranier.vf@gmail.com> wrote:
> At function has_matching_range, if variable ranges->nranges == 0,
> we exit quickly with a result equal to false.
>
> This means that nranges can be zero.
> It occurs then that it is possible then to occur an array out of bonds, in the initialization of the variable maxvalue.
> So if nranges is equal to zero, there is no need to initialize minvalue and maxvalue.

I think there's more strange coding in the same file that might need
addressed, for example, AssertCheckRanges() does:

if (ranges->nranges == 0)
break;

from within the first for() loop.  Why can't that check be outside of
the loop. Nothing seems to make any changes to that field from within
the loop.

Also, in the final loop of the same function there's:

if (ranges->nsorted == 0)
break;

It's not very obvious to me why we don't only run that loop when
ranges->nsorted > 0.  Also, isn't it an array overrun to access:

Datum value = ranges->values[2 * ranges->nranges + i];

If there's only 1 range stored in the array, then there should be 2
elements, but that code will try to access the 3rd element with
ranges->values[2].

This is not so critical, but I'll note it down anyway.  The following
looks a bit suboptimal in brin_minmax_multi_summary_out():

StringInfoData str;

initStringInfo(&str);

a = FunctionCall1(&fmgrinfo, ranges_deserialized->values[idx++]);

appendStringInfoString(&str, DatumGetCString(a));

b = cstring_to_text(str.data);

Why do we need a StringInfoData there?  Why not just do:

b = cstring_to_text(DatumGetCString(a)); ?

That requires less memcpy()s and pallocs().