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

Em qui., 5 de set. de 2024 às 09:18, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:

> 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.
>
Please, disregard the first patch, it contains a bug.
New version attached, v1.

best regards,
Ranier Vilela