From e217c57624fd9bf952efff1476fec32604ef56d4 Mon Sep 17 00:00:00 2001 From: wangxiaoran Date: Mon, 19 Aug 2024 11:44:00 +0800 Subject: [PATCH] Imporve pg_re_throw: check if sigjmp_buf is valid and report error If the code in PG_TRY contains any non local control flow other than ereport(ERROR) like goto, break etc., the PG_CATCH or PG_END_TRY cannot be called, then the PG_exception_stack will point to the memory whose stack frame has been released. So after that, when the pg_re_throw called, __longjmp() will crash and report Segmentation fault error. In that case, to help developers to figure out the root cause easily, it is better to report that 'the sigjmp_buf is invalid' rather than letting the __longjmp report any error. Addition to sigjmp_buf, add another field 'int magic' which is next to the sigjum_buf in the local stack frame memory. The magic's value is always 'PG_exception_magic 0x12345678'. And in 'pg_re_throw' routine, check if the magic's value is still '0x12345678', if not, that means the memory where the 'PG_exception_stack' points to has been released, and the 'sigbuf' must be invalid. --- src/backend/postmaster/autovacuum.c | 8 ++++---- src/backend/postmaster/bgworker.c | 4 ++-- src/backend/postmaster/bgwriter.c | 4 ++-- src/backend/postmaster/checkpointer.c | 5 ++--- src/backend/postmaster/pgarch.c | 4 ++-- src/backend/postmaster/walsummarizer.c | 4 ++-- src/backend/postmaster/walwriter.c | 4 ++-- src/backend/replication/logical/slotsync.c | 4 ++-- src/backend/tcop/postgres.c | 5 ++--- src/backend/utils/error/elog.c | 12 ++++++++++-- src/include/utils/elog.h | 14 ++++++++++---- 11 files changed, 40 insertions(+), 28 deletions(-) diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c index 7d0877c95e..4076d99938 100644 --- a/src/backend/postmaster/autovacuum.c +++ b/src/backend/postmaster/autovacuum.c @@ -361,7 +361,7 @@ static void avl_sigusr2_handler(SIGNAL_ARGS); void AutoVacLauncherMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; Assert(startup_data_len == 0); @@ -436,7 +436,7 @@ AutoVacLauncherMain(char *startup_data, size_t startup_data_len) * call redundant, but it is not since InterruptPending might be set * already. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; @@ -1359,7 +1359,7 @@ avl_sigusr2_handler(SIGNAL_ARGS) void AutoVacWorkerMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; Oid dbid; Assert(startup_data_len == 0); @@ -1421,7 +1421,7 @@ AutoVacWorkerMain(char *startup_data, size_t startup_data_len) * seem that this policy makes the HOLD_INTERRUPTS() call redundant, but * it is not since InterruptPending might be set already. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c index b83967cda3..d9cb01cd3a 100644 --- a/src/backend/postmaster/bgworker.c +++ b/src/backend/postmaster/bgworker.c @@ -716,7 +716,7 @@ bgworker_die(SIGNAL_ARGS) void BackgroundWorkerMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; BackgroundWorker *worker; bgworker_main_type entrypt; @@ -781,7 +781,7 @@ BackgroundWorkerMain(char *startup_data, size_t startup_data_len) * * We just need to clean up, report the error, and go away. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c index 0f75548759..445b56ac76 100644 --- a/src/backend/postmaster/bgwriter.c +++ b/src/backend/postmaster/bgwriter.c @@ -86,7 +86,7 @@ static XLogRecPtr last_snapshot_lsn = InvalidXLogRecPtr; void BackgroundWriterMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; MemoryContext bgwriter_context; bool prev_hibernate; WritebackContext wb_context; @@ -150,7 +150,7 @@ BackgroundWriterMain(char *startup_data, size_t startup_data_len) * call redundant, but it is not since InterruptPending might be set * already. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c index 199f008bcd..e2a2c7336e 100644 --- a/src/backend/postmaster/checkpointer.c +++ b/src/backend/postmaster/checkpointer.c @@ -172,7 +172,7 @@ static void ReqCheckpointHandler(SIGNAL_ARGS); void CheckpointerMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; MemoryContext checkpointer_context; Assert(startup_data_len == 0); @@ -248,7 +248,7 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) * call redundant, but it is not since InterruptPending might be set * already. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; @@ -311,7 +311,6 @@ CheckpointerMain(char *startup_data, size_t startup_data_len) /* We can now handle ereport(ERROR) */ PG_exception_stack = &local_sigjmp_buf; - /* * Unblock signals (they were blocked when the postmaster forked us) */ diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index 02f91431f5..3a40334bc4 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -515,7 +515,7 @@ pgarch_ArchiverCopyLoop(void) static bool pgarch_archiveXlog(char *xlog) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; MemoryContext oldcontext; char pathname[MAXPGPATH]; char activitymsg[MAXFNAMELEN + 16]; @@ -544,7 +544,7 @@ pgarch_archiveXlog(char *xlog) * PgArchiverMain() and use PG_TRY/PG_CATCH here, but the extra code to * avoid the odd archiver restart doesn't seem worth it. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c index daa7909382..da58b26aa8 100644 --- a/src/backend/postmaster/walsummarizer.c +++ b/src/backend/postmaster/walsummarizer.c @@ -210,7 +210,7 @@ WalSummarizerShmemInit(void) void WalSummarizerMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; MemoryContext context; /* @@ -273,7 +273,7 @@ WalSummarizerMain(char *startup_data, size_t startup_data_len) /* * If an exception is encountered, processing resumes here. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/postmaster/walwriter.c b/src/backend/postmaster/walwriter.c index 6e7918a78d..c18e7eb024 100644 --- a/src/backend/postmaster/walwriter.c +++ b/src/backend/postmaster/walwriter.c @@ -88,7 +88,7 @@ int WalWriterFlushAfter = DEFAULT_WAL_WRITER_FLUSH_AFTER; void WalWriterMain(char *startup_data, size_t startup_data_len) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; MemoryContext walwriter_context; int left_till_hibernate; bool hibernating; @@ -147,7 +147,7 @@ WalWriterMain(char *startup_data, size_t startup_data_len) * call redundant, but it is not since InterruptPending might be set * already. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* Since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/replication/logical/slotsync.c b/src/backend/replication/logical/slotsync.c index 2d1914ce08..c35c54edab 100644 --- a/src/backend/replication/logical/slotsync.c +++ b/src/backend/replication/logical/slotsync.c @@ -1333,7 +1333,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len) WalReceiverConn *wrconn = NULL; char *dbname; char *err; - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; StringInfoData app_name; Assert(startup_data_len == 0); @@ -1366,7 +1366,7 @@ ReplSlotSyncWorkerMain(char *startup_data, size_t startup_data_len) * operates at the bottom of the exception stack, ERRORs turn into FATALs. * Therefore, we create our own exception handler to catch ERRORs. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* since not using PG_TRY, must reset error stack by hand */ error_context_stack = NULL; diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 8bc6bea113..c2108ef72e 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -4220,7 +4220,7 @@ PostgresSingleUserMain(int argc, char *argv[], void PostgresMain(const char *dbname, const char *username) { - sigjmp_buf local_sigjmp_buf; + PG_exception local_sigjmp_buf = {PG_exception_magic}; /* these must be volatile to ensure state is preserved across longjmp: */ volatile bool send_ready_for_query = true; @@ -4422,7 +4422,7 @@ PostgresMain(const char *dbname, const char *username) * were inside a transaction. */ - if (sigsetjmp(local_sigjmp_buf, 1) != 0) + if (sigsetjmp(local_sigjmp_buf.buf, 1) != 0) { /* * NOTE: if you are tempted to add more code in this if-block, @@ -4537,7 +4537,6 @@ PostgresMain(const char *dbname, const char *username) /* We can now handle ereport(ERROR) */ PG_exception_stack = &local_sigjmp_buf; - if (!ignore_till_sync) send_ready_for_query = true; /* initially, or after error */ diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 943d8588f3..09fd4a829d 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -93,7 +93,7 @@ /* Global variables */ ErrorContextCallback *error_context_stack = NULL; -sigjmp_buf *PG_exception_stack = NULL; +PG_exception *PG_exception_stack = NULL; /* * Hook for intercepting messages before they are sent to the server log. @@ -1985,7 +1985,15 @@ pg_re_throw(void) { /* If possible, throw the error to the next outer setjmp handler */ if (PG_exception_stack != NULL) - siglongjmp(*PG_exception_stack, 1); + { +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wunused" + void *stackTop = NULL; +#pragma clang diagnostic pop + Assert((void *)PG_exception_stack > &stackTop || + PG_exception_stack->magic == PG_exception_magic); + siglongjmp(PG_exception_stack->buf, 1); + } else { /* diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 054dd2bf62..d8dc45c1ff 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -383,11 +383,11 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack; */ #define PG_TRY(...) \ do { \ - sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \ + PG_exception *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \ ErrorContextCallback *_save_context_stack##__VA_ARGS__ = error_context_stack; \ - sigjmp_buf _local_sigjmp_buf##__VA_ARGS__; \ + PG_exception _local_sigjmp_buf##__VA_ARGS__ = {PG_exception_magic}; \ bool _do_rethrow##__VA_ARGS__ = false; \ - if (sigsetjmp(_local_sigjmp_buf##__VA_ARGS__, 0) == 0) \ + if (sigsetjmp(_local_sigjmp_buf##__VA_ARGS__.buf, 0) == 0) \ { \ PG_exception_stack = &_local_sigjmp_buf##__VA_ARGS__ @@ -426,7 +426,13 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack; (pg_re_throw(), pg_unreachable()) #endif -extern PGDLLIMPORT sigjmp_buf *PG_exception_stack; +#define PG_exception_magic 0x12345678 +typedef struct PG_exception +{ + int magic; + sigjmp_buf buf; +} PG_exception; +extern PGDLLIMPORT PG_exception *PG_exception_stack; /* Stuff that error handlers might want to use */ -- 2.39.3 (Apple Git-146)