Content-Type: multipart/mixed; boundary="------------6Zty0xF4NUBXe53M5TSNHx0E"
Message-ID: <494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com>
Date: Mon, 8 Jan 2024 11:00:00 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW
 UPDATE trigger
Content-Language: en-US
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, pgsql-bugs@lists.postgresql.org,
 Richard Guo <guofenglinux@gmail.com>, Andres Freund <andres@anarazel.de>,
 Peter Geoghegan <pg@bowt.ie>
References: <17798-0907404928dcf0dd@postgresql.org>
 <87fe6c28-80b2-f709-a8db-d105e5240e39@gmail.com>
 <CAMbWs4_pg7ab=OJF2CGLzs=noWEjujerxfo92sm3RL9ce=ogsQ@mail.gmail.com>
 <CAMbWs48FHhuuuWbxygO4Wey7ahdaSwRjzJODKXx7=GFsge_aLg@mail.gmail.com>
 <f5ff446f-72e2-44f7-80c6-4256cf7caeb3@gmail.com>
 <a4a8cf37-e9d5-3c58-fc2e-d217cf66f315@gmail.com>
 <1849610.1682807087@sss.pgh.pa.us>
 <e989408c-1838-61cd-c968-1fcf47c6fbba@gmail.com>
 <CA+TgmoZz1fGaTnOSf1yQhwYovEEkJZ9iioQXnLgaJz+1fBip=g@mail.gmail.com>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: 
 <CA+TgmoZz1fGaTnOSf1yQhwYovEEkJZ9iioQXnLgaJz+1fBip=g@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/494cd7c4-c109-6886-c0e3-138a95e7e38b%40gmail.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------6Zty0xF4NUBXe53M5TSNHx0E
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hello Robert,

06.01.2024 00:18, Robert Haas wrote:
> On Sun, Jul 9, 2023 at 11:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>> I've found enough time this week to investigate the issue deeper and now
>> I can answer your questions, hopefully.
> This patch lacks a commit message, which I think would be a good thing
> to add. Even if somebody doesn't use the message you write, it would
> help with understanding the patch itself. Deleting the call to
> ExecMaterializeSlot with no explanation in the patch file of why
> that's OK to do is not ideal.

Thanks for looking into this!

I've added a commit message, please look at the new patch version.

> But the real heart of this small patch
> seems to be this:
>
> +               /*
> +                * We need to materialize newslot because 1) it might
> share oldslot's,
> +                * buffer, which might be released on fetching
> trigtuple from oldslot
> +                * if oldslot is the only owner of buffer,
> +                * and 2) if some trigger returns/stores the old trigtuple,
> +                * and the heap tuple passed to the trigger was located locally,
> +                * newslot might reference that tuple storage, which
> is freed at the
> +                * end of this function.
> +                */
>
> Reading this, I found (1) to be very surprising. I would expect that
> advancing the scan would potentially release the buffer pin, but
> fetching the tuple shouldn't advance the scan. I guess this is
> referring to the call, just below, to ExecFetchSlotHeapTuple. I'm
> guessing that can end up calling tts_buffer_heap_materialize which,
> after materializing copying the tuple, thinks that it's OK to release
> the buffer pin.

Yes, this is what happening there as I diagnosed in [1].

>   But that makes me wonder ... how exactly do oldslot
> and newslot end up sharing the same buffer without holding two pins on
> it? tts_heap_copyslot() looks like it basically forces the tuple to be
> materialized first and then copies that, so you'd end up with, I
> guess, no buffer pins at all, rather than 1 or 2.
>
> I'm obviously missing something important here...

As my analysis [2] showed, epqslot_clean is always equal to newslot, so
ExecCopySlot() isn't called at all.

[1] https://www.postgresql.org/message-id/17798-0907404928dcf0dd%40postgresql.org
[2] https://www.postgresql.org/message-id/e989408c-1838-61cd-c968-1fcf47c6fbba%40gmail.com

Best regards,
Alexander
--------------6Zty0xF4NUBXe53M5TSNHx0E
Content-Type: text/x-patch; charset=UTF-8;
 name="v4-0001-Fix-potential-use-after-free-for-BEFORE-ROW-UPDATE-t.patch"
Content-Disposition: attachment;
 filename*0="v4-0001-Fix-potential-use-after-free-for-BEFORE-ROW-UPDATE-t";
 filename*1=".patch"
Content-Transfer-Encoding: base64

