From: | Ranier Vilela <ranier(dot)vf(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Avoid overflowed array index (src/backend/utils/activity/pgstat.c) |
Date: | 2024-09-05 12:18:50 |
Message-ID: | CAEudQAqrKXoys-aSOd4ZUPYzNGU93-TJ9A6HuDCKarEVgw58cQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0002-fix-assorted-issues-pgstat.patch | application/octet-stream | 4.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Ranier Vilela | 2024-09-05 12:25:11 | Re: Avoid overflowed array index (src/backend/utils/activity/pgstat.c) |
Previous Message | Alvaro Herrera | 2024-09-05 12:14:48 | Re: Jargon and acronyms on this mailing list |