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

Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

From

Xuneng Zhou

Date:

08 January, 08:34:48

Hi Tender,

I’ve looked through the patch, and I believe there is a potential issue. The default size for BufferDescriptors appears
tobe 16,384. Passing and casting a negative buffer ID to a large unsigned integer in GetBufferDescriptor, and then
usingit 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
caseand how it’s handled.
 

Best regards,
[Xuneng]

The new status of this patch is: Waiting on Author

Re: Unsafe access BufferDescriptors array in BufferGetLSNAtomic()

From

Tender Wang

Date:

03 February, 17:28:59

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