RnJvbSBkNmZhNjcyYWEzNDUwYjQ0MTkyYmVmNGUxODc0ZDk4MzZjYTczYzBhIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBBbGV4YW5kZXIgTGFraGluIDxleGNsdXNpb25AZ21h
aWwuY29tPgpEYXRlOiBNb24sIDggSmFuIDIwMjQgMDg6MTU6NTkgKzAzMDAKU3ViamVjdDog
W1BBVENIXSBGaXggcG90ZW50aWFsIHVzZS1hZnRlci1mcmVlIGZvciBCRUZPUkUgUk9XIFVQ
REFURSB0cmlnZ2VycwoKV2hlbiBvbGRzbG90IGluIEV4ZWNCUlVwZGF0ZVRyaWdnZXJzKCkg
aXMgdGhlIG9ubHkgb3duZXIgb2YgdGhlIHVuZGVybHlpbmcKYnVmZmVyLCBmZXRjaGluZyBh
IHR1cGxlIGZyb20gb2xkc2xvdCBsZWFkcyB0byBmcmVlaW5nIHRoZSBidWZmZXIsIHdoaWNo
IGlzCmFsc28gbWlnaHQgYmUgdXNlZCBieSBuZXdzbG90IHdpdGggbm8gZXhwbGljaXQgcGlu
LiAgVGh1cywgZmV0Y2hpbmcgYSB0dXBsZQpmcm9tIG5ld3Nsb3QgbWlnaHQgbGVhZCB0byBp
bnZhbGlkIHJlYWQuICBUbyBmaXgsIG1hdGVyaWFsaXplIG5ld3Nsb3QgYmVmb3JlCmZldGNo
aW5nIGEgdHVwbGUgZnJvbSBvbGRzbG90LgoKV2l0aCB0aGUgdW5jb25kaXRpb25hbCBFeGVj
TWF0ZXJpbGl6ZVNsb3QoKSBjYWxsIGFkZGVkLCB0aGUgY29uZGl0aW9uYWwgY2FsbAphZGRl
ZCBieSA3NWUwM2VhYmUgYmVjb21lcyByZWR1bmRhbnQsIGhlbmNlIHJlbW92ZSBpdC4KCkFs
c28gcmVtb3ZlIHVucmVhY2hhYmxlIEV4ZWNDb3B5U2xvdCgpIGNhbGwgaW4gRXhlY0JSVXBk
YXRlVHJpZ2dlcnMoKS4KCkRpc2N1c3Npb246IGh0dHBzOi8vcG9zdGdyLmVzL20vMTc3OTgt
MDkwNzQwNDkyOGRjZjBkZEBwb3N0Z3Jlc3FsLm9yZwotLS0KIHNyYy9iYWNrZW5kL2NvbW1h
bmRzL3RyaWdnZXIuYyB8IDMzICsrKysrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tLS0tLQog
MSBmaWxlIGNoYW5nZWQsIDEyIGluc2VydGlvbnMoKyksIDIxIGRlbGV0aW9ucygtKQoKZGlm
ZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3RyaWdnZXIuYyBiL3NyYy9iYWNrZW5k
L2NvbW1hbmRzL3RyaWdnZXIuYwppbmRleCA2ODczYzNiNGQ5Li5hZWFlNjliOTg3IDEwMDY0
NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy90cmlnZ2VyLmMKKysrIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvdHJpZ2dlci5jCkBAIC0yOTc4LDIxICsyOTc4LDIxIEBAIEV4ZWNCUlVw
ZGF0ZVRyaWdnZXJzKEVTdGF0ZSAqZXN0YXRlLCBFUFFTdGF0ZSAqZXBxc3RhdGUsCiAJCSAq
IHJlY2VpdmVkIGluIG5ld3Nsb3QuICBOZWl0aGVyIHdlIG5vciBvdXIgY2FsbGVycyBoYXZl
IGFueSBmdXJ0aGVyCiAJCSAqIGludGVyZXN0IGluIHRoZSBwYXNzZWQtaW4gdHVwbGUsIHNv
IGl0J3Mgb2theSB0byBvdmVyd3JpdGUgbmV3c2xvdAogCQkgKiB3aXRoIHRoZSBuZXdlciBk
YXRhLgotCQkgKgotCQkgKiAoVHlwaWNhbGx5LCBuZXdzbG90IHdhcyBhbHNvIGdlbmVyYXRl
ZCBieSBFeGVjR2V0VXBkYXRlTmV3VHVwbGUsIHNvCi0JCSAqIHRoYXQgZXBxc2xvdF9jbGVh
biB3aWxsIGJlIHRoYXQgc2FtZSBzbG90IGFuZCB0aGUgY29weSBzdGVwIGJlbG93Ci0JCSAq
IGlzIG5vdCBuZWVkZWQuKQogCQkgKi8KIAkJaWYgKGVwcXNsb3RfY2FuZGlkYXRlICE9IE5V
TEwpCi0JCXsKLQkJCVR1cGxlVGFibGVTbG90ICplcHFzbG90X2NsZWFuOwotCi0JCQllcHFz
bG90X2NsZWFuID0gRXhlY0dldFVwZGF0ZU5ld1R1cGxlKHJlbGluZm8sIGVwcXNsb3RfY2Fu
ZGlkYXRlLAotCQkJCQkJCQkJCQkJICBvbGRzbG90KTsKKwkJCW5ld3Nsb3QgPSBFeGVjR2V0
VXBkYXRlTmV3VHVwbGUocmVsaW5mbywgZXBxc2xvdF9jYW5kaWRhdGUsCisJCQkJCQkJCQkJ
CW9sZHNsb3QpOwogCi0JCQlpZiAobmV3c2xvdCAhPSBlcHFzbG90X2NsZWFuKQotCQkJCUV4
ZWNDb3B5U2xvdChuZXdzbG90LCBlcHFzbG90X2NsZWFuKTsKLQkJfQorCQkvKgorCQkgKiBX
ZSBuZWVkIHRvIG1hdGVyaWFsaXplIG5ld3Nsb3QgYmVjYXVzZSAxKSBpdCBtaWdodCBzaGFy
ZSBvbGRzbG90J3MsCisJCSAqIGJ1ZmZlciwgd2hpY2ggbWlnaHQgYmUgcmVsZWFzZWQgb24g
ZmV0Y2hpbmcgdHJpZ3R1cGxlIGZyb20gb2xkc2xvdAorCQkgKiBpZiBvbGRzbG90IGlzIHRo
ZSBvbmx5IG93bmVyIG9mIHRoZSBidWZmZXIsCisJCSAqIGFuZCAyKSBpZiBzb21lIHRyaWdn
ZXIgcmV0dXJucy9zdG9yZXMgdGhlIG9sZCB0cmlndHVwbGUsCisJCSAqIGFuZCB0aGUgaGVh
cCB0dXBsZSBwYXNzZWQgdG8gdGhlIHRyaWdnZXIgd2FzIGxvY2F0ZWQgbG9jYWxseSwKKwkJ
ICogbmV3c2xvdCBtaWdodCByZWZlcmVuY2UgdGhhdCB0dXBsZSBzdG9yYWdlLCB3aGljaCBp
cyBmcmVlZCBhdCB0aGUKKwkJICogZW5kIG9mIHRoaXMgZnVuY3Rpb24uCisJCSAqLworCQlF
eGVjTWF0ZXJpYWxpemVTbG90KG5ld3Nsb3QpOwogCiAJCXRyaWd0dXBsZSA9IEV4ZWNGZXRj
aFNsb3RIZWFwVHVwbGUob2xkc2xvdCwgdHJ1ZSwgJnNob3VsZF9mcmVlX3RyaWcpOwogCX0K
QEAgLTMwNDksMTUgKzMwNDksNiBAQCBFeGVjQlJVcGRhdGVUcmlnZ2VycyhFU3RhdGUgKmVz
dGF0ZSwgRVBRU3RhdGUgKmVwcXN0YXRlLAogCQl7CiAJCQlFeGVjRm9yY2VTdG9yZUhlYXBU
dXBsZShuZXd0dXBsZSwgbmV3c2xvdCwgZmFsc2UpOwogCi0JCQkvKgotCQkJICogSWYgdGhl
IHR1cGxlIHJldHVybmVkIGJ5IHRoZSB0cmlnZ2VyIC8gYmVpbmcgc3RvcmVkLCBpcyB0aGUg
b2xkCi0JCQkgKiByb3cgdmVyc2lvbiwgYW5kIHRoZSBoZWFwIHR1cGxlIHBhc3NlZCB0byB0
aGUgdHJpZ2dlciB3YXMKLQkJCSAqIGFsbG9jYXRlZCBsb2NhbGx5LCBtYXRlcmlhbGl6ZSB0
aGUgc2xvdC4gT3RoZXJ3aXNlIHdlIG1pZ2h0Ci0JCQkgKiBmcmVlIGl0IHdoaWxlIHN0aWxs
IHJlZmVyZW5jZWQgYnkgdGhlIHNsb3QuCi0JCQkgKi8KLQkJCWlmIChzaG91bGRfZnJlZV90
cmlnICYmIG5ld3R1cGxlID09IHRyaWd0dXBsZSkKLQkJCQlFeGVjTWF0ZXJpYWxpemVTbG90
KG5ld3Nsb3QpOwotCiAJCQlpZiAoc2hvdWxkX2ZyZWVfbmV3KQogCQkJCWhlYXBfZnJlZXR1
cGxlKG9sZHR1cGxlKTsKIAotLSAKMi4zNC4xCgo=

--------------6Zty0xF4NUBXe53M5TSNHx0E--