Message-ID: <b64c183f-4ae1-0b16-afcf-11101ebc4213@gmail.com>
Date: Thu, 11 Jan 2024 06: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>
 <494cd7c4-c109-6886-c0e3-138a95e7e38b@gmail.com>
 <CA+TgmoYAsQWMqMUNNdyp8V0KVxMoz=0y+3XNCf3ST-o1yyz5Jg@mail.gmail.com>
 <74335d44-819d-60e0-859a-efe74d9c84c2@gmail.com>
 <CA+TgmoYkOp=jDg15ugJDwxqoA6drAPBZmOJkhZJ11HZWhEYx6w@mail.gmail.com>
From: Alexander Lakhin <exclusion@gmail.com>
In-Reply-To: 
 <CA+TgmoYkOp=jDg15ugJDwxqoA6drAPBZmOJkhZJ11HZWhEYx6w@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Archived-At: 
 <https://www.postgresql.org/message-id/b64c183f-4ae1-0b16-afcf-11101ebc4213%40gmail.com>
Precedence: bulk

10.01.2024 23:04, Robert Haas wrote:
> OK, thanks for the further explanation. I think I'm starting to
> understand what's going on here. Maybe you already understand all of
> this, but in my own words:
>
> ExecGetUpdateNewTuple() calls ExecProject() which documents that the
> results are short-lived and you should materialize the slot if that's
> a problem. ExecModifyTable() does slot = ExecGetUpdateNewTuple(...),
> does not materialize it, and then passes that slot as the penultimate
> argument to ExecUpdate(), which calls ExecUpdatePrologue(), which
> calls ExecBRUpdateTriggers(), and the value returned by
> ExecModifyTable()'s call to ExecGetUpdateNewTuple() is now stored in
> newslot, but it still hasn't been materialized. So any operation
> that's performed on oldslot at this point invalidates newslot.

Yes, I see this the same way.

> But ExecBRUpdateTriggers() now calls GetTupleForTrigger() to clobber
> oldslot, so now newslot is also junk. That's technically OK, because
> we're not going to access the contents of newslot any more. We're
> instead going to refresh them:
>
>          if (epqslot_candidate != NULL)
>          {
>              TupleTableSlot *epqslot_clean;
>
>              epqslot_clean = ExecGetUpdateNewTuple(relinfo, epqslot_candidate,
>                                                    oldslot);
>
>              if (newslot != epqslot_clean)
>                  ExecCopySlot(newslot, epqslot_clean);
>          }
>
> Since the slot returned by this call to ExecGetUpdateNewTuple() is a
> function of the same projection info as when it was called from
> ExecModifyTable(), the slot returned here must be the same slot that
> was returned there, namely newslot. So, as you say, newslot ==
> epqslot_clean, and we've just clobbered it with updated values.
> Therefore we're back in the situation where oldslot is fully valid and
> newslot is only valid until somebody does something to oldslot. Thus,
> the portion of your patch that simplifies this code doesn't fix any
> bug, but rather just simplifies some unnecessarily complex logic.

Yes, in that portion I was just trying to address Tom's suspicion: "Maybe
we need to materialize even if newslot != epqslot_clean"...

>   But
> the next thing that happens in the existing code is:
>
>          trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig);
>
> That is going to again invalidate newslot, because it can materialize
> oldslot. That's a problem if we ever use newslot after this point, and
> we do, unless we materialize newslot first, so your patch adds
> ExecMaterializeSlot(newslot) right before this, fixing the problem.

Yes, that's the way I find possible to fix it.

> What I don't really understand is why failure to do this causes an
> invalid memory access in ExecFetchSlotHeapTuple(). According to the
> above analysis, the invalid memory access ought to occur the first
> time something touches newslot after this call to
> ExecFetchSlotHeapTuple(), but if I understand your valgrind trace
> correctly, it's showing that ExecFetchSlotHeapTuple() is touching
> something it shouldn't. Do you understand what's happening here?

Maybe you are confused by the first call to ExecFetchSlotHeapTuple()?
The full Valgrind stack from my yesterday's test run is:
==00:00:00:05.052 3934530== Invalid read of size 1
==00:00:00:05.052 3934530==    at 0x1EECAC: heap_compute_data_size (heaptuple.c:244)
==00:00:00:05.052 3934530==    by 0x1EFA84: heap_form_tuple (heaptuple.c:1158)
==00:00:00:05.052 3934530==    by 0x423B5E: tts_buffer_heap_materialize (execTuples.c:747)
==00:00:00:05.052 3934530==    by 0x424EB1: ExecFetchSlotHeapTuple (execTuples.c:1653)
==00:00:00:05.052 3934530==    by 0x3ECD45: ExecBRUpdateTriggers (trigger.c:3055)
==00:00:00:05.052 3934530==    by 0x4483F0: ExecUpdatePrologue (nodeModifyTable.c:1923)
==00:00:00:05.052 3934530==    by 0x44A146: ExecUpdate (nodeModifyTable.c:2284)
==00:00:00:05.052 3934530==    by 0x44CE4E: ExecModifyTable (nodeModifyTable.c:3844)
==00:00:00:05.052 3934530==    by 0x41E9A1: ExecProcNodeFirst (execProcnode.c:464)
==00:00:00:05.052 3934530==    by 0x416F5D: ExecProcNode (executor.h:273)
==00:00:00:05.052 3934530==    by 0x416F5D: ExecutePlan (execMain.c:1670)
==00:00:00:05.052 3934530==    by 0x417120: standard_ExecutorRun (execMain.c:365)
==00:00:00:05.052 3934530==    by 0x4171FA: ExecutorRun (execMain.c:309)
==00:00:00:05.052 3934530==  Address 0x9982c74 is in a rw- anonymous segment
==00:00:00:05.052 3934530==

here trigger.c:3055 points at the second call in ExecBRUpdateTriggers():
             newtuple = ExecFetchSlotHeapTuple(newslot, true, &should_free_new);

Does it makes more sense?

> I think the question we should be asking ourselves here is whether
> we're really OK with having such a vast distance between the point
> where newslot is first populated and where materialization happens. It
> seems like playing with fire to populate a slot with ephemeral data in
> ExecModifyTable() and then press the materialize button four stack
> frames down. There's a lot of opportunity for things to go wrong
> there. On the other hand, this is very performance-sensitive code, so
> maybe materializing here is the only workable solution. If that's the
> case, though, we should probably add comments making the lifetime of
> various slots clearer, because neither Tom nor I was able to
> understand what the heck was happening here on first reading, which is
> probably not a good sign.

I was discouraged by that vast distance and implicit buffer usage too, but
I found no other feasible way to fix it. On the other hand, 75e03eabe and
Andres's words upthread made me believe that it's an acceptable solution.

Thank you very much for diving deep into this subject!

Best regards,
Alexander