Add missing buffer lock acquisition in GetTupleForTrigger().

tglsfdc · tglsfdc · commit 1c316e3a02e7 · 2012-11-30T13:56:00.000-05:00
If we had not been holding buffer pin continuously since the tuple was
initially fetched by the UPDATE or DELETE query, it would be possible for
VACUUM or a page-prune operation to move the tuple while we're trying to
copy it.  This would result in a garbage "old" tuple value being passed to
an AFTER ROW UPDATE or AFTER ROW DELETE trigger.  The preconditions for
this are somewhat improbable, and the timing constraints are very tight;
so it's not so surprising that this hasn't been reported from the field,
even though the bug has been there a long time.

Problem found by Andres Freund.  Back-patch to all active branches.
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
@@ -2663,6 +2663,16 @@ ltrmark:;
 
 		buffer = ReadBuffer(relation, ItemPointerGetBlockNumber(tid));
 
+		/*
+		 * Although we already know this tuple is valid, we must lock the
+		 * buffer to ensure that no one has a buffer cleanup lock; otherwise
+		 * they might move the tuple while we try to copy it.  But we can
+		 * release the lock before actually doing the heap_copytuple call,
+		 * since holding pin is sufficient to prevent anyone from getting a
+		 * cleanup lock they don't already hold.
+		 */
+		LockBuffer(buffer, BUFFER_LOCK_SHARE);
+
 		page = BufferGetPage(buffer);
 		lp = PageGetItemId(page, ItemPointerGetOffsetNumber(tid));
 
@@ -2672,6 +2682,8 @@ ltrmark:;
 		tuple.t_len = ItemIdGetLength(lp);
 		tuple.t_self = *tid;
 		tuple.t_tableOid = RelationGetRelid(relation);
+
+		LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
 	}
 
 	result = heap_copytuple(&tuple);
