From 73a3bb31b789197eb182139fa0fb6e49ce183931 Mon Sep 17 00:00:00 2001 From: Craig Ringer Date: Thu, 3 Sep 2020 13:22:02 +0800 Subject: [PATCH 2/9] Add pg_errcontext_check() to detect ErrorContextCallback escapes The new macro pg_errcontext_check() may be applied to the declaration of any ErrorContextCallback auto variable on the stack to detect failure to pop the error context stack before returning from the function. It has no effect unless PostgreSQL was built with --enable-cassert. Change ErrorContextCallback errcb; to ErrorContextCallback pg_errcontext_check(); to use it. --- src/backend/utils/error/elog.c | 35 +++++++++++++++++++++++++++ src/include/c.h | 43 ++++++++++++++++++++++++++++++++++ src/include/utils/elog.h | 14 ++++++++++- 3 files changed, 91 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index d0b368530e..f9cea36ba1 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -3548,3 +3548,38 @@ trace_recovery(int trace_level) return trace_level; } + +/* + * Check that pointers to auto variables in a function's stack are not retained + * in the error_context_stack when the function's scope ends. + * + * This is the implementation of pgl_errcontext_check() + */ +void +check_errcontext_stack_on_return(const ErrorContextCallback * const cb) +{ +#ifdef USE_ASSERT_CHECKING + /* + * No reference to the stack-allocated ErrorContextCallback that's leaving + * scope may remain on the global error_context_stack. + * + * Ideally we'd be able to report the function name, file and line of the + * ErrorContextCallback declaration, rather than this check function. But + * __attribute__((cleanup(cbfn))) does not offer any way to pass additional + * parameters. + * + * We do our best to emit a backtrace and (if enabled) a core file to help + * diagnose the problem. The pointer is still valid at this point, so we can + * safely use the error context stack to unwind. + * + * Don't attempt to walk the rest of the error_context_stack. We'll assume + * that anything below the top entry must be valid. + */ + if (error_context_stack == cb) + { + ereport(PANIC, + errmsg_internal("error_context_stack not unwound on function return"), + errbacktrace()); + } +#endif +} diff --git a/src/include/c.h b/src/include/c.h index 2c61ca8aa8..5ded7f3d16 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -178,6 +178,49 @@ #define pg_noinline #endif +/* + * pg_attribute_cleanup(cbfunc) can be used to annotate a stack (auto) variable + * with a cleanup call that's automatically executed by the compiler on any + * path where it leaves scope. + * + * This is useful for asserting that something stack-allocated got cleaned up + * even if a function has multiple returns, gotos, etc. + * + * WARNING: Do not use this for code that would execute incorrectly if the + * cleanup function is not called, because not all toolchains support it + * and it will be a silent no-op if unsupported. Instead use it to assert that + * an explicit cleanup step was not missed. + * + * The cleanup function is NOT executed before a longjmp() or siglongjmp(), so + * it will not execute on elog(ERROR) / ereport(ERROR), or as a result of a + * PG_RE_THROW(). It may be coupled with a PG_FINALLY() block if that is a + * concern. The cleanup function can then detect premature escape from the + * PG_TRY() block via an explicit return. + * + * The variant pg_attribute_cleanup_cassert(cb) is ignored unless PostgreSQL + * was built with assertions enabled. + * + * Supported at least on gcc and LLVM clang. + * + * See + * - the pg_errcontext_check() macro in elog.h + * - https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html#index-cleanup-variable-attribute + * + * Added in gcc 3.4, but that's so old we'll just require gcc4. clang claims + * to be a much newer gcc, so it'll be fine. + */ +#if __GNUC__ >= 4 +#define pg_attribute_cleanup(cb) __attribute__((cleanup(cb))) +#ifdef USE_ASSERT_CHECKING +#define pg_attribute_cleanup_cassert(cb) __attribute__((cleanup(cb))) +#else +#define pg_attribute_cleanup_cassert(cb) +#endif +#else +#define pg_attribute_cleanup(cb) +#define pg_attribute_cleanup_cassert(cb) +#endif + /* * Mark a point as unreachable in a portable fashion. This should preferably * be something that the compiler understands, to aid code generation. diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 18276e1e93..95844de850 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -256,7 +256,7 @@ extern char *format_elog_string(const char *fmt,...) pg_attribute_printf(1, 2); * void * my_func(void) * { - * ErrorContextCallback errctx; + * ErrorContextCallback errctx pgl_errcontext_check(); * struct MyFuncErrctxArg ctxarg; * * errctx.callback = my_func_errctx_callback; @@ -286,6 +286,18 @@ typedef struct ErrorContextCallback extern PGDLLIMPORT ErrorContextCallback *error_context_stack; +/*----------- + * Check for error context stack escapes on cassert builds. + * + * Apply to the ErrorContextCallback declaration (not type), i.e.: + * + * ErrorContextCallback errctx check_errcontext_stack_on_return(); + *----------- + */ +extern void check_errcontext_stack_on_return(const ErrorContextCallback * const cb); +#define pg_errcontext_check() \ + pg_attribute_cleanup_cassert(check_errcontext_stack_on_return) + /*---------- * API for catching ereport(ERROR) exits. Use these macros like so: * -- 2.26.2