Horton

Thread

Commits

  1. Fix unsafe access to BufferDescriptors

  1. Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Tender Wang <tndrwang@gmail.com> — 2024-11-07T11:07:35Z

    Hi,

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?


-- 
Thanks,
Tender Wang

  2. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Xuneng Zhou <xunengzhou@gmail.com> — 2025-01-08T05:34:48Z

    Hi Tender,

I’ve looked through the patch, and I believe there is a potential issue. The default size for BufferDescriptors appears to be 16,384. Passing and casting a negative buffer ID to a large unsigned integer in GetBufferDescriptor, and then using it as an array subscript, could potentially lead to an overflow.

void
BufferManagerShmemInit(void)
{
	bool		foundBufs,
				foundDescs,
				foundIOCV,
				foundBufCkpt;

	/* Align descriptors to a cacheline boundary. */
	BufferDescriptors = (BufferDescPadded *)
		ShmemInitStruct("Buffer Descriptors",
						NBuffers * sizeof(BufferDescPadded),
						&foundDescs);

int			NBuffers = 16384;

The changes proposed in the patch seem reasonable to me, but it might be helpful to include an explanation of the error case and how it’s handled.

Best regards,
[Xuneng]

The new status of this patch is: Waiting on Author

  3. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Tender Wang <tndrwang@gmail.com> — 2025-02-03T14:28:59Z

    Xuneng Zhou <xunengzhou@gmail.com> 于2025年1月8日周三 13:35写道：

> Hi Tender,
>
> I’ve looked through the patch, and I believe there is a potential issue.
> The default size for BufferDescriptors appears to be 16,384. Passing and
> casting a negative buffer ID to a large unsigned integer in
> GetBufferDescriptor, and then using it as an array subscript, could
> potentially lead to an overflow.
>
> void
> BufferManagerShmemInit(void)
> {
>         bool            foundBufs,
>                                 foundDescs,
>                                 foundIOCV,
>                                 foundBufCkpt;
>
>         /* Align descriptors to a cacheline boundary. */
>         BufferDescriptors = (BufferDescPadded *)
>                 ShmemInitStruct("Buffer Descriptors",
>                                                 NBuffers *
> sizeof(BufferDescPadded),
>                                                 &foundDescs);
>
> int                     NBuffers = 16384;
>
> The changes proposed in the patch seem reasonable to me, but it might be
> helpful to include an explanation of the error case and how it’s handled.
>

Thanks for reviewing.
The  BufferGetLSNAtomic() with this patch looks not complex. I think no
need more explanation here.


> Best regards,
> [Xuneng]
>
> The new status of this patch is: Waiting on Author
>

I change the status to Ready for commiter
-- 
Thanks,
Tender Wang

  4. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Richard Guo <guofenglinux@gmail.com> — 2025-02-04T07:59:43Z

    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

  5. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Richard Guo <guofenglinux@gmail.com> — 2025-02-19T00:36:35Z

    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

  6. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Michael Paquier <michael@paquier.xyz> — 2025-02-19T00:44:34Z

    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.
--
Michael

  7. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Richard Guo <guofenglinux@gmail.com> — 2025-02-19T00:58:02Z

    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

  8. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Richard Guo <guofenglinux@gmail.com> — 2025-02-19T02:30:24Z

    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

  9. Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

    Tender Wang <tndrwang@gmail.com> — 2025-02-19T02:47:31Z

    Richard Guo <guofenglinux@gmail.com> 于2025年2月19日周三 10:30写道：

> 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 for pushing.

-- 
Thanks,
Tender Wang