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

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.
*/

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 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.

David