Feedback-ID: i0fe9450f:Fastmail
Date: Wed, 10 Jun 2026 12:40:38 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Cc: Andres Freund <andres@anarazel.de>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: GetBufferDescriptor() being called for local buffers from
 MarkBufferDirtyHint()
Message-ID: <aijctocPL6fUibrH@paquier.xyz>
References: 
 <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="hwjAYTziNE03xjjA"
Content-Disposition: inline
In-Reply-To: 
 <CAExHW5uzRMYVZsXXS3HXXT0fG_sNrpUhUqwP4NorhaCqH9JDhA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/aijctocPL6fUibrH%40paquier.xyz>
Precedence: bulk


--hwjAYTziNE03xjjA
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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;
=20
-    bufHdr =3D GetBufferDescriptor(buffer - 1);
-
     if (!BufferIsValid(buffer))
         elog(ERROR, "bad buffer ID: %d", buffer);
=20
@@ -5842,6 +5840,8 @@ MarkBufferDirtyHint(Buffer buffer, bool buffer_std)
         return;
     }
=20
+    bufHdr =3D GetBufferDescriptor(buffer - 1);

Yep, that's clearly wrong.  We are lucky that it does not blow up
today but that's a ticking bomb.  Even with that in mind, the result
leads to a non-defined behavior.

-    return &(BufferDescriptors[id]).bufferdesc;
+    BufferDesc *bdesc =3D &(BufferDescriptors[id]).bufferdesc;
+
+    Assert(bdesc->buf_id =3D=3D id);
+
+    return bdesc;=20
[...]
-        BufferDesc *buf =3D GetBufferDescriptor(i);
+        /* GetBufferDescriptor() expects buf_id to be set in the descripto=
r. */
+        BufferDesc *buf =3D &(BufferDescriptors[i]).bufferdesc;

I am not convinced by these two.  For the first one, the assertion is
a bound check in disguise, which could also use NBuffers as of an (id
< NBuffers).  With the inlining we care about the performance.  This
also forces the second change in ShmemInit(), which makes even less
sense once we think about the first assertion.

Will fix the first issue on HEAD, that's wrong.  Thanks for the
report.
--
Michael

--hwjAYTziNE03xjjA
Content-Type: application/pgp-signature; name=signature.asc

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmoo3LYACgkQnvQgOdby
QH0KWQ/+IpiLKR8fEjizkMF5sEbtpWNki2dIpH1DMqdAJOCGPnjEblsK0tK2VIVc
OoJMqw6hjLPeY0eUNX2gbq6d/KHE1UIdx0e86Alz/usG5/c9PAxBOWHgUehLecfp
j2qn+53ejA9eoBXJTEvfkfOCXfgrcl4NU8TVNmCN9qs0JxQPGporUD2XVreW7S7w
Msg72X+deigUL0G1dKPF1084rkqg7aJqbpq9CBuPYic3bY+Liru+yQj6rx8Rixdd
8TL/aqhm/ts0k+6iHe/8e72y/ruUBeq0YXqVjNJSmJ+5TCi5+jU49c+EsdDgqLgd
d1ydbYEWAF8h8W+jNudYGoEw4hPbsX9KoV2/IKD2nggBjkSsLy//v23VgYSNDfgw
hp8eOvG++1Ij35DLIVfIEmhIiAVLcBWLxyZAH9DiZNO1W++S5wiI4Rmoy1u/2gJJ
L7QKWxRVWhpTE6KLd4gSMY4FjUQHfKAEDDj3QPmbJl7NoldXjZ3tHKnves7FB6qj
XTaFN/iB59KLUMOkk/kDK4ruSKa67uyPStumBGfAoUYVC84KeP9FmhxvtyJnC+7E
jkhe5SZKE8NK6BP68lPdsAyZYbE4QMat2j7bGnDKoaZTwQ3BuziD1vf0Z0QVppS2
8cAgC0aJnwbHRI47ZYnnVQOjEXbek2GConjaUVAfIUXJqFm8qD0=
=wOYl
-----END PGP SIGNATURE-----

--hwjAYTziNE03xjjA--