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
Attachments:
0001-Avoid-corrupting-PG_exception_stack-in-ExecAppendAsy.patchtext/x-diff; name=0001-Avoid-corrupting-PG_exception_stack-in-ExecAppendAsy.patchDownload
From 025f40894d6d8f499144f0f7c45c0a124a46c408 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Date: Fri, 23 Feb 2024 13:06:40 +0300
Subject: [PATCH] Avoid corrupting PG_exception_stack in
ExecAppendAsyncEventWait()
After commit 555276f8594087ba15e0d58e38cd2186b9f39f6d
ExecAppendAsyncEventWait() could corrupt PG_exception_stack
after returning in the the middle of PG_TRY()/PG_END_TRY() block.
It should exit only after PG_END_TRY().
---
src/backend/executor/nodeAppend.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index baba3ceea23..42962e80177 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1052,21 +1052,22 @@ ExecAppendAsyncEventWait(AppendState *node)
*/
if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
{
- FreeWaitEventSet(node->as_eventset);
- node->as_eventset = NULL;
- return;
+ /* Return after PG_TRY()/PG_END_TRY() block */
+ noccurred = 0;
}
+ else
+ {
+ /* Return at most EVENT_BUFFER_SIZE events in one call. */
+ if (nevents > EVENT_BUFFER_SIZE)
+ nevents = EVENT_BUFFER_SIZE;
- /* Return at most EVENT_BUFFER_SIZE events in one call. */
- if (nevents > EVENT_BUFFER_SIZE)
- nevents = EVENT_BUFFER_SIZE;
-
- /*
- * If the timeout is -1, wait until at least one event occurs. If the
- * timeout is 0, poll for events, but do not wait at all.
- */
- noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
- nevents, WAIT_EVENT_APPEND_READY);
+ /*
+ * If the timeout is -1, wait until at least one event occurs. If
+ * the timeout is 0, poll for events, but do not wait at all.
+ */
+ noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
+ nevents, WAIT_EVENT_APPEND_READY);
+ }
}
PG_FINALLY();
{
--
2.34.1
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
Attachments:
0001-Avoid-corrupting-PG_exception_stack-in-ExecAppendAsy-v2.patchapplication/octet-stream; name=0001-Avoid-corrupting-PG_exception_stack-in-ExecAppendAsy-v2.patchDownload
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index c5cada55fb..23a57104e8 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -7174,14 +7174,16 @@ postgresForeignAsyncConfigureWait(AsyncRequest *areq)
{
/*
* This is the case when the in-process request was made by another
- * Append. Note that it might be useless to process the request,
- * because the query might not need tuples from that Append anymore.
- * If there are any child subplans of the same parent that are ready
- * for new requests, skip the given request. Likewise, if there are
- * any configured events other than the postmaster death event, skip
- * it. Otherwise, process the in-process request, then begin a fetch
- * to configure the event below, because we might otherwise end up
- * with no configured events other than the postmaster death event.
+ * Append. Note that it might be useless to process the request made
+ * by that Append, because the query might not need tuples from that
+ * Append anymore; so we avoid processing it to begin a fetch for the
+ * given request if possible. If there are any child subplans of the
+ * same parent that are ready for new requests, skip the given
+ * request. Likewise, if there are any configured events other than
+ * the postmaster death event, skip it. Otherwise, process the
+ * in-process request, then begin a fetch to configure the event
+ * below, because we might otherwise end up with no configured events
+ * other than the postmaster death event.
*/
if (!bms_is_empty(requestor->as_needrequest))
return;
diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c
index 99818d3ebc..338484bac9 100644
--- a/src/backend/executor/nodeAppend.c
+++ b/src/backend/executor/nodeAppend.c
@@ -1043,26 +1043,25 @@ ExecAppendAsyncEventWait(AppendState *node)
}
/*
- * No need for further processing if there are no configured events
- * other than the postmaster death event.
+ * If there are no configured events other than the postmaster death
+ * event, we don't need to wait or poll.
*/
if (GetNumRegisteredWaitEvents(node->as_eventset) == 1)
+ noccurred = 0;
+ else
{
- FreeWaitEventSet(node->as_eventset);
- node->as_eventset = NULL;
- return;
- }
+ /* Return at most EVENT_BUFFER_SIZE events in one call. */
+ if (nevents > EVENT_BUFFER_SIZE)
+ nevents = EVENT_BUFFER_SIZE;
- /* Return at most EVENT_BUFFER_SIZE events in one call. */
- if (nevents > EVENT_BUFFER_SIZE)
- nevents = EVENT_BUFFER_SIZE;
-
- /*
- * If the timeout is -1, wait until at least one event occurs. If the
- * timeout is 0, poll for events, but do not wait at all.
- */
- noccurred = WaitEventSetWait(node->as_eventset, timeout, occurred_event,
- nevents, WAIT_EVENT_APPEND_READY);
+ /*
+ * If the timeout is -1, wait until at least one event occurs. If
+ * the timeout is 0, poll for events, but do not wait at all.
+ */
+ noccurred = WaitEventSetWait(node->as_eventset, timeout,
+ occurred_event, nevents,
+ WAIT_EVENT_APPEND_READY);
+ }
}
PG_FINALLY();
{
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