From: Alexander Lakhin <exclusion@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs@lists.postgresql.org, Richard Guo <guofenglinux@gmail.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: Peter Geoghegan <pg@bowt.ie>
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger
Date: Mon, 8 Jan 2024 11:00:00 +0300
Message-ID: <494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com> (raw)
In-Reply-To: <CA+TgmoZz1fGaTnOSf1yQhwYovEEkJZ9iioQXnLgaJz+1fBip=g@mail.gmail.com>
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>
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
Attachments:v4-0001-Fix-potential-use-after-free-for-BEFORE-ROW-UPDATE-t.patch (text/x-patch, 3.0K) download
From d6fa672aa3450b44192bef4e1874d9836ca73c0a Mon Sep 17 00:00:00 2001
From: Alexander Lakhin <exclusion@gmail.com>
Date: Mon, 8 Jan 2024 08:15:59 +0300
Subject: [PATCH] Fix potential use-after-free for BEFORE ROW UPDATE triggers
When oldslot in ExecBRUpdateTriggers() is the only owner of the underlying
buffer, fetching a tuple from oldslot leads to freeing the buffer, which is
also might be used by newslot with no explicit pin. Thus, fetching a tuple
from newslot might lead to invalid read. To fix, materialize newslot before
fetching a tuple from oldslot.
With the unconditional ExecMaterilizeSlot() call added, the conditional call
added by 75e03eabe becomes redundant, hence remove it.
Also remove unreachable ExecCopySlot() call in ExecBRUpdateTriggers().
Discussion: https://postgr.es/m/17798-0907404928dcf0dd@postgresql.org
---
src/backend/commands/trigger.c | 33 ++++++++++++---------------------
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.cindex 6873c3b4d9..aeae69b987 100644--- a/src/backend/commands/trigger.c+++ b/src/backend/commands/trigger.c@@ -2978,21 +2978,21 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
* received in newslot. Neither we nor our callers have any further
* interest in the passed-in tuple, so it's okay to overwrite newslot
* with the newer data.
- *- * (Typically, newslot was also generated by ExecGetUpdateNewTuple, so- * that epqslot_clean will be that same slot and the copy step below- * is not needed.)
*/
if (epqslot_candidate != NULL)
- {- TupleTableSlot *epqslot_clean;-- epqslot_clean = ExecGetUpdateNewTuple(relinfo, epqslot_candidate,- oldslot);+ newslot = ExecGetUpdateNewTuple(relinfo, epqslot_candidate,+ oldslot);- if (newslot != epqslot_clean)- ExecCopySlot(newslot, epqslot_clean);- }+ /*+ * 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 the 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.+ */+ ExecMaterializeSlot(newslot);
trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig);
}
@@ -3049,15 +3049,6 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
{
ExecForceStoreHeapTuple(newtuple, newslot, false);
- /*- * If the tuple returned by the trigger / being stored, is the old- * row version, and the heap tuple passed to the trigger was- * allocated locally, materialize the slot. Otherwise we might- * free it while still referenced by the slot.- */- if (should_free_trig && newtuple == trigtuple)- ExecMaterializeSlot(newslot);-
if (should_free_new)
heap_freetuple(oldtuple);
--
2.34.1
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: exclusion@gmail.com, robertmhaas@gmail.com, tgl@sss.pgh.pa.us, guofenglinux@gmail.com, andres@anarazel.de, pg@bowt.ie
Subject: Re: BUG #17798: Incorrect memory access occurs when using BEFORE ROW UPDATE trigger
In-Reply-To: <494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox