Thread: Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

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.

Thanks
Richard

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

On Wed, Feb 19, 2025 at 9:44 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Feb 19, 2025 at 09:36:35AM +0900, Richard Guo wrote:
> > 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?
>
> I can see a point in doing a backpatch to keep the code consistent
> across branches, and this avoids incorrect access patterns when
> copy-pasting this code.

Hmm, fair point.  I will backpatch-through 13.

Thanks
Richard

On Wed, Feb 19, 2025 at 9:58 AM Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Feb 19, 2025 at 9:44 AM Michael Paquier <michael@paquier.xyz> wrote:
> > On Wed, Feb 19, 2025 at 09:36:35AM +0900, Richard Guo wrote:
> > > 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?
> >
> > I can see a point in doing a backpatch to keep the code consistent
> > across branches, and this avoids incorrect access patterns when
> > copy-pasting this code.
>
> Hmm, fair point.  I will backpatch-through 13.

Done.

Thanks
Richard