public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system().
11+ messages / 3 participants
[nested] [flat]
* [PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 +-----
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..f9925c8d8e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3730,6 +3730,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *fmt)
+{
+ int nwritten = 0;
+ int ntotal = strlen(fmt);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, fmt + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..20f1b54c6a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -529,11 +529,7 @@ extern void write_pipe_chunks(char *data, int len, int dest);
extern void write_csvlog(ErrorData *edata);
extern void write_jsonlog(ErrorData *edata);
-/*
- * Write errors to stderr (or by equal means when stderr is
- * not available). Used before ereport/elog can be used
- * safely (memory context, GUC load etc)
- */
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void write_stderr_signal_safe(const char *fmt);
#endif /* ELOG_H */
--
2.25.1
--IS0zKkzwUGydFO0o
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v14-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 +-----
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..f9925c8d8e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3730,6 +3730,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *fmt)
+{
+ int nwritten = 0;
+ int ntotal = strlen(fmt);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, fmt + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..20f1b54c6a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -529,11 +529,7 @@ extern void write_pipe_chunks(char *data, int len, int dest);
extern void write_csvlog(ErrorData *edata);
extern void write_jsonlog(ErrorData *edata);
-/*
- * Write errors to stderr (or by equal means when stderr is
- * not available). Used before ereport/elog can be used
- * safely (memory context, GUC load etc)
- */
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void write_stderr_signal_safe(const char *fmt);
#endif /* ELOG_H */
--
2.25.1
--Q68bSM7Ycu6FN28Q--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v13 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 +-----
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..f9925c8d8e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3730,6 +3730,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *fmt)
+{
+ int nwritten = 0;
+ int ntotal = strlen(fmt);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, fmt + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..20f1b54c6a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -529,11 +529,7 @@ extern void write_pipe_chunks(char *data, int len, int dest);
extern void write_csvlog(ErrorData *edata);
extern void write_jsonlog(ErrorData *edata);
-/*
- * Write errors to stderr (or by equal means when stderr is
- * not available). Used before ereport/elog can be used
- * safely (memory context, GUC load etc)
- */
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void write_stderr_signal_safe(const char *fmt);
#endif /* ELOG_H */
--
2.25.1
--5vNYLRcllDrimb99
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v13-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v11 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds checks in proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 4 ++++
src/backend/storage/lmgr/proc.c | 8 ++++++++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 ++++++
5 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..6591b5d6a8 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,10 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* not safe if forked by system(), etc. */
+ if (MyProcPid != (int) getpid())
+ elog(PANIC, "proc_exit() called in child process");
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 5b663a2997..e9e445bb21 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -806,6 +806,10 @@ ProcKill(int code, Datum arg)
Assert(MyProc != NULL);
+ /* not safe if forked by system(), etc. */
+ if (MyProc->pid != (int) getpid())
+ elog(PANIC, "ProcKill() called in child process");
+
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -926,6 +930,10 @@ AuxiliaryProcKill(int code, Datum arg)
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ /* not safe if forked by system(), etc. */
+ if (MyProc->pid != (int) getpid())
+ elog(PANIC, "AuxiliaryProcKill() called in child process");
+
auxproc = &AuxiliaryProcs[proctype];
Assert(MyProc == auxproc);
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 8e1f3e8521..eeb238331e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3733,6 +3733,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *str)
+{
+ int nwritten = 0;
+ int ntotal = strlen(str);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, str + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..0292e88b4f 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -536,4 +536,10 @@ extern void write_jsonlog(ErrorData *edata);
*/
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ */
+extern void write_stderr_signal_safe(const char *fmt);
+
#endif /* ELOG_H */
--
2.25.1
--jRHKVT23PllUwdXP--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v15 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
src/backend/utils/error/elog.c | 28 ++++++++++++++++++++++++++++
src/include/utils/elog.h | 6 +-----
5 files changed, 50 insertions(+), 6 deletions(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..0e7de26bc2 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr_signal_safe("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5898100acb..f9925c8d8e 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -3730,6 +3730,34 @@ write_stderr(const char *fmt,...)
}
+/*
+ * Write a message to STDERR using only async-signal-safe functions. This can
+ * be used to safely emit a message from a signal handler.
+ *
+ * TODO: It is likely possible to safely do a limited amount of string
+ * interpolation (e.g., %s and %d), but that is not presently supported.
+ */
+void
+write_stderr_signal_safe(const char *fmt)
+{
+ int nwritten = 0;
+ int ntotal = strlen(fmt);
+
+ while (nwritten < ntotal)
+ {
+ int rc;
+
+ rc = write(STDERR_FILENO, fmt + nwritten, ntotal - nwritten);
+
+ /* Just give up on error. There isn't much else we can do. */
+ if (rc == -1)
+ return;
+
+ nwritten += rc;
+ }
+}
+
+
/*
* Adjust the level of a recovery-related message per trace_recovery_messages.
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaa..20f1b54c6a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -529,11 +529,7 @@ extern void write_pipe_chunks(char *data, int len, int dest);
extern void write_csvlog(ErrorData *edata);
extern void write_jsonlog(ErrorData *edata);
-/*
- * Write errors to stderr (or by equal means when stderr is
- * not available). Used before ereport/elog can be used
- * safely (memory context, GUC load etc)
- */
extern void write_stderr(const char *fmt,...) pg_attribute_printf(1, 2);
+extern void write_stderr_signal_safe(const char *fmt);
#endif /* ELOG_H */
--
2.25.1
--k+w/mQv8wyuph6w0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v15-0003-introduce-routine-for-checking-mutually-exclusiv.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..de2b56c2fa 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..e3da0622d7 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--wac7ysb48OaltWcw--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v7 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..bace915881 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process\n";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..e3da0622d7 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProcPid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--u3/rZRmxL6MmkK24--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v8 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 20 +++++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..bace915881 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,23 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ const char msg[] = "StartupProcShutdownHandler() called in child process\n";
+ int rc pg_attribute_unused();
+
+ rc = write(STDERR_FILENO, msg, sizeof(msg));
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--FCuugMFkClbJLl1L--
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v9 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system().
@ 2023-02-14 17:44 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2023-02-14 17:44 UTC (permalink / raw)
Instead, emit a message to STDERR and _exit() in this case. This
change also adds assertions to proc_exit(), ProcKill(), and
AuxiliaryProcKill() to verify that these functions are not called
by a process forked by system(), etc.
---
src/backend/postmaster/startup.c | 17 ++++++++++++++++-
src/backend/storage/ipc/ipc.c | 3 +++
src/backend/storage/lmgr/proc.c | 2 ++
3 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index efc2580536..261f992357 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -19,6 +19,8 @@
*/
#include "postgres.h"
+#include <unistd.h>
+
#include "access/xlog.h"
#include "access/xlogrecovery.h"
#include "access/xlogutils.h"
@@ -121,7 +123,20 @@ StartupProcShutdownHandler(SIGNAL_ARGS)
int save_errno = errno;
if (in_restore_command)
- proc_exit(1);
+ {
+ /*
+ * If we are in a child process (e.g., forked by system() in
+ * RestoreArchivedFile()), we don't want to call any exit callbacks.
+ * The parent will take care of that.
+ */
+ if (MyProcPid == (int) getpid())
+ proc_exit(1);
+ else
+ {
+ write_stderr("StartupProcShutdownHandler() called in child process\n");
+ _exit(1);
+ }
+ }
else
shutdown_requested = true;
WakeupRecovery();
diff --git a/src/backend/storage/ipc/ipc.c b/src/backend/storage/ipc/ipc.c
index 1904d21795..d5097dc008 100644
--- a/src/backend/storage/ipc/ipc.c
+++ b/src/backend/storage/ipc/ipc.c
@@ -103,6 +103,9 @@ static int on_proc_exit_index,
void
proc_exit(int code)
{
+ /* proc_exit() is not safe in forked processes from system(), etc. */
+ Assert(MyProcPid == (int) getpid());
+
/* Clean up everything that must be cleaned up */
proc_exit_prepare(code);
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 22b4278610..b9e2c3aafe 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -805,6 +805,7 @@ ProcKill(int code, Datum arg)
dlist_head *procgloballist;
Assert(MyProc != NULL);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
/* Make sure we're out of the sync rep lists */
SyncRepCleanupAtProcExit();
@@ -925,6 +926,7 @@ AuxiliaryProcKill(int code, Datum arg)
PGPROC *proc;
Assert(proctype >= 0 && proctype < NUM_AUXILIARY_PROCS);
+ Assert(MyProc->pid == (int) getpid()); /* not safe if forked by system(), etc. */
auxproc = &AuxiliaryProcs[proctype];
--
2.25.1
--IS0zKkzwUGydFO0o--
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Another WaitEventSet resource leakage in back branches
@ 2024-04-11 16:29 Andres Freund <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Andres Freund @ 2024-04-11 16:29 UTC (permalink / raw)
To: Etsuro Fujita <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Another WaitEventSet resource leakage in back branches
@ 2024-04-12 11:21 Etsuro Fujita <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Etsuro Fujita @ 2024-04-12 11:21 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
Hi Andres,
On Fri, Apr 12, 2024 at 1:29 AM Andres Freund <[email protected]> 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
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-04-12 11:21 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-14 17:44 [PATCH v14 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v11 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v8 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v9 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v10 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v13 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v15 2/7] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v6 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2023-02-14 17:44 [PATCH v7 2/2] Don't proc_exit() in startup's SIGTERM handler if forked by system(). Nathan Bossart <[email protected]>
2024-04-11 16:29 Re: Another WaitEventSet resource leakage in back branches Andres Freund <[email protected]>
2024-04-12 11:21 ` Re: Another WaitEventSet resource leakage in back branches Etsuro Fujita <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox