Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

On Tue, Feb 4, 2025 at 4:59 PM Richard Guo <guofenglinux@gmail.com> wrote:
> On Thu, Nov 7, 2024 at 7:07 PM Tender Wang <tndrwang@gmail.com> wrote:
> > While learning gist index insert codes, I find a little issue with BufferGetLSNAtomic().
> > At first, it wants to get bufHdr by accessing the buffer descriptor array, as below:
> >
> > BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1);
> >
> > However, it doesn't check whether the passed buffer is a local or shared buffer.
> > If the buffer is local, then buffer < 0; it will be cast to uint32 when
> > passed to GetBufferDescriptor().
> > This may be unsafe, although no someone reports the problem.
> >
> > I tweak a few codes; see the attached patch.
> > Any thoughts?
>
> LGTM.  When considering a local buffer, the GetBufferDescriptor() call
> in BufferGetLSNAtomic() would be retrieving a shared buffer with a bad
> buffer ID.  Since the code checks whether the buffer is shared before
> using the retrieved BufferDesc, this issue did not lead to any
> malfunction.  Nonetheless this seems like trouble waiting to happen.

I plan to push this shortly.  This is arguably a bug, but it hasn't
caused any real issues, and nobody has complained about it until now,
so I don't think it needs to be back-patched.  Any thoughts?

Thanks
Richard