Обсуждение: ExecAppendAsyncEventWait() in REL_14_STABLE can corrupt PG_exception_stack

Hi.

Recent commit 555276f8594087ba15e0d58e38cd2186b9f39f6d introduced final 
cleanup of node->as_eventset in ExecAppendAsyncEventWait().
Unfortunately, now this function can return in the middle of TRY/FINALLY 
block, without restoring PG_exception_stack.

We found this while working on our FDW. Unfortunately, I couldn't 
reproduce the issue with postgres_fdw, but it seems it is also affected.

The following patch heals the issue.

-- l
Best regards,
Alexander Pyhalov,
Postgres Professional

On Fri, Feb 23, 2024 at 01:21:14PM +0300, Alexander Pyhalov wrote:
> Recent commit 555276f8594087ba15e0d58e38cd2186b9f39f6d introduced final
> cleanup of node->as_eventset in ExecAppendAsyncEventWait().
> Unfortunately, now this function can return in the middle of TRY/FINALLY
> block, without restoring PG_exception_stack.
>
> We found this while working on our FDW. Unfortunately, I couldn't reproduce
> the issue with postgres_fdw, but it seems it is also affected.

Ugh, yes, you are obviously right that the early return is wrong.
I'll look into fixing that where appropriate.  Thanks for the report,
Alexander!
--
Michael

Hi,

On Sat, Feb 24, 2024 at 10:06 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Feb 23, 2024 at 01:21:14PM +0300, Alexander Pyhalov wrote:
> > Recent commit 555276f8594087ba15e0d58e38cd2186b9f39f6d introduced final
> > cleanup of node->as_eventset in ExecAppendAsyncEventWait().
> > Unfortunately, now this function can return in the middle of TRY/FINALLY
> > block, without restoring PG_exception_stack.
> >
> > We found this while working on our FDW. Unfortunately, I couldn't reproduce
> > the issue with postgres_fdw, but it seems it is also affected.

I think this would happen when FDWs configure no events; IIRC I think
while the core allows them to do so, postgres_fdw does not do so, so
this would never happen with it.  Anyway, thanks for the report and
patch, Alexander!

> Ugh, yes, you are obviously right that the early return is wrong.
> I'll look into fixing that where appropriate.

Thanks for taking care of this, Michael-san!  This would result
originally from my fault, so If you don't mind, could you let me do
that?

Best regards,
Etsuro Fujita

On Sun, Feb 25, 2024 at 06:34:30PM +0900, Etsuro Fujita wrote:
> I think this would happen when FDWs configure no events; IIRC I think
> while the core allows them to do so, postgres_fdw does not do so, so
> this would never happen with it.  Anyway, thanks for the report and
> patch, Alexander!

I don't see how that's directly your fault as this is a thinko in the
set of commits 481d7d1c01, 555276f859 and 501cfd07da that have hit
14~16, ignoring entirely the TRY/CATCH block.

Anyway, if you want to address it yourself, feel free to go ahead,
thanks!  I would have done it but I've been busy with life stuff for
the last couple of days.
--
Michael

On Mon, Feb 26, 2024 at 8:37 AM Michael Paquier <michael@paquier.xyz> wrote:
> I don't see how that's directly your fault as this is a thinko in the
> set of commits 481d7d1c01, 555276f859 and 501cfd07da that have hit
> 14~16, ignoring entirely the TRY/CATCH block.

The set of commits is actually a fix for resource leaks in my commit 27e1f1456.

> Anyway, if you want to address it yourself, feel free to go ahead,
> thanks!  I would have done it but I've been busy with life stuff for
> the last couple of days.

Will do.  (I was thinking you would get busy from now on.)

Best regards,
Etsuro Fujita

On Mon, Feb 26, 2024 at 04:29:44PM +0900, Etsuro Fujita wrote:
> Will do.  (I was thinking you would get busy from now on.)

Fujita-san, have you been able to look at this thread?
--
Michael

Hi Michael-san,

On Mon, Mar 11, 2024 at 8:12 AM Michael Paquier <michael@paquier.xyz> wrote:
> On Mon, Feb 26, 2024 at 04:29:44PM +0900, Etsuro Fujita wrote:
> > Will do.  (I was thinking you would get busy from now on.)
>
> Fujita-san, have you been able to look at this thread?

