public inbox for [email protected]
help / color / mirror / Atom feedFrom: Ashutosh Bapat <[email protected]>
To: Andres Freund <[email protected]>
To: PostgreSQL Hackers <[email protected]>
Subject: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()
Date: Sat, 6 Jun 2026 13:37:42 +0530
Message-ID: <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com> (raw)
Hi Andres,
82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint()
which invokes GetBufferDescriptor() even for local buffers for which
id < 0. Since GetBufferDescriptor() declares id as uint32, -1 is
converted to a very large int32 value which is way larger than
NBuffers. Thus GetBufferDescriptor() may be returning something from
the BufferBlocks which probably has enough memory to accommodate that
memory access. But it's a bogus BufferDesc nevertheless. We are not
seeing any problem with this right now since MarkBufferDirtyHint()
uses the BufferDesc only when it's a shared buffer. Right fix is to
let that function handle local buffers first and then call
GetBufferDescriptor() as in the attached patch.
I caught this because of an Assertion added in GetBufferDescription()
in my shared buffer resizing patches. I think it's worth committing
that assertion and the related change to BufferManagerShmemInit()
separately from shared buffer resizing patches. Included those changes
in the attached patch as well.
--
Best Wishes,
Ashutosh Bapat
Attachments:
[text/x-patch] v20260606-0001-MarkBufferDirtyHint-calls-GetBufferDescrip.patch (3.4K, 2-v20260606-0001-MarkBufferDirtyHint-calls-GetBufferDescrip.patch)
download | inline diff:
From f66dd8d526451780775aa2a91de7f1781adbbc73 Mon Sep 17 00:00:00 2001
From: Ashutosh Bapat <[email protected]>
Date: Sat, 6 Jun 2026 13:21:50 +0530
Subject: [PATCH v20260606 1/6] MarkBufferDirtyHint() calls
GetBufferDescriptor() for local buffers
82467f627bd478569de04f4a3f1993098e80c812 added MarkBufferDirtyHint() which
invokes GetBufferDescriptor() even for local buffers which have negative buffer
identifiers. Since GetBufferDescriptor() declares id as uint32, a negative
integer passed to it is converted to a 32 bit integer value which is way larger
than NBuffers. Thus GetBufferDescriptor() may be returning a pointer inside
BufferBlocks which probably has enough memory to accommodate that memory access.
But it's a bogus BufferDesc nevertheless. We are not seeing any problem with
this right now since MarkBufferDirtyHint() uses the returned BufferDesc only
when it is a shared buffer. Fix it by letting MarkBufferDirtyHint() handle local
buffers first and then calling GetBufferDescriptor().
In order to catch bogus BufferDescs returned by GetBufferDescriptor(), add an
assertion in GetBufferDescription() to check that the buffer identifier in the
BufferDesc is indeed the expeted one. Because of this assertion we can not rely
on using GetBufferDescriptor() in the initialization code. Hence change
BufferManagerShmemInit() to access BufferDescriptors directly.
Reported by: Ashutosh Bapat <[email protected]>
Author: Ashutosh Bapat <[email protected]>
---
src/backend/storage/buffer/buf_init.c | 3 ++-
src/backend/storage/buffer/bufmgr.c | 4 ++--
src/include/storage/buf_internals.h | 6 +++++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/buffer/buf_init.c b/src/backend/storage/buffer/buf_init.c
index 1407c930c56..5b5703e1012 100644
--- a/src/backend/storage/buffer/buf_init.c
+++ b/src/backend/storage/buffer/buf_init.c
@@ -124,7 +124,8 @@ BufferManagerShmemInit(void *arg)
*/
for (int i = 0; i < NBuffers; i++)
{
- BufferDesc *buf = GetBufferDescriptor(i);
+ /* GetBufferDescriptor() expects buf_id to be set in the descriptor. */
+ BufferDesc *buf = &(BufferDescriptors[i]).bufferdesc;
ClearBufferTag(&buf->tag);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index cc398db124d..d6c0cc1f6d4 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5831,8 +5831,6 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
{
BufferDesc *bufHdr;
- bufHdr = GetBufferDescriptor(buffer - 1);
-
if (!BufferIsValid(buffer))
elog(ERROR, "bad buffer ID: %d", buffer);
@@ -5842,6 +5840,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
return;
}
+ bufHdr = GetBufferDescriptor(buffer - 1);
+
MarkSharedBufferDirtyHint(buffer, bufHdr,
pg_atomic_read_u64(&bufHdr->state),
buffer_std);
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 89615a254a3..100b5272a7a 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -421,7 +421,11 @@ extern PGDLLIMPORT BufferDesc *LocalBufferDescriptors;
static inline BufferDesc *
GetBufferDescriptor(uint32 id)
{
- return &(BufferDescriptors[id]).bufferdesc;
+ BufferDesc *bdesc = &(BufferDescriptors[id]).bufferdesc;
+
+ Assert(bdesc->buf_id == id);
+
+ return bdesc;
}
static inline BufferDesc *
base-commit: 89eafad297a9b01ad77cfc1ab93a433e0af894b0
--
2.34.1
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()
In-Reply-To: <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox