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

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);
 }
 
 /*

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);

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;
 }

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

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);
 }

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);
 }
 
 /*

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);
 

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);
 }
 
 /*

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);
 
 

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);
 
 

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);
 }
 
 /*--------------------------------------------------------------------------