Avoid premature free of pass-by-reference CALL arguments.

tglsfdc · tglsfdc · commit d02d4a6d4f27 · 2018-02-10T13:37:12.000-05:00
Prematurely freeing the EState used to evaluate CALL arguments led, in some cases, to passing dangling pointers to the procedure. This was masked in trivial cases because the argument pointers would point to Const nodes in the original expression tree, and in some other cases because the result value would end up in the standalone ExprContext rather than in memory belonging to the EState --- but that wasn't exactly high quality programming either, because the standalone ExprContext was never explicitly freed, breaking assorted API contracts. In addition, using a separate EState for each argument was just silly. So let's use just one EState, and one ExprContext, and make the latter belong to the former rather than be standalone, and clean up the EState (and hence the ExprContext) post-call. While at it, improve the function's commentary a bit. Discussion: https://postgr.es/m/29173.1518282748@sss.pgh.pa.us
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
@@ -2204,6 +2204,12 @@ ExecuteDoStmt(DoStmt *stmt, bool atomic)
  * transaction commands based on that information, e.g., call
  * SPI_connect_ext(SPI_OPT_NONATOMIC).  The language should also pass on the
  * atomic flag to any nested invocations to CALL.
+ *
+ * The expression data structures and execution context that we create
+ * within this function are children of the portalContext of the Portal
+ * that the CALL utility statement runs in.  Therefore, any pass-by-ref
+ * values that we're passing to the procedure will survive transaction
+ * commits that might occur inside the procedure.
  */
 void
 ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
@@ -2218,8 +2224,11 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	FmgrInfo	flinfo;
 	FunctionCallInfoData fcinfo;
 	CallContext *callcontext;
+	EState	   *estate;
+	ExprContext *econtext;
 	HeapTuple	tp;
 
+	/* We need to do parse analysis on the procedure call and its arguments */
 	targs = NIL;
 	foreach(lc, stmt->funccall->args)
 	{
@@ -2241,7 +2250,6 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	aclresult = pg_proc_aclcheck(fexpr->funcid, GetUserId(), ACL_EXECUTE);
 	if (aclresult != ACLCHECK_OK)
 		aclcheck_error(aclresult, OBJECT_PROCEDURE, get_func_name(fexpr->funcid));
-	InvokeFunctionExecuteHook(fexpr->funcid);
 
 	nargs = list_length(fexpr->args);
 
@@ -2254,6 +2262,7 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 							   FUNC_MAX_ARGS,
 							   FUNC_MAX_ARGS)));
 
+	/* Prep the context object we'll pass to the procedure */
 	callcontext = makeNode(CallContext);
 	callcontext->atomic = atomic;
 
@@ -2270,23 +2279,28 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 		callcontext->atomic = true;
 	ReleaseSysCache(tp);
 
+	/* Initialize function call structure */
+	InvokeFunctionExecuteHook(fexpr->funcid);
 	fmgr_info(fexpr->funcid, &flinfo);
 	InitFunctionCallInfoData(fcinfo, &flinfo, nargs, fexpr->inputcollid, (Node *) callcontext, NULL);
 
+	/*
+	 * Evaluate procedure arguments inside a suitable execution context.  Note
+	 * we can't free this context till the procedure returns.
+	 */
+	estate = CreateExecutorState();
+	econtext = CreateExprContext(estate);
+
 	i = 0;
 	foreach(lc, fexpr->args)
 	{
-		EState	   *estate;
 		ExprState  *exprstate;
-		ExprContext *econtext;
 		Datum		val;
 		bool		isnull;
 
-		estate = CreateExecutorState();
 		exprstate = ExecPrepareExpr(lfirst(lc), estate);
-		econtext = CreateStandaloneExprContext();
+
 		val = ExecEvalExprSwitchContext(exprstate, econtext, &isnull);
-		FreeExecutorState(estate);
 
 		fcinfo.arg[i] = val;
 		fcinfo.argnull[i] = isnull;
@@ -2295,4 +2309,6 @@ ExecuteCallStmt(ParseState *pstate, CallStmt *stmt, bool atomic)
 	}
 
 	FunctionCallInvoke(&fcinfo);
+
+	FreeExecutorState(estate);
 }
diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out
@@ -21,18 +21,22 @@ LINE 1: SELECT ptest1('x');
                ^
 HINT:  To call a procedure, use CALL.
 CALL ptest1('a');  -- ok
+CALL ptest1('xy' || 'zzy');  -- ok, constant-folded arg
+CALL ptest1(substring(random()::text, 1, 1));  -- ok, volatile arg
 \df ptest1
                         List of functions
  Schema |  Name  | Result data type | Argument data types | Type 
 --------+--------+------------------+---------------------+------
  public | ptest1 |                  | x text              | proc
 (1 row)
 
-SELECT * FROM cp_test ORDER BY a;
- a | b 
----+---
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
+ a |   b   
+---+-------
+ 1 | 0
  1 | a
-(1 row)
+ 1 | xyzzy
+(3 rows)
 
 CREATE PROCEDURE ptest2()
 LANGUAGE SQL
diff --git a/src/test/regress/sql/create_procedure.sql b/src/test/regress/sql/create_procedure.sql
@@ -13,10 +13,12 @@ $$;
 
 SELECT ptest1('x');  -- error
 CALL ptest1('a');  -- ok
+CALL ptest1('xy' || 'zzy');  -- ok, constant-folded arg
+CALL ptest1(substring(random()::text, 1, 1));  -- ok, volatile arg
 
 \df ptest1
 
-SELECT * FROM cp_test ORDER BY a;
+SELECT * FROM cp_test ORDER BY b COLLATE "C";
 
 
 CREATE PROCEDURE ptest2()
