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

Em sex., 2 de set. de 2022 às 09:01, Justin Pryzby <pryzby(at)telsasoft(dot)com>
escreveu:

> On Mon, Aug 29, 2022 at 10:06:55AM +0900, Kyotaro Horiguchi wrote:
> > At Fri, 26 Aug 2022 10:28:50 -0300, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
> wrote in
> > > 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.
> > >
> > > The patch tries to fix it, avoiding possible errors by using maxvalue.
> >
> > However it seems that nranges will never be zero, still the fix looks
> > good to me since it is generally allowed to be zero. I don't find a
> > similar mistake related to Range.nranges.
>
> Actually, the nranges==0 branch is hit during regression tests:
>
> https://coverage.postgresql.org/src/backend/access/brin/brin_minmax_multi.c.gcov.html
>
> I'm not sure, but I *suspect* that compilers usually check
> ranges->nranges==0
> before reading ranges->values[2 * ranges->nranges - 1];
>
> Especially since it's a static function.
>
> Even if they didn't (say, under -O0), values[-1] would probably point to
> a palloc header, which would be enough to "not crash" before returning
> one line later.
>
> But +1 to fix this and other issues even if they would never crash.
>
Thanks Justin.

Based on comments by David, I made a new patch.
To simplify I've included the 0001 in the 0002 patch.

Summary:
1. Once that ranges->nranges is invariant, avoid the loop if
ranges->nranges <= 0.
This matches the current behavior.

2. Once that ranges->nsorted is invariant, avoid the loop if
ranges->nsorted <= 0.
This matches the current behavior.

3. Remove the invariant cxt from ranges->nsorted loop.

4. Avoid possible overflows when using int to store length strings.

5. Avoid possible out-of-bounds when ranges->nranges == 0.

6. Avoid overhead when using unnecessary StringInfoData to convert Datum a
to Text b.

Attached is 0002.

regards,
Ranier Vilela