[Bug] Heap Use After Free in parallel_vacuum_reset_dead_items Function
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
Attachments:
parallel_vacuum_fix.patchapplication/octet-stream; name=parallel_vacuum_fix.patchDownload+2-3
Import Notes
Reply to msg id not found:
On Tue, Nov 26, 2024 at 1:58 AM Vallimaharajan G <
vallimaharajan.gs@zohocorp.com> wrote:
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);
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
Attachments:
v2-fix-parallel-vacuum.patchtext/x-patch; charset=US-ASCII; name=v2-fix-parallel-vacuum.patchDownload+8-13
On Tue, Nov 26, 2024 at 4:53 PM John Naylor <johncnaylorls@gmail.com> 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