Horton

Thread

Commits

  1. sequences: Lock buffer before initializing page

  1. fill_seq_fork_with_data() initializes buffer without lock

    Andres Freund <andres@anarazel.de> — 2023-04-04T18:55:01Z

    Hi,

Look at:

static void
fill_seq_fork_with_data(Relation rel, HeapTuple tuple, ForkNumber forkNum)
{
	Buffer		buf;
	Page		page;
	sequence_magic *sm;
	OffsetNumber offnum;

	/* Initialize first page of relation with special magic number */

	buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_NORMAL, NULL);
	Assert(BufferGetBlockNumber(buf) == 0);

	page = BufferGetPage(buf);

	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
	sm = (sequence_magic *) PageGetSpecialPointer(page);
	sm->magic = SEQ_MAGIC;

	/* Now insert sequence tuple */

	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);


Clearly we are modifying the page (via PageInit()), without holding a buffer
lock, which is only acquired subsequently.

It's clearly unlikely to cause bad consequences - the sequence doesn't yet
really exist, and we haven't seen any reports of a problem - but it doesn't
seem quite impossible that it would cause problems.

As far as I can tell, this goes back to the initial addition of the sequence
code, in e8647c45d66a - I'm too lazy to figure out whether it possibly wasn't
a problem in 1997 for some reason.

Greetings,

Andres Freund

  2. Re: fill_seq_fork_with_data() initializes buffer without lock

    Andres Freund <andres@anarazel.de> — 2023-04-04T23:23:38Z

    Hi,

On 2023-04-04 11:55:01 -0700, Andres Freund wrote:
> Look at:
>
> static void
> fill_seq_fork_with_data(Relation rel, HeapTuple tuple, ForkNumber forkNum)
> {
> 	Buffer		buf;
> 	Page		page;
> 	sequence_magic *sm;
> 	OffsetNumber offnum;
>
> 	/* Initialize first page of relation with special magic number */
>
> 	buf = ReadBufferExtended(rel, forkNum, P_NEW, RBM_NORMAL, NULL);
> 	Assert(BufferGetBlockNumber(buf) == 0);
>
> 	page = BufferGetPage(buf);
>
> 	PageInit(page, BufferGetPageSize(buf), sizeof(sequence_magic));
> 	sm = (sequence_magic *) PageGetSpecialPointer(page);
> 	sm->magic = SEQ_MAGIC;
>
> 	/* Now insert sequence tuple */
>
> 	LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
>
>
> Clearly we are modifying the page (via PageInit()), without holding a buffer
> lock, which is only acquired subsequently.
>
> It's clearly unlikely to cause bad consequences - the sequence doesn't yet
> really exist, and we haven't seen any reports of a problem - but it doesn't
> seem quite impossible that it would cause problems.
>
> As far as I can tell, this goes back to the initial addition of the sequence
> code, in e8647c45d66a - I'm too lazy to figure out whether it possibly wasn't
> a problem in 1997 for some reason.

Robert suggested to add an assertion to PageInit() to defend against such
omissions. I quickly hacked one together. The assertion immediately found the
issue here, but no other currently existing ones.

I'm planning to push a fix for this to HEAD. Given that the risk seems low and
the issue is so longstanding, it doesn't seem quite worth backpatching?


FWIW, the assertion I used is:

    if (page >= BufferBlocks && page <= BufferBlocks + BLCKSZ * NBuffers)
    {
        Buffer buffer = (page - BufferBlocks) / BLCKSZ + 1;
        BufferDesc *buf = GetBufferDescriptor(buffer - 1);

        Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(buf), LW_EXCLUSIVE));
    }

If there's interest in having such an assertion permenantly, it clearly can't
live in bufpage.c.

I have a bit of a hard time coming up with a good name. Any suggestions?

Greetings,

Andres Freund