Feedback-ID: id4a34324:Fastmail
Date: Wed, 10 Jun 2026 10:36:22 -0400
From: Andres Freund <andres@anarazel.de>
To: Michael Paquier <michael@paquier.xyz>
Cc: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: GetBufferDescriptor() being called for local buffers from
 MarkBufferDirtyHint()
Message-ID: <hfz3gif4frhxacjsqjjxhcxs4uacgbtrvjwyjzzhejvtj75v65@wq6xllusmmqn>
References: 
 <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com>
 <aijctocPL6fUibrH@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <aijctocPL6fUibrH@paquier.xyz>
Archived-At: 
 <https://www.postgresql.org/message-id/hfz3gif4frhxacjsqjjxhcxs4uacgbtrvjwyjzzhejvtj75v65%40wq6xllusmmqn>
Precedence: bulk

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