Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger

From: Alexander Lakhin <exclusion(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-bugs(at)lists(dot)postgresql(dot)org, Richard Guo <guofenglinux(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Peter Geoghegan <pg(at)bowt(dot)ie>
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger
Date: 2024-01-08 08:00:00
Message-ID: 494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-bugs

Hello Robert,

06.01.2024 00:18, Robert Haas wrote:
> On Sun, Jul 9, 2023 at 11:00 AM Alexander Lakhin <exclusion(at)gmail(dot)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

Attachment Content-Type Size
v4-0001-Fix-potential-use-after-free-for-BEFORE-ROW-UPDATE-t.patch text/x-patch 3.0 KB

In response to

Responses

Browse pgsql-bugs by date

  From Date Subject
Next Message Kieren Diment 2024-01-08 08:40:34 centos7 setup - repomd.xml signature could not be verified for pgdg-common
Previous Message Dilip Kumar 2024-01-08 05:13:52 Re: BUG #18267: Logical replication bug: data is not synchronized after Alter Publication.