Yeah, I took a look; the patch looks good to me, but I am thiking to
update some comments in a related function in postgres_fdw.c.  I will
have time to work on this later this week, so I would like to propose
an updated patch then.

Thanks for taking care of this!

Best regards,
Etsuro Fujita

On Sun, Feb 25, 2024 at 6:34 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:

> > On Fri, Feb 23, 2024 at 01:21:14PM +0300, Alexander Pyhalov wrote:
> > > Recent commit 555276f8594087ba15e0d58e38cd2186b9f39f6d introduced final
> > > cleanup of node->as_eventset in ExecAppendAsyncEventWait().
> > > Unfortunately, now this function can return in the middle of TRY/FINALLY
> > > block, without restoring PG_exception_stack.
> > >
> > > We found this while working on our FDW. Unfortunately, I couldn't reproduce
> > > the issue with postgres_fdw, but it seems it is also affected.
>
> I think this would happen when FDWs configure no events; IIRC I think
> while the core allows them to do so, postgres_fdw does not do so, so
> this would never happen with it.

I was wrong; as you pointed out, this would affect postgres_fdw as
well.  See commit 1ec7fca85, which is my commit, but I forgot it
completely.  :-(

As I said before, the patch looks good to me.  I tweaked comments in
ExecAppendAsyncEventWait() a bit.  Attached is an updated patch.  In
the patch I also fixed a confusing comment in a related function in
postgres_fdw.c about handling of the in-process request that might be
useless to process.

Sorry, it took more time than expected to get back to this thread.

Best regards,
Etsuro Fujita

Etsuro Fujita писал(а) 2024-03-21 13:59:
> On Sun, Feb 25, 2024 at 6:34 PM Etsuro Fujita <etsuro.fujita@gmail.com> 
> wrote:
> 
>> > On Fri, Feb 23, 2024 at 01:21:14PM +0300, Alexander Pyhalov wrote:
>> > > Recent commit 555276f8594087ba15e0d58e38cd2186b9f39f6d introduced final
>> > > cleanup of node->as_eventset in ExecAppendAsyncEventWait().
>> > > Unfortunately, now this function can return in the middle of TRY/FINALLY
>> > > block, without restoring PG_exception_stack.
>> > >
>> > > We found this while working on our FDW. Unfortunately, I couldn't reproduce
>> > > the issue with postgres_fdw, but it seems it is also affected.
>> 
>> I think this would happen when FDWs configure no events; IIRC I think
>> while the core allows them to do so, postgres_fdw does not do so, so
>> this would never happen with it.
> 
> I was wrong; as you pointed out, this would affect postgres_fdw as
> well.  See commit 1ec7fca85, which is my commit, but I forgot it
> completely.  :-(
> 
> As I said before, the patch looks good to me.  I tweaked comments in
> ExecAppendAsyncEventWait() a bit.  Attached is an updated patch.  In
> the patch I also fixed a confusing comment in a related function in
> postgres_fdw.c about handling of the in-process request that might be
> useless to process.
> 
> Sorry, it took more time than expected to get back to this thread.
> 

Hi. The updated patch still looks good to me.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

Hi Alexander,

On Fri, Mar 22, 2024 at 7:23 PM Alexander Pyhalov
<a.pyhalov@postgrespro.ru> wrote:
> The updated patch still looks good to me.

Great!  I am planning to apply the patch to the back branches next week.

Thanks for the review!

Best regards,
Etsuro Fujita

On Fri, Mar 22, 2024 at 9:09 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> On Fri, Mar 22, 2024 at 7:23 PM Alexander Pyhalov
> <a.pyhalov@postgrespro.ru> wrote:
> > The updated patch still looks good to me.
>
> I am planning to apply the patch to the back branches next week.

Pushed.  Sorry for the delay.

Best regards,
Etsuro Fujita

On Thu, Apr 04, 2024 at 06:08:40PM +0900, Etsuro Fujita wrote:
> Pushed.  Sorry for the delay.

Thanks!
--
Michael