public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Michael Paquier <[email protected]>
Cc: Ashutosh Bapat <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()
Date: Wed, 10 Jun 2026 10:36:22 -0400
Message-ID: <hfz3gif4frhxacjsqjjxhcxs4uacgbtrvjwyjzzhejvtj75v65@wq6xllusmmqn> (raw)
In-Reply-To: <[email protected]>
References: <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com>
<[email protected]>
On 2026-06-10 12:40:38 +0900, Michael Paquier wrote:
> On Sat, Jun 06, 2026 at 01:37:42PM +0530, Ashutosh Bapat wrote:
> > 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.
>
> @@ -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);
>
> Yep, that's clearly wrong. We are lucky that it does not blow up
> today but that's a ticking bomb.
I think it *should* blow up. It doesn't because we're lacking assertions in
GetBufferDescriptor(). But I don't think the assertions added in the patch are
quite right.
We can't trivially add the correct assertions, because somebody though it was
a good idea to give GetBufferDescriptor() a uint32 parameter, which seems
completely wrong to me.
> Even with that in mind, the result leads to a non-defined behavior.
I'm not sure it really does, but it's clearly wrong.
Greetings,
Andres Freund
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], [email protected]
Subject: Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()
In-Reply-To: <hfz3gif4frhxacjsqjjxhcxs4uacgbtrvjwyjzzhejvtj75v65@wq6xllusmmqn>
* 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