Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit7321d5c

Browse files
committed
Fix corner-case uninitialized-variable issues in plpgsql.
If an error was raised during our initial attempt to check whethera successfully-compiled expression is "simple", subsequent calls ofexec_stmt_execsql would suppose that stmt->mod_stmt was already computedwhen it had not been. This could lead to assertion failures in debugbuilds; in production builds the effect would typically be to act asif INTO STRICT had been specified even when it had not been. Of coursethat only matters if the subsequent attempt to execute the expressionsucceeds, so that the problem can only be reached by fixing a failurein some referenced, inline-able SQL function and then retrying thecalling plpgsql function in the same session.(There might be even-more-obscure ways to change the expression'sbehavior without changing the plpgsql function, but that one seemslike the only one people would be likely to hit in practice.)The most foolproof way to fix this would be to arrange forexec_prepare_plan to not set expr->plan until we've finished thesubsidiary simple-expression check. But it seems hard to do thatwithout creating reference-count leak issues. So settle for documentingthe hazard in a comment and fixing exec_stmt_execsql to test separatelyfor whether it's computed stmt->mod_stmt. (That adds a test-and-branchper execution, but hopefully that's negligible in context.) In v11 andup, also fix exec_stmt_call which had a variant of the same issue.Per bug #17113 from Alexander Lakhin. Back-patch to allsupported branches.Discussion:https://postgr.es/m/17113-077605ce00e0e7ec@postgresql.org
1 parent795a916 commit7321d5c

File tree

3 files changed

+57
-25
lines changed

3 files changed

+57
-25
lines changed

‎src/pl/plpgsql/src/pl_exec.c

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2140,31 +2140,31 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
21402140
*/
21412141
if (plan==NULL||local_plan)
21422142
{
2143-
/* Don't let SPI save the plan if it's going to be local */
2144-
exec_prepare_plan(estate,expr,0, !local_plan);
2145-
plan=expr->plan;
2146-
2147-
/*
2148-
* A CALL or DO can never be a simple expression. (If it could
2149-
* be, we'd have to worry about saving/restoring the previous
2150-
* values of the related expr fields, not just expr->plan.)
2151-
*/
2152-
Assert(!expr->expr_simple_expr);
2153-
2154-
/*
2155-
* Tell SPI to allow non-atomic execution. (The field name is a
2156-
* legacy choice.)
2157-
*/
2158-
plan->no_snapshots= true;
2159-
21602143
/*
21612144
* Force target to be recalculated whenever the plan changes, in
21622145
* case the procedure's argument list has changed.
21632146
*/
21642147
stmt->target=NULL;
21652148
cur_target=NULL;
2149+
2150+
/* Don't let SPI save the plan if it's going to be local */
2151+
exec_prepare_plan(estate,expr,0, !local_plan);
2152+
plan=expr->plan;
21662153
}
21672154

2155+
/*
2156+
* A CALL or DO can never be a simple expression. (If it could be,
2157+
* we'd have to worry about saving/restoring the previous values of
2158+
* the related expr fields, not just expr->plan.)
2159+
*/
2160+
Assert(!expr->expr_simple_expr);
2161+
2162+
/*
2163+
* Tell SPI to allow non-atomic execution. (The field name is a
2164+
* legacy choice.)
2165+
*/
2166+
plan->no_snapshots= true;
2167+
21682168
/*
21692169
* We construct a DTYPE_ROW datum representing the plpgsql variables
21702170
* associated with the procedure's output arguments. Then we can use
@@ -4058,6 +4058,22 @@ exec_eval_cleanup(PLpgSQL_execstate *estate)
40584058

40594059
/* ----------
40604060
* Generate a prepared plan
4061+
*
4062+
* CAUTION: it is possible for this function to throw an error after it has
4063+
* built a SPIPlan and saved it in expr->plan. Therefore, be wary of doing
4064+
* additional things contingent on expr->plan being NULL. That is, given
4065+
* code like
4066+
*
4067+
*if (query->plan == NULL)
4068+
*{
4069+
*// okay to put setup code here
4070+
*exec_prepare_plan(estate, query, ...);
4071+
*// NOT okay to put more logic here
4072+
*}
4073+
*
4074+
* extra steps at the end are unsafe because they will not be executed when
4075+
* re-executing the calling statement, if exec_prepare_plan failed the first
4076+
* time. This is annoyingly error-prone, but the alternatives are worse.
40614077
* ----------
40624078
*/
40634079
staticvoid
@@ -4087,15 +4103,15 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
40874103
SPI_keepplan(plan);
40884104
expr->plan=plan;
40894105

