From a096e1ccb3e2e175053070e27f2c0b02061b8c0d Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Sun, 9 Mar 2025 20:11:51 -0400 Subject: [PATCH v2.8 16/38] 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. Instead, declare buf_state in the narrower scopes and re-read the state in conditional branches. Besides being safer, it also fits well with an upcoming set of cleanup patches that move the contents of the conditional branches in GetLocalVictimBuffer() into helper functions. I "broke" this in 794f2594479. Arguably this should be backpatched, but as the relevant functions are not exported and there is no actual misbehaviour, I chose to not backpatch, at least for now. Reviewed-by: Melanie Plageman Discussion: https://postgr.es/m/CAAKRu_b9anbWzEs5AAF9WCvcEVmgz-1AkHSQ-CLLy-p7WHzvFw@mail.gmail.com --- 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 919f600ed41..456a2232f22 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