Re: Avoid overflowed array index (src/backend/utils/activity/pgstat.c)

m qua., 4 de set. de 2024 às 20:58, Michael Paquier <michael(at)paquier(dot)xyz>
escreveu:

> On Wed, Sep 04, 2024 at 03:14:34PM -0300, Ranier Vilela wrote:
> > The commit 7949d95 <http://7949d9594582ab49dee221e1db1aa5401ace49d4>,
> left
> > out an oversight.
> >
> > The report is:
> > CID 1559468: (#1 of 1): Overflowed array index read (INTEGER_OVERFLOW)
> >
> > I think that Coverity is right.
> > In the function *pgstat_read_statsfile* It is necessary to first check
> > whether it is the most restrictive case.
> >
> > Otherwise, if PgStat_Kind is greater than 11, a negative index may occur.
>
> You are missing the fact that there is a call to
> pgstat_is_kind_valid() a couple of lines above, meaning that we are
> sure that the kind ID we are dealing with is within the range [1,11]
> for built-in kinds or [128,256] for the custom kinds, so any ID not
> within the first range would just be within the second range.
>
Yeah, it seems that I and Coverity are mistaken about this warning.
Sorry for the noise.

>
> Speaking of which, I am spotting two possible pointer dereferences
> when reading the stats file if we are loading custom stats that do not
> exist anymore compared to the moment when they were written, for two
> cases:
> - Fixed-numbered stats entries.
> - Named entries, like replication slot stats, but for the custom case.
> It would mean that we'd crash at startup when reading stats depending
> on how shared_preload_libraries has changed, which is not the original
> intention. The patch includes details to inform what was found
> wrong with two new WARNING messages. Will fix in a bit, attaching it
> for now.
>
> Kind of interesting that your tool did not spot that, and missed the
> two I have noticed considering that we're dealing with the same code
> paths. The community coverity did not complain on any of them, AFAIK.
>
Yeah, Coverity do not spot this.

After reading the code more carefully, I found some possible issues.
I think it's worth reviewing the attached patch more carefully.

Therefore, I attach the patch for your consideration,
that tries to fix these issues.

best regards,
Ranier Vilela