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

On Wed, Sep 04, 2024 at 03:14:34PM -0300, Ranier Vilela wrote:

The commit 7949d95 <http://7949d9594582ab49dee221e1db1aa5401ace49d4&gt;, 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.

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

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

On Wed, Sep 04, 2024 at 03:14:34PM -0300, Ranier Vilela wrote:

The commit 7949d95 <http://7949d9594582ab49dee221e1db1aa5401ace49d4&gt;,

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

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

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

On Wed, Sep 04, 2024 at 03:14:34PM -0300, Ranier Vilela wrote:

The commit 7949d95 <http://7949d9594582ab49dee221e1db1aa5401ace49d4&gt;,

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

On Thu, Sep 05, 2024 at 09:25:11AM -0300, Ranier Vilela wrote:

Please, disregard the first patch, it contains a bug.
New version attached, v1.

These are wrong, because they are changing code paths where we know
that stats kinds should be set. For custom stats, particularly, this
would silently skip doing something for code paths that would expect
an action to be taken. (I thought originally about using some
elog(ERRORs) in these areas, refrained from it.)

The change in pgstat_write_statsfile() is equally unnecessary: we are
not going to write to the pgstats file an entry that we have not found
previously, as per the knowledge that these would be compiled with the
code for the builtin code, or added at startup for the custom ones.

I'd suggest to study the code a bit more. Perhaps more documentation
is required, not sure about that yet.
--
Michael