heap_lock_updated_tuple_rec can leak a buffer refcount

It seems to me that heap_lock_updated_tuple_rec can lead to a buffer
refcount leak while locking an updated tuple by an aborted
transaction. In commit - 5c609a74, we have added the code to deal
with aborted transactions as below:

heap_lock_updated_tuple_rec()
{
..

if (PageIsAllVisible(BufferGetPage(buf)))
visibilitymap_pin(rel, block, &vmbuffer);
else
vmbuffer = InvalidBuffer;

LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
..
-------------------------- below code is added by commit -5c609a74 -----------
if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(mytup.t_data)))
{
UnlockReleaseBuffer(buf);
return HeapTupleMayBeUpdated;
}
-------------------------------------------------------------

I think the above code forgets to deal with vmbuffer and can lead to a
leak of the same. Attached patch ensures that it deals with vmbuffer
when required.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Feb 13, 2018 at 10:11 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> It seems to me that heap_lock_updated_tuple_rec can lead to a buffer
> refcount leak while locking an updated tuple by an aborted
> transaction. In commit - 5c609a74, we have added the code to deal
> with aborted transactions as below:
>
> heap_lock_updated_tuple_rec()
> {
> ..
>
> if (PageIsAllVisible(BufferGetPage(buf)))
> visibilitymap_pin(rel, block, &vmbuffer);
> else
> vmbuffer = InvalidBuffer;
>
> LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE);
> ..
> -------------------------- below code is added by commit -5c609a74 -----------
> if (TransactionIdDidAbort(HeapTupleHeaderGetXmin(mytup.t_data)))
> {
> UnlockReleaseBuffer(buf);
> return HeapTupleMayBeUpdated;
> }
> -------------------------------------------------------------
>
> I think the above code forgets to deal with vmbuffer and can lead to a
> leak of the same. Attached patch ensures that it deals with vmbuffer
> when required.
>

Registered the patch for next CF:
https://commitfest.postgresql.org/17/1531/

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Looks like a leak indeed, the fix seems right.

Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> writes:
> Looks like a leak indeed, the fix seems right.

Yup, it's a leak. It's hard to hit because you need to be starting
with an update of a tuple in an all-visible page; otherwise we never
pin the vm page so there's nothing to leak. But if you lobotomize
the test a few lines above so that it always pins the vm page, then
the regression tests (specifically combocid) reveal the leak, and
show that the proposed patch indeed fixes it.

However ... with said lobotomization, the isolation tests trigger an
Assert(BufferIsPinned(buffer)) inside visibilitymap_pin, showing that
there's another bug here too. That seems to be because at the bottom
of the outer loop, we do

if (vmbuffer != InvalidBuffer)
ReleaseBuffer(vmbuffer);

and then loop back around with vmbuffer still not equal to InvalidBuffer.
This causes the next loop iteration's visibilitymap_pin call to think it
needs to release that vmbuffer pin a second time; kaboom.

And eyeing this, I see still a third problem: if either of the "goto l4"
jumps occur, we'll loop back to l4 with vmbuffer possibly pinned, and then
if the new page isn't all-visible, we'll just set vmbuffer = InvalidBuffer
and leak the pin that way. (If it is all-visible, we unpin the old page
correctly in the visibilitymap_pin call, but that can charitably be
described as accidental.)

In short, this is pretty darn broken. We need to treat the vmbuffer
variable honestly as state that may persist across either the outer loop
or the "goto l4" sub-loop. Furthermore, it's not really cool to use
"vmbuffer == InvalidBuffer" as the indicator of whether we acquired the
vmbuffer pin pre-lock. To do that, we'd be forced to drop the old pin in
the not-all-visible path, even though we might need it right back again.
Also, remembering that one vm page serves many heap pages, even if we have
a vm pin for the "wrong" page it's conceivable that it'd be the right one
for the next time we actually need it. So we should use the
visibilitymap_pin API the way it's meant to be used, and hold any vm pin
we've acquired until the very end of the function.

Hence, I propose the attached patch. The test lobotomization
(the "if (1) //" change) isn't meant for commit but shows how I tested
the take-the-pin paths. This passes make check-world with or without
the lobotomization change.

regards, tom lane

On Sat, Mar 3, 2018 at 3:26 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alexander Kuzmenkov <a(dot)kuzmenkov(at)postgrespro(dot)ru> writes:
>> Looks like a leak indeed, the fix seems right.
>
>
> Hence, I propose the attached patch. The test lobotomization
> (the "if (1) //" change) isn't meant for commit but shows how I tested
> the take-the-pin paths. This passes make check-world with or without
> the lobotomization change.
>

Thanks for taking care of this.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com