public inbox for [email protected]
help / color / mirror / Atom feedFrom: Nathan Bossart <[email protected]>
Subject: [PATCH v3 1/1] Fix double-free in pg_stat_autovacuum_scores.
Date: Thu, 9 Apr 2026 11:20:21 -0500
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)
--KrZsvcbSG1UCPWv8--
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected]
Subject: Re: [PATCH v3 1/1] Fix double-free in pg_stat_autovacuum_scores.
In-Reply-To: <no-message-id-602204@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox