Fix inadequate buffer locking in FSM and VM page re-initialization.

When reading an existing FSM or VM page that was found to be corrupt by the
buffer manager, the code applied PageInit() to reinitialize the page, but
did so without any locking.  There is thus a hazard that two backends might
concurrently do PageInit, which in itself would still be OK, but the slower
one might then zero over subsequent data changes applied by the faster one.
Even that is unlikely to be fatal; but it's not desirable, so add locking
to prevent it.

This does not add any locking overhead in the normal code path where the
page is OK.  It's not immediately obvious that that's safe, but I believe
it is, for reasons explained in the added comments.

Problem noted by R P Asim.  It's been like this for a long time, so
back-patch to all supported branches.

Discussion: https://postgr.es/m/CANXE4Te4G0TGq6cr0-TvwP0H4BNiK_-hB5gHe8mF+nz0mcYfMQ@mail.gmail.com

diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index f020737f800f8f07b2c7663d624b165581bec32d..cd5c6d0e9afc9da5d2cbab78681ef114496c862e 100644 (file)
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -610,11 +610,30 @@ vm_readbuf(Relation rel, BlockNumber blkno, bool extend)
         * Use ZERO_ON_ERROR mode, and initialize the page if necessary. It's
         * always safe to clear bits, so it's better to clear corrupt pages than
         * error out.
+        *
+        * The initialize-the-page part is trickier than it looks, because of the
+        * possibility of multiple backends doing this concurrently, and our
+        * desire to not uselessly take the buffer lock in the normal path where
+        * the page is OK.  We must take the lock to initialize the page, so
+        * recheck page newness after we have the lock, in case someone else
+        * already did it.  Also, because we initially check PageIsNew with no
+        * lock, it's possible to fall through and return the buffer while someone
+        * else is still initializing the page (i.e., we might see pd_upper as set
+        * but other page header fields are still zeroes).  This is harmless for
+        * callers that will take a buffer lock themselves, but some callers
+        * inspect the page without any lock at all.  The latter is OK only so
+        * long as it doesn't depend on the page header having correct contents.
+        * Current usage is safe because PageGetContents() does not require that.
         */
        buf = ReadBufferExtended(rel, VISIBILITYMAP_FORKNUM, blkno,
                                                         RBM_ZERO_ON_ERROR, NULL);
        if (PageIsNew(BufferGetPage(buf)))
-               PageInit(BufferGetPage(buf), BLCKSZ, 0);
+       {
+               LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+               if (PageIsNew(BufferGetPage(buf)))
+                       PageInit(BufferGetPage(buf), BLCKSZ, 0);
+               LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+       }
        return buf;
 }
 

diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 4138b04839a66da21e830b820bf18418574f7f4d..9c7f325ab3ed3717a377f9d80742babba158d440 100644 (file)
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -593,10 +593,29 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
         * pages than error out. Since the FSM changes are not WAL-logged, the
         * so-called torn page problem on crash can lead to pages with corrupt
         * headers, for example.
+        *
+        * The initialize-the-page part is trickier than it looks, because of the
+        * possibility of multiple backends doing this concurrently, and our
+        * desire to not uselessly take the buffer lock in the normal path where
+        * the page is OK.  We must take the lock to initialize the page, so
+        * recheck page newness after we have the lock, in case someone else
+        * already did it.  Also, because we initially check PageIsNew with no
+        * lock, it's possible to fall through and return the buffer while someone
+        * else is still initializing the page (i.e., we might see pd_upper as set
+        * but other page header fields are still zeroes).  This is harmless for
+        * callers that will take a buffer lock themselves, but some callers
+        * inspect the page without any lock at all.  The latter is OK only so
+        * long as it doesn't depend on the page header having correct contents.
+        * Current usage is safe because PageGetContents() does not require that.
         */
        buf = ReadBufferExtended(rel, FSM_FORKNUM, blkno, RBM_ZERO_ON_ERROR, NULL);
        if (PageIsNew(BufferGetPage(buf)))
-               PageInit(BufferGetPage(buf), BLCKSZ, 0);
+       {
+               LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
+               if (PageIsNew(BufferGetPage(buf)))
+                       PageInit(BufferGetPage(buf), BLCKSZ, 0);
+               LockBuffer(buf, BUFFER_LOCK_UNLOCK);
+       }
        return buf;
 }