Avoid overflowed array index (src/backend/utils/activity/pgstat.c)
Hi.
Per Coverity.
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.
Patch attached.
best regards,
Ranier Vilela
Attachments:
0001-avoid-overflowed-array-index-pgstat.patchapplication/octet-stream; name=0001-avoid-overflowed-array-index-pgstat.patchDownload
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b2ca3f39b7..d1f7eebbdb 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1790,16 +1790,16 @@ pgstat_read_statsfile(XLogRecPtr redo)
}
/* Load back stats into shared memory */
- if (pgstat_is_kind_builtin(kind))
- ptr = ((char *) shmem) + info->shared_ctl_off +
- info->shared_data_off;
- else
+ if (pgstat_is_kind_custom(kind))
{
int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
ptr = ((char *) shmem->custom_data[idx]) +
info->shared_data_off;
}
+ else
+ ptr = ((char *) shmem) + info->shared_ctl_off +
+ info->shared_data_off;
if (!read_chunk(fpin, ptr, info->shared_data_len))
{
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.
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
Attachments:
pgstat-dereference.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index b2ca3f39b7..612158f2b9 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1781,6 +1781,12 @@ pgstat_read_statsfile(XLogRecPtr redo)
}
info = pgstat_get_kind_info(kind);
+ if (!info)
+ {
+ elog(WARNING, "could not find information of kind %u for entry of type %c",
+ kind, t);
+ goto error;
+ }
if (!info->fixed_amount)
{
@@ -1861,6 +1867,12 @@ pgstat_read_statsfile(XLogRecPtr redo)
}
kind_info = pgstat_get_kind_info(kind);
+ if (!kind_info)
+ {
+ elog(WARNING, "could not find information of kind %u for entry of type %c",
+ kind, t);
+ goto error;
+ }
if (!kind_info->from_serialized_name)
{
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>,
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
Attachments:
0002-fix-assorted-issues-pgstat.patchapplication/octet-stream; name=0002-fix-assorted-issues-pgstat.patchDownload
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 178b5ef65a..514d395885 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -834,7 +834,7 @@ pgstat_reset_of_kind(PgStat_Kind kind)
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
TimestampTz ts = GetCurrentTimestamp();
- if (kind_info->fixed_amount)
+ if (kind_info != NULL && kind_info->fixed_amount)
kind_info->reset_all_cb(ts);
else
pgstat_reset_entries_of_kind(kind, ts);
@@ -894,6 +894,10 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
void *stats_data;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+ /* nothing to do */
+ if (kind_info == NULL)
+ return NULL;
+
/* should be called from backends */
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(!kind_info->fixed_amount);
@@ -1110,14 +1114,14 @@ pgstat_build_snapshot(void)
* database or objects not associated with a database (e.g. shared
* relations).
*/
+ if (kind_info == NULL || p->dropped)
+ continue;
+
if (p->key.dboid != MyDatabaseId &&
p->key.dboid != InvalidOid &&
!kind_info->accessed_across_databases)
continue;
- if (p->dropped)
- continue;
-
Assert(pg_atomic_read_u32(&p->refcount) > 0);
stats_data = dsa_get_address(pgStatLocal.dsa, p->body);
@@ -1166,7 +1170,7 @@ static void
pgstat_build_snapshot_fixed(PgStat_Kind kind)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- int idx;
+ uint32 idx;
bool *valid;
/* Position in fixed_valid or custom_valid */
@@ -1198,7 +1202,9 @@ pgstat_build_snapshot_fixed(PgStat_Kind kind)
Assert(!valid[idx]);
- kind_info->snapshot_cb();
+ /* call callback if exist */
+ if (kind_info != NULL && kind_info->snapshot_cb)
+ kind_info->snapshot_cb();
Assert(!valid[idx]);
valid[idx] = true;
@@ -1238,12 +1244,17 @@ pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid, Oid objoid, bool *created
if (entry_ref->pending == NULL)
{
- size_t entrysize = pgstat_get_kind_info(kind)->pending_size;
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (kind_info != NULL)
+ {
+ size_t entrysize = kind_info->pending_size;
- Assert(entrysize != (size_t) -1);
+ Assert(entrysize != (size_t) -1);
- entry_ref->pending = MemoryContextAllocZero(pgStatPendingContext, entrysize);
- dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
+ entry_ref->pending = MemoryContextAllocZero(pgStatPendingContext, entrysize);
+ dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
+ }
}
return entry_ref;
@@ -1279,7 +1290,7 @@ pgstat_delete_pending_entry(PgStat_EntryRef *entry_ref)
/* !fixed_amount stats should be handled explicitly */
Assert(!pgstat_get_kind_info(kind)->fixed_amount);
- if (kind_info->delete_pending_cb)
+ if (kind_info != NULL && kind_info->delete_pending_cb)
kind_info->delete_pending_cb(entry_ref);
pfree(pending_data);
@@ -1320,8 +1331,13 @@ pgstat_flush_pending_entries(bool nowait)
bool did_flush;
dlist_node *next;
+ if (kind_info == NULL && kind_info->flush_pending_cb != NULL)
+ {
+ cur = next;
+ continue;
+ }
+
Assert(!kind_info->fixed_amount);
- Assert(kind_info->flush_pending_cb != NULL);
/* flush the stats, if possible */
did_flush = kind_info->flush_pending_cb(entry_ref, nowait);
@@ -1594,7 +1610,7 @@ pgstat_write_statsfile(XLogRecPtr redo)
while ((ps = dshash_seq_next(&hstat)) != NULL)
{
PgStatShared_Common *shstats;
- const PgStat_KindInfo *kind_info = NULL;
+ const PgStat_KindInfo *kind_info;
CHECK_FOR_INTERRUPTS();
@@ -1614,9 +1630,15 @@ pgstat_write_statsfile(XLogRecPtr redo)
continue;
}
- shstats = (PgStatShared_Common *) dsa_get_address(pgStatLocal.dsa, ps->body);
-
kind_info = pgstat_get_kind_info(ps->key.kind);
+ if (kind_info == NULL)
+ {
+ elog(WARNING, "found unknown stats entry %u/%u/%u",
+ ps->key.kind, ps->key.dboid, ps->key.objoid);
+ continue;
+ }
+
+ shstats = (PgStatShared_Common *) dsa_get_address(pgStatLocal.dsa, ps->body);
/* if not dropped the valid-entry refcount should exist */
Assert(pg_atomic_read_u32(&ps->refcount) > 0);
@@ -1811,7 +1833,7 @@ pgstat_read_statsfile(XLogRecPtr redo)
info->shared_data_off;
else
{
- int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
+ uint32 idx = kind - PGSTAT_KIND_CUSTOM_MIN;
ptr = ((char *) shmem->custom_data[idx]) +
info->shared_data_off;
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>,
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
Attachments:
v1-0002-fix-assorted-issues-pgstat.patchapplication/octet-stream; name=v1-0002-fix-assorted-issues-pgstat.patchDownload
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 178b5ef65a..97dc2e1ee2 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -834,7 +834,7 @@ pgstat_reset_of_kind(PgStat_Kind kind)
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
TimestampTz ts = GetCurrentTimestamp();
- if (kind_info->fixed_amount)
+ if (kind_info != NULL && kind_info->fixed_amount)
kind_info->reset_all_cb(ts);
else
pgstat_reset_entries_of_kind(kind, ts);
@@ -894,6 +894,10 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, Oid objoid)
void *stats_data;
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+ /* nothing to do */
+ if (kind_info == NULL)
+ return NULL;
+
/* should be called from backends */
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(!kind_info->fixed_amount);
@@ -1110,14 +1114,14 @@ pgstat_build_snapshot(void)
* database or objects not associated with a database (e.g. shared
* relations).
*/
+ if (kind_info == NULL || p->dropped)
+ continue;
+
if (p->key.dboid != MyDatabaseId &&
p->key.dboid != InvalidOid &&
!kind_info->accessed_across_databases)
continue;
- if (p->dropped)
- continue;
-
Assert(pg_atomic_read_u32(&p->refcount) > 0);
stats_data = dsa_get_address(pgStatLocal.dsa, p->body);
@@ -1166,7 +1170,7 @@ static void
pgstat_build_snapshot_fixed(PgStat_Kind kind)
{
const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
- int idx;
+ uint32 idx;
bool *valid;
/* Position in fixed_valid or custom_valid */
@@ -1198,7 +1202,9 @@ pgstat_build_snapshot_fixed(PgStat_Kind kind)
Assert(!valid[idx]);
- kind_info->snapshot_cb();
+ /* call callback if exist */
+ if (kind_info != NULL && kind_info->snapshot_cb)
+ kind_info->snapshot_cb();
Assert(!valid[idx]);
valid[idx] = true;
@@ -1238,12 +1244,17 @@ pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid, Oid objoid, bool *created
if (entry_ref->pending == NULL)
{
- size_t entrysize = pgstat_get_kind_info(kind)->pending_size;
+ const PgStat_KindInfo *kind_info = pgstat_get_kind_info(kind);
+
+ if (kind_info != NULL)
+ {
+ size_t entrysize = kind_info->pending_size;
- Assert(entrysize != (size_t) -1);
+ Assert(entrysize != (size_t) -1);
- entry_ref->pending = MemoryContextAllocZero(pgStatPendingContext, entrysize);
- dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
+ entry_ref->pending = MemoryContextAllocZero(pgStatPendingContext, entrysize);
+ dlist_push_tail(&pgStatPending, &entry_ref->pending_node);
+ }
}
return entry_ref;
@@ -1279,7 +1290,7 @@ pgstat_delete_pending_entry(PgStat_EntryRef *entry_ref)
/* !fixed_amount stats should be handled explicitly */
Assert(!pgstat_get_kind_info(kind)->fixed_amount);
- if (kind_info->delete_pending_cb)
+ if (kind_info != NULL && kind_info->delete_pending_cb)
kind_info->delete_pending_cb(entry_ref);
pfree(pending_data);
@@ -1320,8 +1331,13 @@ pgstat_flush_pending_entries(bool nowait)
bool did_flush;
dlist_node *next;
+ if (kind_info == NULL || kind_info->flush_pending_cb == NULL)
+ {
+ cur = next;
+ continue;
+ }
+
Assert(!kind_info->fixed_amount);
- Assert(kind_info->flush_pending_cb != NULL);
/* flush the stats, if possible */
did_flush = kind_info->flush_pending_cb(entry_ref, nowait);
@@ -1594,7 +1610,7 @@ pgstat_write_statsfile(XLogRecPtr redo)
while ((ps = dshash_seq_next(&hstat)) != NULL)
{
PgStatShared_Common *shstats;
- const PgStat_KindInfo *kind_info = NULL;
+ const PgStat_KindInfo *kind_info;
CHECK_FOR_INTERRUPTS();
@@ -1614,9 +1630,15 @@ pgstat_write_statsfile(XLogRecPtr redo)
continue;
}
- shstats = (PgStatShared_Common *) dsa_get_address(pgStatLocal.dsa, ps->body);
-
kind_info = pgstat_get_kind_info(ps->key.kind);
+ if (kind_info == NULL)
+ {
+ elog(WARNING, "found unknown stats entry %u/%u/%u",
+ ps->key.kind, ps->key.dboid, ps->key.objoid);
+ continue;
+ }
+
+ shstats = (PgStatShared_Common *) dsa_get_address(pgStatLocal.dsa, ps->body);
/* if not dropped the valid-entry refcount should exist */
Assert(pg_atomic_read_u32(&ps->refcount) > 0);
@@ -1811,7 +1833,7 @@ pgstat_read_statsfile(XLogRecPtr redo)
info->shared_data_off;
else
{
- int idx = kind - PGSTAT_KIND_CUSTOM_MIN;
+ uint32 idx = kind - PGSTAT_KIND_CUSTOM_MIN;
ptr = ((char *) shmem->custom_data[idx]) +
info->shared_data_off;
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