Another WaitEventSet resource leakage in back branches
Hi,
While working on [1]/messages/by-id/CAPmGK15DF6EE7O6hTLbe5-fHvPDwEx9vm-BOCN3dsKOjZCo7bw@mail.gmail.com, I noticed $SUBJECT: WaitLatchOrSocket in back
branches is ignoring the possibility of failing partway through, too.
I added a PG_FAINALLY block to that function, like commit 555276f85.
Patch attached.
Best regards,
Etsuro Fujita
[1]: /messages/by-id/CAPmGK15DF6EE7O6hTLbe5-fHvPDwEx9vm-BOCN3dsKOjZCo7bw@mail.gmail.com
Attachments:
fix-another-WaitEventSet-resource-leakage.patchapplication/octet-stream; name=fix-another-WaitEventSet-resource-leakage.patchDownload
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index cdb95c1931..767328039e 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -545,48 +545,54 @@ WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
WaitEvent event;
WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3);
- if (wakeEvents & WL_TIMEOUT)
- Assert(timeout >= 0);
- else
- timeout = -1;
+ PG_TRY();
+ {
+ if (wakeEvents & WL_TIMEOUT)
+ Assert(timeout >= 0);
+ else
+ timeout = -1;
- if (wakeEvents & WL_LATCH_SET)
- AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- latch, NULL);
+ if (wakeEvents & WL_LATCH_SET)
+ AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
+ latch, NULL);
- /* Postmaster-managed callers must handle postmaster death somehow. */
- Assert(!IsUnderPostmaster ||
- (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
- (wakeEvents & WL_POSTMASTER_DEATH));
+ /* Postmaster-managed callers must handle postmaster death somehow. */
+ Assert(!IsUnderPostmaster ||
+ (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
+ (wakeEvents & WL_POSTMASTER_DEATH));
- if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if (wakeEvents & WL_SOCKET_MASK)
- {
- int ev;
+ if (wakeEvents & WL_SOCKET_MASK)
+ {
+ int ev;
- ev = wakeEvents & WL_SOCKET_MASK;
- AddWaitEventToSet(set, ev, sock, NULL, NULL);
- }
+ ev = wakeEvents & WL_SOCKET_MASK;
+ AddWaitEventToSet(set, ev, sock, NULL, NULL);
+ }
- rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
+ rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
- if (rc == 0)
- ret |= WL_TIMEOUT;
- else
+ if (rc == 0)
+ ret |= WL_TIMEOUT;
+ else
+ {
+ ret |= event.events & (WL_LATCH_SET |
+ WL_POSTMASTER_DEATH |
+ WL_SOCKET_MASK);
+ }
+ }
+ PG_FINALLY();
{
- ret |= event.events & (WL_LATCH_SET |
- WL_POSTMASTER_DEATH |
- WL_SOCKET_MASK);
+ FreeWaitEventSet(set);
}
-
- FreeWaitEventSet(set);
+ PG_END_TRY();
return ret;
}
On Fri, Mar 22, 2024 at 9:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
While working on [1], I noticed $SUBJECT: WaitLatchOrSocket in back
branches is ignoring the possibility of failing partway through, too.
I added a PG_FAINALLY block to that function, like commit 555276f85.
Patch attached.
I noticed that PG_FAINALLY was added in v13. I created a separate
patch for v12 using PG_CATCH instead. Patch attached. I am attaching
the previous patch for later versions as well.
I am planning to back-patch these next week.
Best regards,
Etsuro Fujita
Attachments:
fix-another-WaitEventSet-resource-leakage.patchapplication/octet-stream; name=fix-another-WaitEventSet-resource-leakage.patchDownload
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index cdb95c1931..767328039e 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -545,48 +545,54 @@ WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
WaitEvent event;
WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3);
- if (wakeEvents & WL_TIMEOUT)
- Assert(timeout >= 0);
- else
- timeout = -1;
+ PG_TRY();
+ {
+ if (wakeEvents & WL_TIMEOUT)
+ Assert(timeout >= 0);
+ else
+ timeout = -1;
- if (wakeEvents & WL_LATCH_SET)
- AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- latch, NULL);
+ if (wakeEvents & WL_LATCH_SET)
+ AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
+ latch, NULL);
- /* Postmaster-managed callers must handle postmaster death somehow. */
- Assert(!IsUnderPostmaster ||
- (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
- (wakeEvents & WL_POSTMASTER_DEATH));
+ /* Postmaster-managed callers must handle postmaster death somehow. */
+ Assert(!IsUnderPostmaster ||
+ (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
+ (wakeEvents & WL_POSTMASTER_DEATH));
- if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if (wakeEvents & WL_SOCKET_MASK)
- {
- int ev;
+ if (wakeEvents & WL_SOCKET_MASK)
+ {
+ int ev;
- ev = wakeEvents & WL_SOCKET_MASK;
- AddWaitEventToSet(set, ev, sock, NULL, NULL);
- }
+ ev = wakeEvents & WL_SOCKET_MASK;
+ AddWaitEventToSet(set, ev, sock, NULL, NULL);
+ }
- rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
+ rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
- if (rc == 0)
- ret |= WL_TIMEOUT;
- else
+ if (rc == 0)
+ ret |= WL_TIMEOUT;
+ else
+ {
+ ret |= event.events & (WL_LATCH_SET |
+ WL_POSTMASTER_DEATH |
+ WL_SOCKET_MASK);
+ }
+ }
+ PG_FINALLY();
{
- ret |= event.events & (WL_LATCH_SET |
- WL_POSTMASTER_DEATH |
- WL_SOCKET_MASK);
+ FreeWaitEventSet(set);
}
-
- FreeWaitEventSet(set);
+ PG_END_TRY();
return ret;
}
fix-another-WaitEventSet-resource-leakage-PG12.patchapplication/octet-stream; name=fix-another-WaitEventSet-resource-leakage-PG12.patchDownload
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index d35b7ee7d1..d5f7f8d636 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -374,46 +374,55 @@ WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
WaitEvent event;
WaitEventSet *set = CreateWaitEventSet(CurrentMemoryContext, 3);
- if (wakeEvents & WL_TIMEOUT)
- Assert(timeout >= 0);
- else
- timeout = -1;
+ PG_TRY();
+ {
+ if (wakeEvents & WL_TIMEOUT)
+ Assert(timeout >= 0);
+ else
+ timeout = -1;
- if (wakeEvents & WL_LATCH_SET)
- AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- latch, NULL);
+ if (wakeEvents & WL_LATCH_SET)
+ AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
+ latch, NULL);
- /* Postmaster-managed callers must handle postmaster death somehow. */
- Assert(!IsUnderPostmaster ||
- (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
- (wakeEvents & WL_POSTMASTER_DEATH));
+ /* Postmaster-managed callers must handle postmaster death somehow. */
+ Assert(!IsUnderPostmaster ||
+ (wakeEvents & WL_EXIT_ON_PM_DEATH) ||
+ (wakeEvents & WL_POSTMASTER_DEATH));
- if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_POSTMASTER_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_POSTMASTER_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
- AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
- NULL, NULL);
+ if ((wakeEvents & WL_EXIT_ON_PM_DEATH) && IsUnderPostmaster)
+ AddWaitEventToSet(set, WL_EXIT_ON_PM_DEATH, PGINVALID_SOCKET,
+ NULL, NULL);
- if (wakeEvents & WL_SOCKET_MASK)
- {
- int ev;
+ if (wakeEvents & WL_SOCKET_MASK)
+ {
+ int ev;
- ev = wakeEvents & WL_SOCKET_MASK;
- AddWaitEventToSet(set, ev, sock, NULL, NULL);
- }
+ ev = wakeEvents & WL_SOCKET_MASK;
+ AddWaitEventToSet(set, ev, sock, NULL, NULL);
+ }
- rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
+ rc = WaitEventSetWait(set, timeout, &event, 1, wait_event_info);
- if (rc == 0)
- ret |= WL_TIMEOUT;
- else
+ if (rc == 0)
+ ret |= WL_TIMEOUT;
+ else
+ {
+ ret |= event.events & (WL_LATCH_SET |
+ WL_POSTMASTER_DEATH |
+ WL_SOCKET_MASK);
+ }
+ }
+ PG_CATCH();
{
- ret |= event.events & (WL_LATCH_SET |
- WL_POSTMASTER_DEATH |
- WL_SOCKET_MASK);
+ FreeWaitEventSet(set);
+ PG_RE_THROW();
}
+ PG_END_TRY();
FreeWaitEventSet(set);
On Fri, Apr 5, 2024 at 7:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
I am planning to back-patch these next week.
Done.
Best regards,
Etsuro Fujita
Hi,
On 2024-03-22 21:15:45 +0900, Etsuro Fujita wrote:
While working on [1], I noticed $SUBJECT: WaitLatchOrSocket in back
branches is ignoring the possibility of failing partway through, too.
I added a PG_FAINALLY block to that function, like commit 555276f85.
Patch attached.
Could you expand a bit on the concrete scenario you're worried about here?
PG_TRY/CATCH aren't free, so adding something like this to a quite common
path, in the back branches, without a concrete analysis as to why it's needed,
seems a bit scary.
Greetings,
Andres Freund
Hi Andres,
On Fri, Apr 12, 2024 at 1:29 AM Andres Freund <andres@anarazel.de> wrote:
On 2024-03-22 21:15:45 +0900, Etsuro Fujita wrote:
While working on [1], I noticed $SUBJECT: WaitLatchOrSocket in back
branches is ignoring the possibility of failing partway through, too.
I added a PG_FAINALLY block to that function, like commit 555276f85.
Patch attached.Could you expand a bit on the concrete scenario you're worried about here?
PG_TRY/CATCH aren't free, so adding something like this to a quite common
path, in the back branches, without a concrete analysis as to why it's needed,
seems a bit scary.
What I am worried about is that system calls used in
WaitLatchOrSocket, like epoll_ctl, might fail, throwing an error
(epoll_ctl might fail due to eg, ENOMEM or ENOSPC). The probability
of such failures would be pretty low, but not zero.
This causes more problems than it solves?
Thanks for the comment!
Best regards,
Etsuro Fujita