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

Commit0fce76b

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 parentfb2b860 commit0fce76b

File tree

5 files changed

+101
-25
lines changed

5 files changed

+101
-25
lines changed

‎src/pl/plpgsql/src/expected/plpgsql_simple.out

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,3 +66,26 @@ select simplecaller();
6666
555
6767
(1 row)
6868

69+
-- Check case where first attempt to determine if it's simple fails
70+
create function simplesql() returns int language sql
71+
as $$select 1 / 0$$;
72+
create or replace function simplecaller() returns int language plpgsql
73+
as $$
74+
declare x int;
75+
begin
76+
select simplesql() into x;
77+
return x;
78+
end$$;
79+
select simplecaller(); -- division by zero occurs during simple-expr check
80+
ERROR: division by zero
81+
CONTEXT: SQL function "simplesql" during inlining
82+
SQL statement "select simplesql()"
83+
PL/pgSQL function simplecaller() line 4 at SQL statement
84+
create or replace function simplesql() returns int language sql
85+
as $$select 2 + 2$$;
86+
select simplecaller();
87+
simplecaller
88+
--------------
89+
4
90+
(1 row)
91+

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

Lines changed: 54 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2172,31 +2172,31 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
21722172
*/
21732173
if (plan==NULL||local_plan)
21742174
{
2175-
/* Don't let SPI save the plan if it's going to be local */
2176-
exec_prepare_plan(estate,expr,0, !local_plan);
2177-
plan=expr->plan;
2178-
2179-
/*
2180-
* A CALL or DO can never be a simple expression. (If it could
2181-
* be, we'd have to worry about saving/restoring the previous
2182-
* values of the related expr fields, not just expr->plan.)
2183-
*/
2184-
Assert(!expr->expr_simple_expr);
2185-
2186-
/*
2187-
* Tell SPI to allow non-atomic execution. (The field name is a
2188-
* legacy choice.)
2189-
*/
2190-
plan->no_snapshots= true;
2191-
21922175
/*
21932176
* Force target to be recalculated whenever the plan changes, in
21942177
* case the procedure's argument list has changed.
21952178
*/
21962179
stmt->target=NULL;
21972180
cur_target=NULL;
2181+
2182+
/* Don't let SPI save the plan if it's going to be local */
2183+
exec_prepare_plan(estate,expr,0, !local_plan);
2184+
plan=expr->plan;
21982185
}
21992186

2187+
/*
2188+
* A CALL or DO can never be a simple expression. (If it could be,
2189+
* we'd have to worry about saving/restoring the previous values of
2190+
* the related expr fields, not just expr->plan.)
2191+
*/
2192+
Assert(!expr->expr_simple_expr);
2193+
2194+
/*
2195+
* Tell SPI to allow non-atomic execution. (The field name is a
2196+
* legacy choice.)
2197+
*/
2198+
plan->no_snapshots= true;
2199+
22002200
/*
22012201
* We construct a DTYPE_ROW datum representing the plpgsql variables
22022202
* associated with the procedure's output arguments. Then we can use
@@ -4089,6 +4089,22 @@ exec_eval_cleanup(PLpgSQL_execstate *estate)
40894089

40904090
/* ----------
40914091
* Generate a prepared plan
4092+
*
4093+
* CAUTION: it is possible for this function to throw an error after it has
4094+
* built a SPIPlan and saved it in expr->plan. Therefore, be wary of doing
4095+
* additional things contingent on expr->plan being NULL. That is, given
4096+
* code like
4097+
*
4098+
*if (query->plan == NULL)
4099+
*{
4100+
*// okay to put setup code here
4101+
*exec_prepare_plan(estate, query, ...);
4102+
*// NOT okay to put more logic here
4103+
*}
4104+
*
4105+
* extra steps at the end are unsafe because they will not be executed when
4106+
* re-executing the calling statement, if exec_prepare_plan failed the first
4107+
* time. This is annoyingly error-prone, but the alternatives are worse.
40924108
* ----------
40934109
*/
40944110
staticvoid
@@ -4118,15 +4134,15 @@ exec_prepare_plan(PLpgSQL_execstate *estate,
41184134
SPI_keepplan(plan);
41194135
expr->plan=plan;
41204136

4121-
/* Check to see if it's a simple expression */
4122-
exec_simple_check_plan(estate,expr);
4123-
41244137
/*
41254138
* Mark expression as not using a read-write param. exec_assign_value has
41264139
* to take steps to override this if appropriate; that seems cleaner than
41274140
* adding parameters to all other callers.
41284141
*/
41294142
expr->rwparam=-1;
4143+
4144+
/* Check to see if it's a simple expression */
4145+
exec_simple_check_plan(estate,expr);
41304146
}
41314147

