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

Em qui., 1 de set. de 2022 às 21:27, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> On Sat, 27 Aug 2022 at 01:29, Ranier Vilela <ranier(dot)vf(at)gmail(dot)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].
>
Yeah, it seems to me that both nranges and nsorted are invariant there,
so we can safely avoid loops.

>
> 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().
>
I agree that StringInfoData is not needed there.
Is it strange to convert char * to only store a temporary str.data.

Why not?
astate_values = accumArrayResult(astate_values,
PointerGetDatum(a),
false,
TEXTOID,
CurrentMemoryContext);

Is it possible to avoid cstring_to_text conversion?

regards,
Ranier Vilela