database/postgres
1
0
Fork 0
mirror of https://github.com/postgres/postgres.git synced 2025-07-12 21:01:52 +03:00
Code Activity

Fix use-after-free in parallel_vacuum_reset_dead_items

Browse Source 
parallel_vacuum_reset_dead_items used a local variable to hold a
pointer from the passed vacrel, purely as a shorthand. This pointer
was later freed and a new allocation was made and stored to the
struct. Then the local pointer was mistakenly referenced again.

This apparently happened not to break anything since the freed chunk
would have been put on the context's freelist, so it was accidentally
the same pointer anyway, in which case the DSA handle was correctly
updated. The minimal fix is to change two places so they access
dead_items through the vacrel. This coding style is a maintenance
hazard, so while at it get rid of most other similar usages, which
were inconsistently used anyway.

Analysis and patch by Vallimaharajan G, with further defensive coding
by me

Backpath to v17, when TidStore came in

Discussion: https://postgr.es/m/1936493cc38.68cb2ef27266.7456585136086197135@zohocorp.com
This commit is contained in:
John Naylor
2024-12-04 16:51:55 +07:00
parent 9abdc1841e
commit 83ce20d671
2 changed files with 9 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
src/backend/access/heap/vacuumlazy.c
View File

@ -820,8 +820,6 @@ lazy_scan_heap(LVRelState *vacrel)
next_fsm_block_to_vacuum = 0; next_fsm_block_to_vacuum = 0;
bool all_visible_according_to_vm; bool all_visible_according_to_vm;
TidStore *dead_items = vacrel->dead_items;
VacDeadItemsInfo *dead_items_info = vacrel->dead_items_info;
Buffer vmbuffer = InvalidBuffer; Buffer vmbuffer = InvalidBuffer;
const int initprog_index[] = { const int initprog_index[] = {
PROGRESS_VACUUM_PHASE, PROGRESS_VACUUM_PHASE,
@ -833,7 +831,7 @@ lazy_scan_heap(LVRelState *vacrel)
/* Report that we're scanning the heap, advertising total # of blocks */ /* Report that we're scanning the heap, advertising total # of blocks */
initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP; initprog_val[0] = PROGRESS_VACUUM_PHASE_SCAN_HEAP;
initprog_val[1] = rel_pages; initprog_val[1] = rel_pages;
initprog_val[2] = dead_items_info->max_bytes; initprog_val[2] = vacrel->dead_items_info->max_bytes;
pgstat_progress_update_multi_param(3, initprog_index, initprog_val); pgstat_progress_update_multi_param(3, initprog_index, initprog_val);
/* Initialize for the first heap_vac_scan_next_block() call */ /* Initialize for the first heap_vac_scan_next_block() call */
@ -876,7 +874,7 @@ lazy_scan_heap(LVRelState *vacrel)
* dead_items TIDs, pause and do a cycle of vacuuming before we tackle * dead_items TIDs, pause and do a cycle of vacuuming before we tackle
* this page. * this page.
*/ */
if (TidStoreMemoryUsage(dead_items) > dead_items_info->max_bytes) if (TidStoreMemoryUsage(vacrel->dead_items) > vacrel->dead_items_info->max_bytes)
{ {
/* /*
* Before beginning index vacuuming, we release any pin we may * Before beginning index vacuuming, we release any pin we may
@ -1046,7 +1044,7 @@ lazy_scan_heap(LVRelState *vacrel)
* Do index vacuuming (call each index's ambulkdelete routine), then do * Do index vacuuming (call each index's ambulkdelete routine), then do
* related heap vacuuming * related heap vacuuming
*/ */
if (dead_items_info->num_items > 0) if (vacrel->dead_items_info->num_items > 0)
lazy_vacuum(vacrel); lazy_vacuum(vacrel);
/* /*
@ -2882,19 +2880,18 @@ static void
dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *offsets, dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *offsets,
int num_offsets) int num_offsets)
{ {
TidStore *dead_items = vacrel->dead_items;
const int prog_index[2] = { const int prog_index[2] = {
PROGRESS_VACUUM_NUM_DEAD_ITEM_IDS, PROGRESS_VACUUM_NUM_DEAD_ITEM_IDS,
PROGRESS_VACUUM_DEAD_TUPLE_BYTES PROGRESS_VACUUM_DEAD_TUPLE_BYTES
}; };
int64 prog_val[2]; int64 prog_val[2];
TidStoreSetBlockOffsets(dead_items, blkno, offsets, num_offsets); TidStoreSetBlockOffsets(vacrel->dead_items, blkno, offsets, num_offsets);
vacrel->dead_items_info->num_items += num_offsets; vacrel->dead_items_info->num_items += num_offsets;
/* update the progress information */ /* update the progress information */
prog_val[0] = vacrel->dead_items_info->num_items; prog_val[0] = vacrel->dead_items_info->num_items;
prog_val[1] = TidStoreMemoryUsage(dead_items); prog_val[1] = TidStoreMemoryUsage(vacrel->dead_items);
pgstat_progress_update_multi_param(2, prog_index, prog_val); pgstat_progress_update_multi_param(2, prog_index, prog_val);
} }
@ -2904,8 +2901,6 @@ dead_items_add(LVRelState *vacrel, BlockNumber blkno, OffsetNumber *offsets,
static void static void
dead_items_reset(LVRelState *vacrel) dead_items_reset(LVRelState *vacrel)
{ {
TidStore *dead_items = vacrel->dead_items;
if (ParallelVacuumIsActive(vacrel)) if (ParallelVacuumIsActive(vacrel))
{ {
parallel_vacuum_reset_dead_items(vacrel->pvs); parallel_vacuum_reset_dead_items(vacrel->pvs);
@ -2913,7 +2908,7 @@ dead_items_reset(LVRelState *vacrel)
} }
/* Recreate the tidstore with the same max_bytes limitation */ /* 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); vacrel->dead_items = TidStoreCreateLocal(vacrel->dead_items_info->max_bytes, true);
/* Reset the counter */ /* Reset the counter */

7
src/backend/commands/vacuumparallel.c
View File

@ -472,7 +472,6 @@ parallel_vacuum_get_dead_items(ParallelVacuumState *pvs, VacDeadItemsInfo **dead
void void
parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs) parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
{ {
TidStore *dead_items = pvs->dead_items;
VacDeadItemsInfo *dead_items_info = &(pvs->shared->dead_items_info); VacDeadItemsInfo *dead_items_info = &(pvs->shared->dead_items_info);
/* /*
@ -480,13 +479,13 @@ parallel_vacuum_reset_dead_items(ParallelVacuumState *pvs)
* operating system. Then we recreate the tidstore with the same max_bytes * operating system. Then we recreate the tidstore with the same max_bytes
* limitation we just used. * limitation we just used.
*/ */
TidStoreDestroy(dead_items); TidStoreDestroy(pvs->dead_items);
pvs->dead_items = TidStoreCreateShared(dead_items_info->max_bytes, pvs->dead_items = TidStoreCreateShared(dead_items_info->max_bytes,
LWTRANCHE_PARALLEL_VACUUM_DSA); LWTRANCHE_PARALLEL_VACUUM_DSA);
/* Update the DSA pointer for dead_items to the new one */ /* 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_dsa_handle = dsa_get_handle(TidStoreGetDSA(pvs->dead_items));
pvs->shared->dead_items_handle = TidStoreGetHandle(dead_items); pvs->shared->dead_items_handle = TidStoreGetHandle(pvs->dead_items);
/* Reset the counter */ /* Reset the counter */
dead_items_info->num_items = 0; dead_items_info->num_items = 0;