41324148

@@ -4157,10 +4173,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
41574173
* whether the statement is INSERT/UPDATE/DELETE
41584174
*/
41594175
if (expr->plan==NULL)
4176+
exec_prepare_plan(estate,expr,CURSOR_OPT_PARALLEL_OK, true);
4177+
4178+
if (!stmt->mod_stmt_set)
41604179
{
41614180
ListCell*l;
41624181

4163-
exec_prepare_plan(estate,expr,CURSOR_OPT_PARALLEL_OK, true);
41644182
stmt->mod_stmt= false;
41654183
foreach(l,SPI_plan_get_plan_sources(expr->plan))
41664184
{
@@ -4180,6 +4198,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
41804198
break;
41814199
}
41824200
}
4201+
stmt->mod_stmt_set= true;
41834202
}
41844203

41854204
/*
@@ -4963,6 +4982,14 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
49634982
* if we can pass the target variable as a read-write parameter to the
49644983
* expression. (This is a bit messy, but it seems cleaner than modifying
49654984
* the API of exec_eval_expr for the purpose.)
4985+
*
4986+
* NOTE: this coding ignores the advice given in exec_prepare_plan's
4987+
* comments, that one should not do additional setup contingent on
4988+
* expr->plan being NULL. This means that if we get an error while trying
4989+
* to check for the expression being simple, we won't check for a
4990+
* read-write parameter either, so that neither optimization will be
4991+
* applied in future executions. Nothing will fail though, so we live
4992+
* with that bit of messiness too.
49664993
*/
49674994
if (expr->plan==NULL)
49684995
{
@@ -8047,6 +8074,10 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
80478074
* exec_simple_check_plan -Check if a plan is simple enough to
80488075
*be evaluated by ExecEvalExpr() instead
80498076
*of SPI.
8077+
*
8078+
* Note: the refcount manipulations in this function assume that expr->plan
8079+
* is a "saved" SPI plan. That's a bit annoying from the caller's standpoint,
8080+
* but it's otherwise difficult to avoid leaking the plan on failure.
80508081
* ----------
80518082
*/
80528083
staticvoid
@@ -8129,7 +8160,8 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
81298160
* OK, we can treat it as a simple plan.
81308161
*
81318162
* Get the generic plan for the query. If replanning is needed, do that
8132-
* work in the eval_mcontext.
8163+
* work in the eval_mcontext. (Note that replanning could throw an error,
8164+
* in which case the expr is left marked "not simple", which is fine.)
81338165
*/
81348166
oldcontext=MemoryContextSwitchTo(get_eval_mcontext(estate));
81358167
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
@@ -3040,7 +3040,7 @@ make_execsql_stmt(int firsttoken, int location)
30403040

30413041
check_sql_expr(expr->query, location, 0);
30423042

3043-
execsql =palloc(sizeof(PLpgSQL_stmt_execsql));
3043+
execsql =palloc0(sizeof(PLpgSQL_stmt_execsql));
30443044
execsql->cmd_type = PLPGSQL_STMT_EXECSQL;
30453045
execsql->lineno = plpgsql_location_to_lineno(location);
30463046
execsql->stmtid = ++plpgsql_curr_compile->nstatements;

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -919,10 +919,10 @@ typedef struct PLpgSQL_stmt_execsql
919919
intlineno;
920920
unsignedintstmtid;
921921
PLpgSQL_expr*sqlstmt;
922-
boolmod_stmt;/* is the stmt INSERT/UPDATE/DELETE? Note:
923-
* mod_stmt is set when we plan the query */
922+
boolmod_stmt;/* is the stmt INSERT/UPDATE/DELETE? */
924923
boolinto;/* INTO supplied? */
925924
boolstrict;/* INTO STRICT flag */
925+
boolmod_stmt_set;/* is mod_stmt valid yet? */
926926
PLpgSQL_variable*target;/* INTO target (record or row) */
927927
}PLpgSQL_stmt_execsql;
928928

‎src/pl/plpgsql/src/sql/plpgsql_simple.sql

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,24 @@ select simplecaller();
5959
\c-
6060

6161
select simplecaller();
62+
63+
64+
-- Check case where first attempt to determine if it's simple fails
65+
66+
createfunctionsimplesql() returnsint language sql
67+
as $$select1/0$$;
68+
69+
create or replacefunctionsimplecaller() returnsint language plpgsql
70+
as $$
71+
declare xint;
72+
begin
73+
select simplesql() into x;
74+
return x;
75+
end$$;
76+
77+
select simplecaller();-- division by zero occurs during simple-expr check
78+
79+
create or replacefunctionsimplesql() returnsint language sql
80+
as $$select2+2$$;
81+
82+
select simplecaller();

0 commit comments

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