From d02d4a6d4f27c223f48b03a5e651a22c8460b3c4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 10 Feb 2018 13:37:12 -0500 Subject: [PATCH] Avoid premature free of pass-by-reference CALL arguments. 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 --- src/backend/commands/functioncmds.c | 28 +++++++++++++++---- .../regress/expected/create_procedure.out | 12 +++++--- src/test/regress/sql/create_procedure.sql | 4 ++- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c index 5d94c1ca27..a027b19744 100644 --- 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 index ccad5c87d5..873907dc43 100644 --- a/src/test/regress/expected/create_procedure.out +++ b/src/test/regress/expected/create_procedure.out @@ -21,6 +21,8 @@ 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 @@ -28,11 +30,13 @@ CALL ptest1('a'); -- ok 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 index 8c47b7e9ef..d65e568a64 100644 --- 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() -- 2.40.0