Avoid core dump in pgstat_read_statsfile()
Hi hackers,
please find attached a patch to $SUBJECT.
Indeed one could generate a core dump similar to:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x0000000000a7a7a7 in pgstat_init_entry (kind=129, shhashent=0x7a55845cfd98) at pgstat_shmem.c:314
314 chunk = dsa_allocate0(pgStatLocal.dsa, pgstat_get_kind_info(kind)->shared_size);
(gdb) bt
#0 0x0000000000a7a7a7 in pgstat_init_entry (kind=129, shhashent=0x7a55845cfd98) at pgstat_shmem.c:314
#1 0x0000000000a72935 in pgstat_read_statsfile () at pgstat.c:1982
#2 0x0000000000a700a8 in pgstat_restore_stats () at pgstat.c:507
by:
1. creating a custom stat kind (non fixed_amount and write_to_file enabled)
2. generate stats
3. stop the engine
4. change the custom PGSTAT_KIND id linked to 1., compile and install
5. re-start the engine
I think that a check on pgstat_get_kind_info() is missing for this scenario, the
patch adds it. Such a check already exists for PGSTAT_FILE_ENTRY_FIXED and
for stats entry identified by name on disk, but not for PGSTAT_FILE_ENTRY_HASH.
The v18 existing checks mentioned above as well as the new check were missing
in pre-18 but I don't think it's worth a back-patch as the issue is unlikely to
occur without custom stats. Adding a v18 open item then.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Attachments:
v1-0001-Avoid-core-dump-in-pgstat_read_statsfile.patchtext/x-diff; charset=us-asciiDownload
From b80bcbcf05e44d31cc90c1de2e3200899db1294f Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Wed, 23 Apr 2025 07:19:44 +0000
Subject: [PATCH v1] Avoid core dump in pgstat_read_statsfile()
Adding a check that pgstat_get_kind_info() is not NULL when loading the
stats file. Without this extra check a non fixed_amount custom stats kind that
has write_to_file enabled would produce a core dump should its PGSTAT_KIND ID
change before a re-start.
---
src/backend/utils/activity/pgstat.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c
index 21bdff106a9..d735a2c1a58 100644
--- a/src/backend/utils/activity/pgstat.c
+++ b/src/backend/utils/activity/pgstat.c
@@ -1908,6 +1908,13 @@ pgstat_read_statsfile(void)
key.objid, t);
goto error;
}
+
+ if (!pgstat_get_kind_info(key.kind))
+ {
+ elog(WARNING, "could not find information of kind %u for entry of type %c",
+ key.kind, t);
+ goto error;
+ }
}
else
{
--
2.34.1
On Wed, Apr 23, 2025 at 08:01:40AM +0000, Bertrand Drouvot wrote:
I think that a check on pgstat_get_kind_info() is missing for this scenario, the
patch adds it. Such a check already exists for PGSTAT_FILE_ENTRY_FIXED and
for stats entry identified by name on disk, but not for PGSTAT_FILE_ENTRY_HASH.
It is, indeed. This can be reproduced with what's in core, just
disable pgstat_register_inj_fixed() to bypass the check for fixed
entries that would be read first if the module is removed from
shared_preload_libraries in injection_points.c and we are good to go.
I can see that you have copy-pasted the elog from the two other entry
types. This is incomplete here because we are dealing with an entry
in the dshash and we know its full key already: kind, database OID,
object ID. And this can provide more information to help with the
debugging. I have added this information, and applied the result.
The v18 existing checks mentioned above as well as the new check were missing
in pre-18 but I don't think it's worth a back-patch as the issue is unlikely to
occur without custom stats. Adding a v18 open item then.
The check based on the kind ID should be enough, because this ensures
that we'll have the PgStat_KindInfo to rely on. So stable branches
are OK as they are.
--
Michael