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 a caller would
rather leave it to the memory context machinery to free the result,
it can pass NULL as the "may_free" argument (or just ignore its
value).

Oversight in commit 87f61f0c82.

Reported-by: Tender Wang <tndrwang@gmail.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Suggested-by: Andres Freund <andres@anarazel.de>
Suggested-by: Tom Lane <tgl@sss.pgh.pa.us>
Author: Sami Imseih <samimseih@gmail.com>
Discussion: https://postgr.es/m/CAHewXNkJKdwb3D5OnksrdOqzqUnXUEMpDam1TPW0vfUkW%3D7jUw%40mail.gmail.com
Discussion: https://postgr.es/m/5684f479-858e-4c5d-b8f5-bcf05de1f909%40gmail.com

diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index f9fa874b79f2232ba9eeddbc07241ba2892e5743..82061247988110aa8fbf75cdd1aaf9c74e04dd69 100644 (file)
--- 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 eb8ccbaa62858473e6c5955132f83be4235fa58a..b67da88c7dc22b9f2e085e320a6090fb4d674129 100644 (file)
--- 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 the result in the caller's context and aren't
+                * caching it, the caller can safely pfree() it.
+                */
+               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 04fe13e64c69350e78e9d17f8c5e135ec3394eb0..73461c9bca5907114e570e9240d6ead25a1c5500 100644 (file)
--- 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 933dcb5cae51e096d5a5778da4e38910fb80ebb4..f1846d3236ccf9fb3d0961f66d346a9f351d045d 100644 (file)
--- 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 e6b84283c6c8eb5a8acf3dd4d80b6dbc0bd20596..d47d05e3d922cc2130ec75b89df883aeb0578ac4 100644 (file)
--- 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 bc8c43b96aa08de77dafbcb8e381e3b8325a039d..b2ca28f83ba8a2e09466cf08229e554e462cce9e 100644 (file)
--- 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 168ef8f8f451fee15a7735ab03c83077358a461b..0d00dd5d93aa519863d01c2c4e8d7ffcc928464b 100644 (file)
--- 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 3277cf88a4eb7d30115c732ccc834dbce05d6905..3eaf3e0390fc0bec6e75a6f2942837f96a5b0387 100644 (file)
--- 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 2786a7c5ffbbd47b40ecd2198faf72ecf9400082..dfa2e8376382ab8c61d6b7f8a549dbb64945232d 100644 (file)
--- 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 c0496f2c69c8dd9ac4ba10f278f20f63c52e8abc..fe463faaf63937434f07353600de6250d63b8612 100644 (file)
--- 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 2ef0e903745bbd83df8b679502adfef2736f9651..5c4871ed37cacac11c410feb2f1ee769398b7f1f 100644 (file)
--- 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);
 }
 
 /*--------------------------------------------------------------------------