Fix memory leakage when function compilation fails.

In pl_comp.c, initially create the plpgsql function's cache context
under the assumed-short-lived caller's context, and reparent it under
CacheMemoryContext only upon success.  This avoids a process-lifespan
leak of 8kB or more if the function contains syntax errors.  (This
leakage has existed for a long time without many complaints, but as
we move towards a possibly multi-threaded future, getting rid of
process-lifespan leaks grows more important.)

In funccache.c, arrange to reclaim the CachedFunction struct in case
the language-specific compile callback function throws an error;
previously, that resulted in an independent process-lifespan leak.
This is arguably a new bug in v18, since the leakage now occurred
for SQL-language functions as well as plpgsql.

Also, don't fill fn_xmin/fn_tid/dcallback until after successful
completion of the compile callback.  This avoids a scenario where a
partially-built function cache might appear already valid upon later
inspection, and another scenario where dcallback might fail upon being
presented with an incomplete cache entry.  We would have to reach such
a faulty cache entry via a pre-existing fn_extra pointer, so I'm not
sure these scenarios correspond to any live bug.  (The predecessor
code in pl_comp.c never took any care about this, and we've heard no
complaints about that.)  Still, it's better to be careful.

Given the lack of field complaints, I'm not very excited about
back-patching any of this; but it seems still in-scope for v18.

Discussion: https://postgr.es/m/999171.1748300004@sss.pgh.pa.us

2 files changed, 41 insertions, 9 deletions

diff --git a/src/backend/utils/cache/funccache.c b/src/backend/utils/cache/funccache.c
index 150c502a612..afc048a051e 100644
--- a/src/backend/utils/cache/funccache.c
+++ b/src/backend/utils/cache/funccache.c
@@ -491,6 +491,7 @@ cached_function_compile(FunctionCallInfo fcinfo,
 	CachedFunctionHashKey hashkey;
 	bool		function_valid = false;
 	bool		hashkey_valid = false;
+	bool		new_function = false;
 
 	/*
 	 * Lookup the pg_proc tuple by Oid; we'll need it in any case
@@ -570,13 +571,15 @@ recheck:
 
 		/*
 		 * Create the new function struct, if not done already.  The function
-		 * structs are never thrown away, so keep them in TopMemoryContext.
+		 * cache entry will be kept for the life of the backend, so put it in
+		 * TopMemoryContext.
 		 */
 		Assert(cacheEntrySize >= sizeof(CachedFunction));
 		if (function == NULL)
 		{
 			function = (CachedFunction *)
 				MemoryContextAllocZero(TopMemoryContext, cacheEntrySize);
+			new_function = true;
 		}
 		else
 		{
@@ -585,17 +588,36 @@ recheck:
 		}
 
 		/*
-		 * Fill in the CachedFunction part.  fn_hashkey and use_count remain
-		 * zeroes for now.
+		 * However, if function compilation fails, we'd like not to leak the
+		 * function struct, so use a PG_TRY block to prevent that.  (It's up
+		 * to the compile callback function to avoid its own internal leakage
+		 * in such cases.)  Unfortunately, freeing the struct is only safe if
+		 * we just allocated it: otherwise there are probably fn_extra
+		 * pointers to it.
 		 */
-		function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);
-		function->fn_tid = procTup->t_self;
-		function->dcallback = dcallback;
+		PG_TRY();
+		{
+			/*
+			 * Do the hard, language-specific part.
+			 */
+			ccallback(fcinfo, procTup, &hashkey, function, forValidator);
+		}
+		PG_CATCH();
+		{
+			if (new_function)
+				pfree(function);
+			PG_RE_THROW();
+		}
+		PG_END_TRY();
 
 		/*
-		 * Do the hard, language-specific part.
+		 * Fill in the CachedFunction part.  (We do this last to prevent the
+		 * function from looking valid before it's fully built.)  fn_hashkey
+		 * will be set by cfunc_hashtable_insert; use_count remains zero.
 		 */
-		ccallback(fcinfo, procTup, &hashkey, function, forValidator);
+		function->fn_xmin = HeapTupleHeaderGetRawXmin(procTup->t_data);
+		function->fn_tid = procTup->t_self;
+		function->dcallback = dcallback;
 
 		/*
 		 * Add the completed struct to the hash table.
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index 519f7695d7c..b80c59447fb 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -226,8 +226,13 @@ plpgsql_compile_callback(FunctionCallInfo fcinfo,
 	/*
 	 * All the permanent output of compilation (e.g. parse tree) is kept in a
 	 * per-function memory context, so it can be reclaimed easily.
+	 *
+	 * While the func_cxt needs to be long-lived, we initially make it a child
+	 * of the assumed-short-lived caller's context, and reparent it under
+	 * CacheMemoryContext only upon success.  This arrangement avoids memory
+	 * leakage during compilation of a faulty function.
 	 */
-	func_cxt = AllocSetContextCreate(TopMemoryContext,
+	func_cxt = AllocSetContextCreate(CurrentMemoryContext,
 									 "PL/pgSQL function",
 									 ALLOCSET_DEFAULT_SIZES);
 	plpgsql_compile_tmp_cxt = MemoryContextSwitchTo(func_cxt);
@@ -704,6 +709,11 @@ plpgsql_compile_callback(FunctionCallInfo fcinfo,
 		plpgsql_dumptree(function);
 
 	/*
+	 * All is well, so make the func_cxt long-lived
+	 */
+	MemoryContextSetParent(func_cxt, CacheMemoryContext);
+
+	/*
 	 * Pop the error context stack
 	 */
 	error_context_stack = plerrcontext.previous;