4090-
/* Check to see if it's a simple expression */
4091-
exec_simple_check_plan(estate,expr);
4092-
40934106
/*
40944107
* Mark expression as not using a read-write param. exec_assign_value has
40954108
* to take steps to override this if appropriate; that seems cleaner than
40964109
* adding parameters to all other callers.
40974110
*/
40984111
expr->rwparam=-1;
4112+
4113+
/* Check to see if it's a simple expression */
4114+
exec_simple_check_plan(estate,expr);
40994115
}
41004116

41014117

@@ -4120,10 +4136,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
41204136
* whether the statement is INSERT/UPDATE/DELETE
41214137
*/
41224138
if (expr->plan==NULL)
4139+
exec_prepare_plan(estate,expr,CURSOR_OPT_PARALLEL_OK, true);
4140+
4141+
if (!stmt->mod_stmt_set)
41234142
{
41244143
ListCell*l;
41254144

4126-
exec_prepare_plan(estate,expr,CURSOR_OPT_PARALLEL_OK, true);
41274145
stmt->mod_stmt= false;
41284146
foreach(l,SPI_plan_get_plan_sources(expr->plan))
41294147
{
@@ -4144,6 +4162,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
41444162
break;
41454163
}
41464164
}
4165+
stmt->mod_stmt_set= true;
41474166
}
41484167

41494168
/*
@@ -4912,6 +4931,14 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
49124931
* if we can pass the target variable as a read-write parameter to the
49134932
* expression. (This is a bit messy, but it seems cleaner than modifying
49144933
* the API of exec_eval_expr for the purpose.)
4934+
*
4935+
* NOTE: this coding ignores the advice given in exec_prepare_plan's
4936+
* comments, that one should not do additional setup contingent on
4937+
* expr->plan being NULL. This means that if we get an error while trying
4938+
* to check for the expression being simple, we won't check for a
4939+
* read-write parameter either, so that neither optimization will be
4940+
* applied in future executions. Nothing will fail though, so we live
4941+
* with that bit of messiness too.
49154942
*/
49164943
if (expr->plan==NULL)
49174944
{
@@ -7837,6 +7864,10 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
78377864
* exec_simple_check_plan -Check if a plan is simple enough to
78387865
*be evaluated by ExecEvalExpr() instead
78397866
*of SPI.
7867+
*
7868+
* Note: the refcount manipulations in this function assume that expr->plan
7869+
* is a "saved" SPI plan. That's a bit annoying from the caller's standpoint,
7870+
* but it's otherwise difficult to avoid leaking the plan on failure.
78407871
* ----------
78417872
*/
78427873
staticvoid
@@ -7919,7 +7950,8 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
79197950
* OK, we can treat it as a simple plan.
79207951
*
79217952
* Get the generic plan for the query. If replanning is needed, do that
7922-
* work in the eval_mcontext.
7953+
* work in the eval_mcontext. (Note that replanning could throw an error,
7954+
* in which case the expr is left marked "not simple", which is fine.)
79237955
*/
79247956
oldcontext=MemoryContextSwitchTo(get_eval_mcontext(estate));
79257957
cplan=SPI_plan_get_cached_plan(expr->plan);

‎src/pl/plpgsql/src/pl_gram.y

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3007,7 +3007,7 @@ make_execsql_stmt(int firsttoken, int location)
30073007

30083008
check_sql_expr(expr->query, location, 0);
30093009

3010-
execsql =palloc(sizeof(PLpgSQL_stmt_execsql));
3010+
execsql =palloc0(sizeof(PLpgSQL_stmt_execsql));
30113011
execsql->cmd_type = PLPGSQL_STMT_EXECSQL;
30123012
execsql->lineno = plpgsql_location_to_lineno(location);
30133013
execsql->sqlstmt = expr;

‎src/pl/plpgsql/src/plpgsql.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -873,10 +873,10 @@ typedef struct PLpgSQL_stmt_execsql
873873
PLpgSQL_stmt_typecmd_type;
874874
intlineno;
875875
PLpgSQL_expr*sqlstmt;
876-
boolmod_stmt;/* is the stmt INSERT/UPDATE/DELETE? Note:
877-
* mod_stmt is set when we plan the query */
876+
boolmod_stmt;/* is the stmt INSERT/UPDATE/DELETE? */
878877
boolinto;/* INTO supplied? */
879878
boolstrict;/* INTO STRICT flag */
879+
boolmod_stmt_set;/* is mod_stmt valid yet? */
880880
PLpgSQL_variable*target;/* INTO target (record or row) */
881881
}PLpgSQL_stmt_execsql;
882882

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp