Subject: [Bug] Heap Use After Free in parallel_vacuum_reset_dead_items Function

From: Vallimaharajan G <[email-masked]>
Date: Mon, 25 Nov 2024 23:57:07 +0530

Hi Developers,
     We have discovered a bug in the parallel_vacuum_reset_dead_items function in PG v17.2. Specifically:

TidStoreDestroy(dead_items) frees the dead_items pointer.

The pointer is reinitialized using TidStoreCreateShared().

However, the code later accesses the freed pointer instead of the newly reinitialized pvs->dead_items, as seen in these lines:

           pvs->shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(dead_items));

           pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items);

       This can lead to undefined behaviour or crashes due to the use of invalid memory.

       Caught this issue while running the existing regression tests from vacuum_parallel.sql with our custom malloc allocator implementation. 

       For your reference, we have previously shared our custom malloc allocator implementation in a separate bug fix. (message ID: ).

Failed regression:

SET max_parallel_maintenance_workers TO 4;

SET min_parallel_index_scan_size TO '128kB';

CREATE TABLE parallel_vacuum_table (a int) WITH (autovacuum_enabled = off);

INSERT INTO parallel_vacuum_table SELECT i from generate_series(1, 10000) i;

CREATE INDEX regular_sized_index ON parallel_vacuum_table(a);

CREATE INDEX typically_sized_index ON parallel_vacuum_table(a);

CREATE INDEX vacuum_in_leader_small_index ON parallel_vacuum_table((1));

DELETE FROM parallel_vacuum_table;

VACUUM (PARALLEL 4, INDEX_CLEANUP ON) parallel_vacuum_table;

Please let me know if you have any questions or would like further details.

Thanks & Regards,

Vallimaharajan G

Member Technical Staff

ZOHO Corporation

---

From: John Naylor <[email-masked]>
Date: Tue, 26 Nov 2024 16:53:45 +0700

On Tue, Nov 26, 2024 at 1:58 AM Vallimaharajan G <
[email-masked]> wrote:
> However, the code later accesses the freed pointer instead of the newly
reinitialized pvs->dead_items, as seen in these lines:
>
>            pvs->shared->dead_items_dsa_handle =
dsa_get_handle(TidStoreGetDSA(dead_items));
>            pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items);

Thanks for the report! I don't see any immediate evidence of deleterious
effects, but it's still sloppy. To reduce risk going forward, I think we
should always access this pointer via the struct rather than a separate
copy, quick attempt attached.

(BTW, it's normally discouraged to cross-post to different lists. Either
one is fine in this case.)

--
John Naylor
Amazon Web Services
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 793bd33cb4..667b1c54e1 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -2917,8 +2917,6 @@ dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *offsets,
 static void
 dead_items_reset(LVRelState *vacrel)
 {
-	TidStore   *dead_items = vacrel->dead_items;
-
 	if (ParallelVacuumIsActive(vacrel))
 	{
 		parallel_vacuum_reset_dead_items(vacrel->pvs);
@@ -2926,7 +2924,7 @@ dead_items_reset(LVRelState *vacrel)
 	}
 
 	/* Recreate the tidstore with the same max_bytes limitation */
-	TidStoreDestroy(dead_items);
+	TidStoreDestroy(vacrel->dead_items);
 	vacrel->dead_items = TidStoreCreateLocal(vacrel->dead_items_info->max_bytes, true);
 
 	/* Reset the counter */
diff --git a/src/backend/commands/vacuumparallel.c b/src/backend/commands/vacuumparallel.c
index 4fd6574e12..4b0669b1e6 100644
--- a/src/backend/commands/vacuumparallel.c
+++ b/src/backend/commands/vacuumparallel.c
@@ -247,7 +247,6 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
 	ParallelVacuumState *pvs;
 	ParallelContext *pcxt;
 	PVShared   *shared;
-	TidStore   *dead_items;
 	PVIndStats *indstats;
 	BufferUsage *buffer_usage;
 	WalUsage   *wal_usage;
@@ -378,11 +377,10 @@ parallel_vacuum_init(Relation rel, Relation *indrels, int nindexes,
 	shared->dead_items_info.max_bytes = vac_work_mem * 1024L;
 
 	/* Prepare DSA space for dead items */
-	dead_items = TidStoreCreateShared(shared->dead_items_info.max_bytes,
-									  LWTRANCHE_PARALLEL_VACUUM_DSA);
-	pvs->dead_items = dead_items;
-	shared->dead_items_handle = TidStoreGetHandle(dead_items);
-	shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(dead_items));
+	pvs->dead_items = TidStoreCreateShared(shared->dead_items_info.max_bytes,
+										   LWTRANCHE_PARALLEL_VACUUM_DSA);
+	shared->dead_items_handle = TidStoreGetHandle(pvs->dead_items);
+	shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(pvs->dead_items));
 
 	/* Use the same buffer size for all workers */
 	shared->ring_nbuffers = GetAccessStrategyBufferCount(bstrategy);
@@ -474,7 +472,6 @@ parallel_vacuum_get_dead_items(ParallelVacuumState *pvs, VacDeadItemsInfo **dead
 void
 parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
 {
-	TidStore   *dead_items = pvs->dead_items;
 	VacDeadItemsInfo *dead_items_info = &(pvs->shared->dead_items_info);
 
 	/*
@@ -482,13 +479,13 @@ parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
 	 * operating system. Then we recreate the tidstore with the same max_bytes
 	 * limitation we just used.
 	 */
-	TidStoreDestroy(dead_items);
+	TidStoreDestroy(pvs->dead_items);
 	pvs->dead_items = TidStoreCreateShared(dead_items_info->max_bytes,
 										   LWTRANCHE_PARALLEL_VACUUM_DSA);
 
 	/* Update the DSA pointer for dead_items to the new one */
-	pvs->shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(dead_items));
-	pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items);
+	pvs->shared->dead_items_dsa_handle = dsa_get_handle(TidStoreGetDSA(pvs->dead_items));
+	pvs->shared->dead_items_handle = TidStoreGetHandle(pvs->dead_items);
 
 	/* Reset the counter */
 	dead_items_info->num_items = 0;

---

From: John Naylor <[email-masked]>
Date: Thu, 5 Dec 2024 08:46:33 +0700

On Tue, Nov 26, 2024 at 4:53 PM John Naylor <[email-masked]> wrote:
> Thanks for the report! I don't see any immediate evidence of deleterious effects, but it's still sloppy. To reduce risk going forward, I think we should always access this pointer via the struct rather than a separate copy, quick attempt attached.

[removing -bugs]

I looked again and changed a few more places for consistency, and committed.

-- 
John Naylor
Amazon Web Services