From 37a8eee547cfaeee83c68f771a7faf05f83e4422 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sun, 9 Mar 2025 20:11:51 -0400 Subject: [PATCH v2.6 16/34] localbuf: Fix dangerous coding pattern in GetLocalVictimBuffer() If PinLocalBuffer() were to modify the buf_state, the buf_state in GetLocalVictimBuffer() would be out of date. Currently that does not happen, as PinLocalBuffer() only modifies the buf_state if adjust_usagecount=true and GetLocalVictimBuffer() passes false. However, it's easy to make this not the case anymore - it cost me a few hours to debug the consequences. The minimal fix would be to just refetch the buf_state after after calling PinLocalBuffer(), but the same danger exists in later parts of the function. So instead refetch declare buf_state in narrower scopes and always refetch. I apparently broke this in 794f2594479. Author: Reviewed-by: Discussion: https://postgr.es/m/ Backpatch: --- src/backend/storage/buffer/localbuf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/backend/storage/buffer/localbuf.c b/src/backend/storage/buffer/localbuf.c index 5378ba84316..d3c869f53f9 100644 --- a/src/backend/storage/buffer/localbuf.c +++ b/src/backend/storage/buffer/localbuf.c @@ -178,7 +178,6 @@ GetLocalVictimBuffer(void) { int victim_bufid; int trycounter; - uint32 buf_state; BufferDesc *bufHdr; ResourceOwnerEnlarge(CurrentResourceOwner); @@ -199,7 +198,7 @@ GetLocalVictimBuffer(void) if (LocalRefCount[victim_bufid] == 0) { - buf_state = pg_atomic_read_u32(&bufHdr->state); + uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); if (BUF_STATE_GET_USAGECOUNT(buf_state) > 0) { @@ -233,8 +232,9 @@ GetLocalVictimBuffer(void) * this buffer is not referenced but it might still be dirty. if that's * the case, write it out before reusing it! */ - if (buf_state & BM_DIRTY) + if (pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY) { + uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); instr_time io_start; SMgrRelation oreln; Page localpage = (char *) LocalBufHdrGetBlock(bufHdr); @@ -267,8 +267,9 @@ GetLocalVictimBuffer(void) /* * Remove the victim buffer from the hashtable and mark as invalid. */ - if (buf_state & BM_TAG_VALID) + if (pg_atomic_read_u32(&bufHdr->state) & BM_TAG_VALID) { + uint32 buf_state = pg_atomic_read_u32(&bufHdr->state); LocalBufferLookupEnt *hresult; hresult = (LocalBufferLookupEnt *) -- 2.48.1.76.g4e746b1a31.dirty