Here's what I have staged for commit. -- nathan
>From 69e545eb08a14c00ac361bfc4a2db85cd56a8226 Mon Sep 17 00:00:00 2001 From: Nathan Bossart <[email protected]> Date: Thu, 9 Apr 2026 11:20:21 -0500 Subject: [PATCH v3 1/1] Fix double-free in pg_stat_autovacuum_scores.
Presently, relation_needs_vacanalyze() unconditionally frees the pgstat entry returned by pgstat_fetch_stat_tabentry_ext(). This behavior was first added by commit 02502c1bca to avoid memory leakage in autovacuum. While this is fine for autovacuum since it forces stats_fetch_consistency to "none", it is not okay for other callers that use "cache" or "snapshot". This manifests as a double-free when pg_stat_autovacuum_scores is called multiple times in the same transaction. To fix, add a "bool *may_free" parameter to pgstat_fetch_stat_tabentry_ext() that returns whether it is safe for the caller to explicitly pfree() the result if desired. If a caller would rather leave it to the memory context machinery to free the result, it can pass NULL for the "may_free" argument (or just ignore its value). Oversight in commit 87f61f0c82. Reported-by: Tender Wang <[email protected]> Reported-by: Alexander Lakhin <[email protected]> Suggested-by: Andres Freund <[email protected]> Suggested-by: Tom Lane <[email protected]> Author: Sami Imseih <[email protected]> Discussion: https://postgr.es/m/CAHewXNkJKdwb3D5OnksrdOqzqUnXUEMpDam1TPW0vfUkW%3D7jUw%40mail.gmail.com Discussion: https://postgr.es/m/5684f479-858e-4c5d-b8f5-bcf05de1f909%40gmail.com --- src/backend/postmaster/autovacuum.c | 7 +++++-- src/backend/utils/activity/pgstat.c | 18 +++++++++++++++++- src/backend/utils/activity/pgstat_backend.c | 3 ++- src/backend/utils/activity/pgstat_database.c | 2 +- src/backend/utils/activity/pgstat_function.c | 2 +- src/backend/utils/activity/pgstat_relation.c | 12 +++++++----- src/backend/utils/activity/pgstat_replslot.c | 3 ++- .../utils/activity/pgstat_subscription.c | 2 +- src/include/pgstat.h | 3 ++- src/include/utils/pgstat_internal.h | 3 ++- .../test_custom_stats/test_custom_var_stats.c | 3 ++- 11 files changed, 42 insertions(+), 16 deletions(-) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index f9fa874b79f..82061247988 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -3080,6 +3080,7 @@ relation_needs_vacanalyze(Oid relid, PgStat_StatTabEntry *tabentry; bool force_vacuum; bool av_enabled; + bool may_free = false; /* constants from reloptions or GUC variables */ int vac_base_thresh, @@ -3246,7 +3247,7 @@ relation_needs_vacanalyze(Oid relid, * forced. */ tabentry = pgstat_fetch_stat_tabentry_ext(classForm->relisshared, - relid); + relid, &may_free); if (!tabentry) return; @@ -3327,7 +3328,9 @@ relation_needs_vacanalyze(Oid relid, anltuples, anlthresh, scores->anl, scores->xid, scores->mxid); - pfree(tabentry); + /* Avoid leaking pgstat entries until the end of autovacuum. */ + if (may_free) + pfree(tabentry); } /* diff --git a/src/backend/utils/activity/pgstat.c b/src/backend/utils/activity/pgstat.c index eb8ccbaa628..a59fb7c66b6 100644 --- a/src/backend/utils/activity/pgstat.c +++ b/src/backend/utils/activity/pgstat.c @@ -960,7 +960,7 @@ pgstat_clear_snapshot(void) } void * -pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid) +pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid, bool *may_free) { PgStat_HashKey key = {0}; PgStat_EntryRef *entry_ref; @@ -971,6 +971,13 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid) Assert(IsUnderPostmaster || !IsPostmasterEnvironment); Assert(!kind_info->fixed_amount); + /* + * Initialize *may_free to false. We'll change it to true later if we end + * up allocating the result in the caller's context and not caching it. + */ + if (may_free) + *may_free = false; + pgstat_prep_snapshot(); key.kind = kind; @@ -1024,7 +1031,16 @@ pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid) * repeated accesses. */ if (pgstat_fetch_consistency == PGSTAT_FETCH_CONSISTENCY_NONE) + { stats_data = palloc(kind_info->shared_data_len); + + /* + * Since we allocated in the caller's context and aren't caching the + * result, it is safe to pfree() the result. + */ + if (may_free) + *may_free = true; + } else stats_data = MemoryContextAlloc(pgStatLocal.snapshot.context, kind_info->shared_data_len); diff --git a/src/backend/utils/activity/pgstat_backend.c b/src/backend/utils/activity/pgstat_backend.c index 04fe13e64c6..73461c9bca5 100644 --- a/src/backend/utils/activity/pgstat_backend.c +++ b/src/backend/utils/activity/pgstat_backend.c @@ -95,7 +95,8 @@ pgstat_fetch_stat_backend(ProcNumber procNumber) PgStat_Backend *backend_entry; backend_entry = (PgStat_Backend *) pgstat_fetch_entry(PGSTAT_KIND_BACKEND, - InvalidOid, procNumber); + InvalidOid, procNumber, + NULL); return backend_entry; } diff --git a/src/backend/utils/activity/pgstat_database.c b/src/backend/utils/activity/pgstat_database.c index 933dcb5cae5..f1846d3236c 100644 --- a/src/backend/utils/activity/pgstat_database.c +++ b/src/backend/utils/activity/pgstat_database.c @@ -288,7 +288,7 @@ PgStat_StatDBEntry * pgstat_fetch_stat_dbentry(Oid dboid) { return (PgStat_StatDBEntry *) - pgstat_fetch_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid); + pgstat_fetch_entry(PGSTAT_KIND_DATABASE, dboid, InvalidOid, NULL); } void diff --git a/src/backend/utils/activity/pgstat_function.c b/src/backend/utils/activity/pgstat_function.c index e6b84283c6c..d47d05e3d92 100644 --- a/src/backend/utils/activity/pgstat_function.c +++ b/src/backend/utils/activity/pgstat_function.c @@ -245,5 +245,5 @@ PgStat_StatFuncEntry * pgstat_fetch_stat_funcentry(Oid func_id) { return (PgStat_StatFuncEntry *) - pgstat_fetch_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id); + pgstat_fetch_entry(PGSTAT_KIND_FUNCTION, MyDatabaseId, func_id, NULL); } diff --git a/src/backend/utils/activity/pgstat_relation.c b/src/backend/utils/activity/pgstat_relation.c index bc8c43b96aa..b2ca28f83ba 100644 --- a/src/backend/utils/activity/pgstat_relation.c +++ b/src/backend/utils/activity/pgstat_relation.c @@ -61,7 +61,8 @@ pgstat_copy_relation_stats(Relation dst, Relation src) PgStat_EntryRef *dst_ref; srcstats = pgstat_fetch_stat_tabentry_ext(src->rd_rel->relisshared, - RelationGetRelid(src)); + RelationGetRelid(src), + NULL); if (!srcstats) return; @@ -468,20 +469,21 @@ pgstat_update_heap_dead_tuples(Relation rel, int delta) PgStat_StatTabEntry * pgstat_fetch_stat_tabentry(Oid relid) { - return pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid); + return pgstat_fetch_stat_tabentry_ext(IsSharedRelation(relid), relid, NULL); } /* * More efficient version of pgstat_fetch_stat_tabentry(), allowing to specify - * whether the to-be-accessed table is a shared relation or not. + * whether the to-be-accessed table is a shared relation or not. This version + * also returns whether the caller can pfree() the result if desired. */ PgStat_StatTabEntry * -pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid) +pgstat_fetch_stat_tabentry_ext(bool shared, Oid reloid, bool *may_free) { Oid dboid = (shared ? InvalidOid : MyDatabaseId); return (PgStat_StatTabEntry *) - pgstat_fetch_entry(PGSTAT_KIND_RELATION, dboid, reloid); + pgstat_fetch_entry(PGSTAT_KIND_RELATION, dboid, reloid, may_free); } /* diff --git a/src/backend/utils/activity/pgstat_replslot.c b/src/backend/utils/activity/pgstat_replslot.c index 168ef8f8f45..0d00dd5d93a 100644 --- a/src/backend/utils/activity/pgstat_replslot.c +++ b/src/backend/utils/activity/pgstat_replslot.c @@ -208,7 +208,8 @@ pgstat_fetch_replslot(NameData slotname) if (idx != -1) slotentry = (PgStat_StatReplSlotEntry *) pgstat_fetch_entry(PGSTAT_KIND_REPLSLOT, - InvalidOid, idx); + InvalidOid, idx, + NULL); LWLockRelease(ReplicationSlotControlLock); diff --git a/src/backend/utils/activity/pgstat_subscription.c b/src/backend/utils/activity/pgstat_subscription.c index 3277cf88a4e..3eaf3e0390f 100644 --- a/src/backend/utils/activity/pgstat_subscription.c +++ b/src/backend/utils/activity/pgstat_subscription.c @@ -107,7 +107,7 @@ PgStat_StatSubEntry * pgstat_fetch_stat_subscription(Oid subid) { return (PgStat_StatSubEntry *) - pgstat_fetch_entry(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid); + pgstat_fetch_entry(PGSTAT_KIND_SUBSCRIPTION, InvalidOid, subid, NULL); } /* diff --git a/src/include/pgstat.h b/src/include/pgstat.h index 2786a7c5ffb..dfa2e837638 100644 --- a/src/include/pgstat.h +++ b/src/include/pgstat.h @@ -763,7 +763,8 @@ extern void pgstat_twophase_postabort(FullTransactionId fxid, uint16 info, extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry(Oid relid); extern PgStat_StatTabEntry *pgstat_fetch_stat_tabentry_ext(bool shared, - Oid reloid); + Oid reloid, + bool *may_free); extern PgStat_TableStatus *find_tabstat_entry(Oid rel_id); diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index c0496f2c69c..fe463faaf63 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -685,7 +685,8 @@ extern PgStat_EntryRef *pgstat_prep_pending_entry(PgStat_Kind kind, Oid dboid, extern PgStat_EntryRef *pgstat_fetch_pending_entry(PgStat_Kind kind, Oid dboid, uint64 objid); -extern void *pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid); +extern void *pgstat_fetch_entry(PgStat_Kind kind, Oid dboid, uint64 objid, + bool *may_free); extern void pgstat_snapshot_fixed(PgStat_Kind kind); diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c index 2ef0e903745..5c4871ed37c 100644 --- a/src/test/modules/test_custom_stats/test_custom_var_stats.c +++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c @@ -494,7 +494,8 @@ test_custom_stats_var_fetch_entry(const char *stat_name) return (PgStat_StatCustomVarEntry *) pgstat_fetch_entry(PGSTAT_KIND_TEST_CUSTOM_VAR_STATS, InvalidOid, - PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name)); + PGSTAT_CUSTOM_VAR_STATS_IDX(stat_name), + NULL); } /*-------------------------------------------------------------------------- -- 2.50.1 (Apple Git-155)
