Unsafe access BufferDescriptors array in BufferGetLSNAtomic()
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
Attachments:
0001-Fix-unsafe-access-BufferDescriptors.patchapplication/octet-stream; name=0001-Fix-unsafe-access-BufferDescriptors.patchDownload
From a26245086f080fa31d5d8076ded7ca2db6a4f4b0 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Thu, 7 Nov 2024 18:47:36 +0800
Subject: [PATCH] Fix unsafe access BufferDescriptors.
---
src/backend/storage/buffer/bufmgr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 0f02bf62fa..2c5d7ff5de 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -3984,10 +3984,10 @@ BufferIsPermanent(Buffer buffer)
XLogRecPtr
BufferGetLSNAtomic(Buffer buffer)
{
- BufferDesc *bufHdr = GetBufferDescriptor(buffer - 1);
char *page = BufferGetPage(buffer);
XLogRecPtr lsn;
uint32 buf_state;
+ BufferDesc *bufHdr;
/*
* If we don't need locking for correctness, fastpath out.
@@ -3999,6 +3999,7 @@ BufferGetLSNAtomic(Buffer buffer)
Assert(BufferIsValid(buffer));
Assert(BufferIsPinned(buffer));
+ bufHdr = GetBufferDescriptor(buffer - 1);
buf_state = LockBufHdr(bufHdr);
lsn = PageGetLSN(page);
UnlockBufHdr(bufHdr, buf_state);
--
2.25.1
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
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
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 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
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
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