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 4c2a13aebaeaae723ebcba874a3e54306a48cad8..fc3ea13b5bd0eeaa961f58ced513ec11f96661c0 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 4648473523291390708e3238190632f116958dd7..3eee5b31aaa3c61b134b515dc7f4472f9e0f57da 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;
 }