Re: GetBufferDescriptor() being called for local buffers from MarkBufferDirtyHint()

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