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

Em seg., 5 de set. de 2022 às 07:15, David Rowley <dgrowleyml(at)gmail(dot)com>
escreveu:

> On Sat, 3 Sept 2022 at 00:37, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> wrote:
> >> But +1 to fix this and other issues even if they would never crash.
>
> Yeah, I don't think any of this coding would lead to a crash, but it's
> pretty weird coding and we should fix it.
>
> > 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.
>
> I've ripped out #4 and #6 for now. I think we should do #6 in master
> only, probably as part of a wider cleanup of StringInfo misusages.
>
> I also spent some time trying to ensure I understand this code
> correctly. I was unable to work out what the nsorted field was from
> just the comments. I needed to look at the code to figure it out, so I
> think the comments for that field need to be improved. A few of the
> others were not that clear either. I hope I've improved them in the
> attached.
>
> I was also a bit confused at various other comments. e.g:
>
> /*
> * Is the value greater than the maxval? If yes, we'll recurse to the
> * right side of range array.
> */
>
The second comment in the v3 patch, must be:
/*
* Is the value greater than the maxval? If yes, we'll recurse
* to the right side of the range array.
*/

I think this is copy-and-paste thinko with the word "minval".

>
> I don't see any sort of recursion going on here. All I see are
> skipping of values that are out of bounds of the lower bound of the
> lowest range, and above the upper bound of the highest range.
>
I think this kind recursion, because the loop is restarted
with:
start = (midpoint + 1);
continue;

>
> I propose to backpatch the attached into v14 tomorrow morning (about
> 12 hours from now).
>
> The only other weird coding I found was in brin_range_deserialize:
>
> for (i = 0; (i < nvalues) && (!typbyval); i++)
>
> I imagine most compilers would optimize that so that the typbyval
> check is done before the first loop and not done on every loop, but I
> don't think that coding practice helps the human readers out much. I
> left that one alone, for now.
>
Yeah, I prefer write:
if (!typbyval)
{
for (i = 0; (i < nvalues); i++)
}

regards,
Ranier Vilela