Clean up memory leakage that occurs in context callback functions.

An error context callback function might leak some memory into
ErrorContext, since those functions are run with ErrorContext as
current context.  In the case where the elevel is ERROR, this is
no problem since the code level that catches the error should do
FlushErrorState to clean up, and that will reset ErrorContext.
However, if the elevel is less than ERROR then no such cleanup occurs.
In principle, repeated leaks while emitting log messages or client
notices could accumulate arbitrarily much leaked data, if no ERROR
occurs in the session.

To fix, let errfinish() perform an ErrorContext reset if it is
at the outermost error nesting level.  (If it isn't, we'll delay
cleanup until the outermost nesting level is exited.)

The only actual leakage of this sort that I've been able to observe
within our regression tests was recently introduced by commit
f727b63e8.  While it seems plausible that there are other such
leaks not reached in the regression tests, the lack of field
reports suggests that they're not a big problem.  Accordingly,
I won't take the risk of back-patching this now.  We can always
back-patch later if we get field reports of leaks.

Reported-by: Andres Freund <[email protected]>
Author: Tom Lane <[email protected]>
Discussion: https://postgr.es/m/jngsjonyfscoont4tnwi2qoikatpd5hifsg373vmmjvugwiu6g@m6opxh7uisgd

diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index b7b9692f8c88403bbb828481d6dea766592cf830..648d2d2e70c9f0b455e956450a369a4309ae798f 100644 (file)
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -542,11 +542,20 @@ errfinish(const char *filename, int lineno, const char *funcname)
    /* Emit the message to the right places */
    EmitErrorReport();
 
-   /* Now free up subsidiary data attached to stack entry, and release it */
-   FreeErrorDataContents(edata);
-   errordata_stack_depth--;
+   /*
+    * If this is the outermost recursion level, we can clean up by resetting
+    * ErrorContext altogether (compare FlushErrorState), which is good
+    * because it cleans up any random leakages that might have occurred in
+    * places such as context callback functions.  If we're nested, we can
+    * only safely remove the subsidiary data of the current stack entry.
+    */
+   if (errordata_stack_depth == 0 && recursion_depth == 1)
+       MemoryContextReset(ErrorContext);
+   else
+       FreeErrorDataContents(edata);
 
-   /* Exit error-handling context */
+   /* Release stack entry and exit error-handling context */
+   errordata_stack_depth--;
    MemoryContextSwitchTo(oldcontext);
    recursion_depth--;
 

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 107b5a273a2ede87e69326d3f55a1f5f56b6be34..8794e26ef1dc37935ebd74da808ab029a5c0c254 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -6849,7 +6849,7 @@ call_bool_check_hook(struct config_bool *conf, bool *newval, void **extra,
                 errdetail_internal("%s", GUC_check_errdetail_string) : 0,
                 GUC_check_errhint_string ?
                 errhint("%s", GUC_check_errhint_string) : 0));
-       /* Flush any strings created in ErrorContext */
+       /* Flush strings created in ErrorContext (ereport might not have) */
        FlushErrorState();
        return false;
    }
@@ -6883,7 +6883,7 @@ call_int_check_hook(struct config_int *conf, int *newval, void **extra,
                 errdetail_internal("%s", GUC_check_errdetail_string) : 0,
                 GUC_check_errhint_string ?
                 errhint("%s", GUC_check_errhint_string) : 0));
-       /* Flush any strings created in ErrorContext */
+       /* Flush strings created in ErrorContext (ereport might not have) */
        FlushErrorState();
        return false;
    }
@@ -6917,7 +6917,7 @@ call_real_check_hook(struct config_real *conf, double *newval, void **extra,
                 errdetail_internal("%s", GUC_check_errdetail_string) : 0,
                 GUC_check_errhint_string ?
                 errhint("%s", GUC_check_errhint_string) : 0));
-       /* Flush any strings created in ErrorContext */
+       /* Flush strings created in ErrorContext (ereport might not have) */
        FlushErrorState();
        return false;
    }
@@ -6960,7 +6960,7 @@ call_string_check_hook(struct config_string *conf, char **newval, void **extra,
                     errdetail_internal("%s", GUC_check_errdetail_string) : 0,
                     GUC_check_errhint_string ?
                     errhint("%s", GUC_check_errhint_string) : 0));
-           /* Flush any strings created in ErrorContext */
+           /* Flush strings created in ErrorContext (ereport might not have) */
            FlushErrorState();
            result = false;
        }
@@ -7002,7 +7002,7 @@ call_enum_check_hook(struct config_enum *conf, int *newval, void **extra,
                 errdetail_internal("%s", GUC_check_errdetail_string) : 0,
                 GUC_check_errhint_string ?
                 errhint("%s", GUC_check_errhint_string) : 0));
-       /* Flush any strings created in ErrorContext */
+       /* Flush strings created in ErrorContext (ereport might not have) */
        FlushErrorState();
        return false;
    }