Skip to content

Commit 7b7ed04

Browse files
tglsfdctglsfdc
committed
Prevent access to no-longer-pinned buffer in heapam_tuple_lock().
heap_fetch() used to have a "keep_buf" parameter that told it to return ownership of the buffer pin to the caller after finding that the requested tuple TID exists but is invisible to the specified snapshot. This was thoughtlessly removed in commit 5db6df0, which broke heapam_tuple_lock() (formerly EvalPlanQualFetch) because that function needs to do more accesses to the tuple even if it's invisible. The net effect is that we would continue to touch the page for a microsecond or two after releasing pin on the buffer. Usually no harm would result; but if a different session decided to defragment the page concurrently, we could see garbage data and mistakenly conclude that there's no newer tuple version to chain up to. (It's hard to say whether this has happened in the field. The bug was actually found thanks to a later change that allowed valgrind to detect accesses to non-pinned buffers.) The most reasonable way to fix this is to reintroduce keep_buf, although I made it behave slightly differently: buffer ownership is passed back only if there is a valid tuple at the requested TID. In HEAD, we can just add the parameter back to heap_fetch(). To avoid an API break in the back branches, introduce an additional function heap_fetch_extended() in those branches. In HEAD there is an additional, less obvious API change: tuple->t_data will be set to NULL in all cases where buffer ownership is not returned, in particular when the tuple exists but fails the time qual (and !keep_buf). This is to defend against any other callers attempting to access non-pinned buffers. We concluded that making that change in back branches would be more likely to introduce problems than cure any. In passing, remove a comment about heap_fetch that was obsoleted by 9a8ee1d. Per bug #17462 from Daniil Anisimov. Back-patch to v12 where the bug was introduced. Discussion: https://postgr.es/m/17462-9c98a0f00df9bd36@postgresql.org
1 parent 24d2b26 commit 7b7ed04

File tree

3 files changed

+28
-20
lines changed

3 files changed

+28
-20
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1530,10 +1530,14 @@ heap_getnextslot_tidrange(TableScanDesc sscan, ScanDirection direction,
15301530
* must unpin the buffer when done with the tuple.
15311531
*
15321532
* If the tuple is not found (ie, item number references a deleted slot),
1533-
* then tuple->t_data is set to NULL and false is returned.
1533+
* then tuple->t_data is set to NULL, *userbuf is set to InvalidBuffer,
1534+
* and false is returned.
15341535
*
1535-
* If the tuple is found but fails the time qual check, then false is returned
1536-
* but tuple->t_data is left pointing to the tuple.
1536+
* If the tuple is found but fails the time qual check, then the behavior
1537+
* depends on the keep_buf parameter. If keep_buf is false, the results
1538+
* are the same as for the tuple-not-found case. If keep_buf is true,
1539+
* then tuple->t_data and *userbuf are returned as for the success case,
1540+
* and again the caller must unpin the buffer; but false is returned.
15371541
*
15381542
* heap_fetch does not follow HOT chains: only the exact TID requested will
15391543
* be fetched.
@@ -1551,7 +1555,8 @@ bool
15511555
heap_fetch(Relation relation,
15521556
Snapshot snapshot,
15531557
HeapTuple tuple,
1554-
Buffer *userbuf)
1558+
Buffer *userbuf,
1559+
bool keep_buf)
15551560
{
15561561
ItemPointer tid = &(tuple->t_self);
15571562
ItemId lp;
@@ -1634,9 +1639,15 @@ heap_fetch(Relation relation,
16341639
return true;
16351640
}
16361641

1637-
/* Tuple failed time qual */
1638-
ReleaseBuffer(buffer);
1639-
*userbuf = InvalidBuffer;
1642+
/* Tuple failed time qual, but maybe caller wants to see it anyway. */
1643+
if (keep_buf)
1644+
*userbuf = buffer;
1645+
else
1646+
{
1647+
ReleaseBuffer(buffer);
1648+
*userbuf = InvalidBuffer;
1649+
tuple->t_data = NULL;
1650+
}
16401651

16411652
return false;
16421653
}
@@ -1659,8 +1670,7 @@ heap_fetch(Relation relation,
16591670
* are vacuumable, false if not.
16601671
*
16611672
* Unlike heap_fetch, the caller must already have pin and (at least) share
1662-
* lock on the buffer; it is still pinned/locked at exit. Also unlike
1663-
* heap_fetch, we do not report any pgstats count; caller may do so if wanted.
1673+
* lock on the buffer; it is still pinned/locked at exit.
16641674
*/
16651675
bool
16661676
heap_hot_search_buffer(ItemPointer tid, Relation relation, Buffer buffer,
@@ -5379,7 +5389,7 @@ heap_lock_updated_tuple_rec(Relation rel, ItemPointer tid, TransactionId xid,
53795389
block = ItemPointerGetBlockNumber(&tupid);
53805390
ItemPointerCopy(&tupid, &(mytup.t_self));
53815391

5382-
if (!heap_fetch(rel, SnapshotAny, &mytup, &buf))
5392+
if (!heap_fetch(rel, SnapshotAny, &mytup, &buf, false))
53835393
{
53845394
/*
53855395
* if we fail to find the updated version of the tuple, it's

‎src/backend/access/heap/heapam_handler.c

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ heapam_fetch_row_version(Relation relation,
188188
Assert(TTS_IS_BUFFERTUPLE(slot));
189189

190190
bslot->base.tupdata.t_self = *tid;
191-
if (heap_fetch(relation, snapshot, &bslot->base.tupdata, &buffer))
191+
if (heap_fetch(relation, snapshot, &bslot->base.tupdata, &buffer, false))
192192
{
193193
/* store in slot, transferring existing pin */
194194
ExecStorePinnedBufferHeapTuple(&bslot->base.tupdata, slot, buffer);
@@ -401,7 +401,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
401401
errmsg("tuple to be locked was already moved to another partition due to concurrent update")));
402402

403403
tuple->t_self = *tid;
404-
if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer))
404+
if (heap_fetch(relation, &SnapshotDirty, tuple, &buffer, true))
405405
{
406406
/*
407407
* If xmin isn't what we're expecting, the slot must have
@@ -500,6 +500,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
500500
*/
501501
if (tuple->t_data == NULL)
502502
{
503+
Assert(!BufferIsValid(buffer));
503504
return TM_Deleted;
504505
}
505506

@@ -509,8 +510,7 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
509510
if (!TransactionIdEquals(HeapTupleHeaderGetXmin(tuple->t_data),
510511
priorXmax))
511512
{
512-
if (BufferIsValid(buffer))
513-
ReleaseBuffer(buffer);
513+
ReleaseBuffer(buffer);
514514
return TM_Deleted;
515515
}
516516

@@ -526,22 +526,20 @@ heapam_tuple_lock(Relation relation, ItemPointer tid, Snapshot snapshot,
526526
*
527527
* As above, it should be safe to examine xmax and t_ctid
528528
* without the buffer content lock, because they can't be
529-
* changing.
529+
* changing. We'd better hold a buffer pin though.
530530
*/
531531
if (ItemPointerEquals(&tuple->t_self, &tuple->t_data->t_ctid))
532532
{
533533
/* deleted, so forget about it */
534-
if (BufferIsValid(buffer))
535-
ReleaseBuffer(buffer);
534+
ReleaseBuffer(buffer);
536535
return TM_Deleted;
537536
}
538537

539538
/* updated, so look at the updated row */
540539
*tid = tuple->t_data->t_ctid;
541540
/* updated row should have xmin matching this xmax */
542541
priorXmax = HeapTupleHeaderGetUpdateXid(tuple->t_data);
543-
if (BufferIsValid(buffer))
544-
ReleaseBuffer(buffer);
542+
ReleaseBuffer(buffer);
545543
/* loop back to fetch next in chain */
546544
}
547545
}

‎src/include/access/heapam.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ extern bool heap_getnextslot_tidrange(TableScanDesc sscan,
133133
ScanDirection direction,
134134
TupleTableSlot *slot);
135135
extern bool heap_fetch(Relation relation, Snapshot snapshot,
136-
HeapTuple tuple, Buffer *userbuf);
136+
HeapTuple tuple, Buffer *userbuf, bool keep_buf);
137137
extern bool heap_hot_search_buffer(ItemPointer tid, Relation relation,
138138
Buffer buffer, Snapshot snapshot, HeapTuple heapTuple,
139139
bool *all_dead, bool first_call);

0 commit comments

Comments
 (0)