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 b251e6970344f432682375351769a0e3a85448c5..239a10f5509730178c4c92bd7a80d7a468b6887f 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 65c4e74999f101a4ae978df0fed5a70fd02288ed..8d0ee7fc937d51edfcfb9d68b6a566c863c1bee3 100644 (file)
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -580,10 +580,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;
 }