Re: BUG #17462: Invalid memory access in heapam_tuple_lock

Andres Freund <andres@anarazel.de> writes:
> On 2022-04-11 15:59:30 -0400, Tom Lane wrote:
>> Yeah, that could work.  You want to draft a patch, or shall I?

> If you would, I'd appreciate it.

I abandoned that idea after noticing that heapam_tuple_lock needs
not only the tuple's xmin and ctid, but usually also the result of
HeapTupleHeaderGetUpdateXid, which is expensive (it can require a
multixact lookup).  I do not think it's a great idea to expect
HeapTupleSatisfiesDirty to compute that.  So the attached patch goes back
to my original idea of reverting the removal of the keep_buf argument.
This worked out pretty cleanly, with minimal code changes.  In HEAD,
we might want to avoid the extra heap_fetch_extended layer by
adding that argument back to heap_fetch, but having the extra layer
avoids an API break for the back branches.

As written here, even though there's no obvious API break, there is
a subtle change: heap_fetch will now return tuple->t_data = NULL after
a snapshot failure.  I'm of two minds whether to back-patch that part
(not doing so would require more changes in heapam_tuple_lock, though).
I'm concerned that some caller might be using that legitimately, say
by testing for pointer nullness without actually dereferencing it.
And even if they are unsafely dereferencing it, I'm not sure people
would thank us for converting a subtle low-probability failure into a
subtle higher-probability one.  Thoughts?

            regards, tom lane

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index ba11bcd99e..8ac6f1eb87 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -1530,10 +1530,8 @@ heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
  * must unpin the buffer when done with the tuple.
  *
  * If the tuple is not found (ie, item number references a deleted slot),
- * then tuple->t_data is set to NULL and false is returned.
- *
- * If the tuple is found but fails the time qual check, then false is returned
- * but tuple->t_data is left pointing to the tuple.
+ * or if it fails the time qual check, then tuple->t_data is set to NULL,
+ * *userbuf is set to InvalidBuffer, and false is returned.
  *
  * heap_fetch does not follow HOT chains: only the exact TID requested will
  * be fetched.
@@ -1552,6 +1550,25 @@ heap_fetch(Relation relation,
            Snapshot snapshot,
            HeapTuple tuple,
            Buffer *userbuf)
+{
+    return heap_fetch_extended(relation, snapshot, tuple, userbuf, false);
+}
+
+/*
+ *    heap_fetch_extended        - fetch tuple even if it fails snapshot test
+ *
+ * If keep_buf is true, then upon finding a tuple that is valid but fails
+ * the snapshot check, we return the tuple pointer in tuple->t_data and the
+ * buffer ID in *userbuf, keeping the buffer pin, just as if it had passed
+ * the snapshot.  (The function result is still "false" though.)
+ * If keep_buf is false then this behaves identically to heap_fetch().
+ */
+bool
+heap_fetch_extended(Relation relation,
+                    Snapshot snapshot,
+                    HeapTuple tuple,
+                    Buffer *userbuf,
+                    bool keep_buf)
 {
     ItemPointer tid = &(tuple->t_self);
     ItemId        lp;
@@ -1634,9 +1651,15 @@ heap_fetch(Relation relation,
         return true;
     }

-    /* Tuple failed time qual */
-    ReleaseBuffer(buffer);
-    *userbuf = InvalidBuffer;
+    /* Tuple failed time qual, but maybe caller wants to see it anyway. */
+    if (keep_buf)
+        *userbuf = buffer;
+    else
+    {
+        ReleaseBuffer(buffer);
+        *userbuf = InvalidBuffer;
+        tuple->t_data = NULL;
+    }

     return false;
 }
@@ -1659,8 +1682,7 @@ heap_fetch(Relation relation,
  * are vacuumable, false if not.
  *
  * Unlike heap_fetch, the caller must already have pin and (at least) share
- * lock on the buffer; it is still pinned/locked at exit.  Also unlike
- * heap_fetch, we do not report any pgstats count; caller may do so if wanted.
+ * lock on the buffer; it is still pinned/locked at exit.
  */
 bool
 heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 666b6205a7..cf925d6017 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -401,7 +401,8 @@ tuple_lock_retry:
                              errmsg("tuple to be locked was already moved to another partition due to concurrent
update")));

                 tuple->t_self = *tid;
-                if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer))
+                if (heap_fetch_extended(relation, &SnapshotDirty, tuple,
+                                        &buffer, true))
                 {
                     /*
                      * If xmin isn't what we're expecting, the slot must have
@@ -500,6 +501,7 @@ tuple_lock_retry:
                  */
                 if (tuple->t_data == NULL)
                 {
+                    Assert(!BufferIsValid(buffer));
                     return TM_Deleted;
                 }

@@ -509,8 +511,7 @@ tuple_lock_retry:
                 if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple->t_data),
                                          priorXmax))
                 {
-                    if (BufferIsValid(buffer))
-                        ReleaseBuffer(buffer);
+                    ReleaseBuffer(buffer);
                     return TM_Deleted;
                 }

@@ -526,13 +527,12 @@ tuple_lock_retry:
                  *
                  * As above, it should be safe to examine xmax and t_ctid
                  * without the buffer content lock, because they can't be
-                 * changing.
+                 * changing.  We'd better hold a buffer pin though.
                  */
                 if (ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid))
                 {
                     /* deleted, so forget about it */
-                    if (BufferIsValid(buffer))
-                        ReleaseBuffer(buffer);
+                    ReleaseBuffer(buffer);
                     return TM_Deleted;
                 }

@@ -540,8 +540,7 @@ tuple_lock_retry:
                 *tid = tuple->t_data->t_ctid;
                 /* updated row should have xmin matching this xmax */
                 priorXmax = HeapTupleHeaderGetUpdateXid(tuple->t_data);
-                if (BufferIsValid(buffer))
-                    ReleaseBuffer(buffer);
+                ReleaseBuffer(buffer);
                 /* loop back to fetch next in chain */
             }
         }
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 4403f01e13..ebfdc3ff7f 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -134,6 +134,9 @@ extern bool heap_getnextslot_tidrange(TableScanDesc sscan,
                                       TupleTableSlot *slot);
 extern bool heap_fetch(Relation relation, Snapshot snapshot,
                        HeapTuple tuple, Buffer *userbuf);
+extern bool heap_fetch_extended(Relation relation, Snapshot snapshot,
+                                HeapTuple tuple, Buffer *userbuf,
+                                bool keep_buf);
 extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
                                    Buffer buffer, Snapshot snapshot, HeapTuple heapTuple,
                                    bool *all_dead, bool first_call);