Fix corner-case uninitialized-variable issues in plpgsql.

If an error was raised during our initial attempt to check whether
a successfully-compiled expression is "simple", subsequent calls of
exec_stmt_execsql would suppose that stmt->mod_stmt was already computed
when it had not been.  This could lead to assertion failures in debug
builds; in production builds the effect would typically be to act as
if INTO STRICT had been specified even when it had not been.  Of course
that only matters if the subsequent attempt to execute the expression
succeeds, so that the problem can only be reached by fixing a failure
in some referenced, inline-able SQL function and then retrying the
calling plpgsql function in the same session.

(There might be even-more-obscure ways to change the expression's
behavior without changing the plpgsql function, but that one seems
like the only one people would be likely to hit in practice.)

The most foolproof way to fix this would be to arrange for
exec_prepare_plan to not set expr->plan until we've finished the
subsidiary simple-expression check.  But it seems hard to do that
without creating reference-count leak issues.  So settle for documenting
the hazard in a comment and fixing exec_stmt_execsql to test separately
for whether it's computed stmt->mod_stmt.  (That adds a test-and-branch
per execution, but hopefully that's negligible in context.)  In v11 and
up, also fix exec_stmt_call which had a variant of the same issue.

Per bug #17113 from Alexander Lakhin.  Back-patch to all
supported branches.

Discussion: https://postgr.es/m/17113-077605ce00e0e7ec@postgresql.org

diff --git a/src/pl/plpgsql/src/expected/plpgsql_simple.out b/src/pl/plpgsql/src/expected/plpgsql_simple.out
index 5a2fefa03ee2895966cace5a11337acc67ef35fb..e1f5d670fbe5b2c0cd091223dc33a9d7980ec6b3 100644 (file)
--- a/src/pl/plpgsql/src/expected/plpgsql_simple.out
+++ b/src/pl/plpgsql/src/expected/plpgsql_simple.out
@@ -66,3 +66,26 @@ select simplecaller();
           555
 (1 row)
 
+-- Check case where first attempt to determine if it's simple fails
+create function simplesql() returns int language sql
+as $$select 1 / 0$$;
+create or replace function simplecaller() returns int language plpgsql
+as $$
+declare x int;
+begin
+  select simplesql() into x;
+  return x;
+end$$;
+select simplecaller();  -- division by zero occurs during simple-expr check
+ERROR:  division by zero
+CONTEXT:  SQL function "simplesql" during inlining
+SQL statement "select simplesql()"
+PL/pgSQL function simplecaller() line 4 at SQL statement
+create or replace function simplesql() returns int language sql
+as $$select 2 + 2$$;
+select simplecaller();
+ simplecaller 
+--------------
+            4
+(1 row)
+

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index b5c208d83d9ef5d8f6df6c47c06163a06fd1620a..14bbe12da5b532e0ca02c658d220bd9998045a44 100644 (file)
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -2162,22 +2162,20 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt)
     * Make a plan if we don't have one already.
     */
    if (expr->plan == NULL)
-   {
        exec_prepare_plan(estate, expr, 0);
 
-       /*
-        * A CALL or DO can never be a simple expression.
-        */
-       Assert(!expr->expr_simple_expr);
+   /*
+    * A CALL or DO can never be a simple expression.
+    */
+   Assert(!expr->expr_simple_expr);
 
-       /*
-        * Also construct a DTYPE_ROW datum representing the plpgsql variables
-        * associated with the procedure's output arguments.  Then we can use
-        * exec_move_row() to do the assignments.
-        */
-       if (stmt->is_call)
-           stmt->target = make_callstmt_target(estate, expr);
-   }
+   /*
+    * Also construct a DTYPE_ROW datum representing the plpgsql variables
+    * associated with the procedure's output arguments.  Then we can use
+    * exec_move_row() to do the assignments.
+    */
+   if (stmt->is_call && stmt->target == NULL)
+       stmt->target = make_callstmt_target(estate, expr);
 
    paramLI = setup_param_list(estate, expr);
 
@@ -4093,6 +4091,22 @@ exec_eval_cleanup(PLpgSQL_execstate *estate)
 
 /* ----------
  * Generate a prepared plan
+ *
+ * CAUTION: it is possible for this function to throw an error after it has
+ * built a SPIPlan and saved it in expr->plan.  Therefore, be wary of doing
+ * additional things contingent on expr->plan being NULL.  That is, given
+ * code like
+ *
+ * if (query->plan == NULL)
+ * {
+ *     // okay to put setup code here
+ *     exec_prepare_plan(estate, query, ...);
+ *     // NOT okay to put more logic here
+ * }
+ *
+ * extra steps at the end are unsafe because they will not be executed when
+ * re-executing the calling statement, if exec_prepare_plan failed the first
+ * time.  This is annoyingly error-prone, but the alternatives are worse.
  * ----------
  */
 static void
@@ -4156,10 +4170,12 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
     * whether the statement is INSERT/UPDATE/DELETE
     */
    if (expr->plan == NULL)
+       exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
+
+   if (!stmt->mod_stmt_set)
    {
        ListCell   *l;
 
-       exec_prepare_plan(estate, expr, CURSOR_OPT_PARALLEL_OK);
        stmt->mod_stmt = false;
        foreach(l, SPI_plan_get_plan_sources(expr->plan))
        {
@@ -4179,6 +4195,7 @@ exec_stmt_execsql(PLpgSQL_execstate *estate,
                break;
            }
        }
+       stmt->mod_stmt_set = true;
    }
 
    /*
@@ -7846,6 +7863,10 @@ get_cast_hashentry(PLpgSQL_execstate *estate,
  * exec_simple_check_plan -        Check if a plan is simple enough to
  *                             be evaluated by ExecEvalExpr() instead
  *                             of SPI.
+ *
+ * Note: the refcount manipulations in this function assume that expr->plan
+ * is a "saved" SPI plan.  That's a bit annoying from the caller's standpoint,
+ * but it's otherwise difficult to avoid leaking the plan on failure.
  * ----------
  */
 static void
@@ -7929,7 +7950,8 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr)
     * OK, we can treat it as a simple plan.
     *
     * Get the generic plan for the query.  If replanning is needed, do that
-    * work in the eval_mcontext.
+    * work in the eval_mcontext.  (Note that replanning could throw an error,
+    * in which case the expr is left marked "not simple", which is fine.)
     */
    oldcontext = MemoryContextSwitchTo(get_eval_mcontext(estate));
    cplan = SPI_plan_get_cached_plan(expr->plan);

diff --git a/src/pl/plpgsql/src/pl_gram.y b/src/pl/plpgsql/src/pl_gram.y
index 3fcca43b904ec5b215cc6f8449d40f5faff69e64..0f6a5b30b1ba8534ab95d791f25a0727b6461893 100644 (file)
--- a/src/pl/plpgsql/src/pl_gram.y
+++ b/src/pl/plpgsql/src/pl_gram.y
@@ -3043,7 +3043,7 @@ make_execsql_stmt(int firsttoken, int location)
 
    check_sql_expr(expr->query, expr->parseMode, location);
 
-   execsql = palloc(sizeof(PLpgSQL_stmt_execsql));
+   execsql = palloc0(sizeof(PLpgSQL_stmt_execsql));
    execsql->cmd_type = PLPGSQL_STMT_EXECSQL;
    execsql->lineno  = plpgsql_location_to_lineno(location);
    execsql->stmtid  = ++plpgsql_curr_compile->nstatements;

diff --git a/src/pl/plpgsql/src/plpgsql.h b/src/pl/plpgsql/src/plpgsql.h
index fcbfcd678b161762873d0440c64dc9d20ac7b4a1..ebd3a5d3c8a33d9726120083c0ce7e11cb3ae16f 100644 (file)
--- a/src/pl/plpgsql/src/plpgsql.h
+++ b/src/pl/plpgsql/src/plpgsql.h
@@ -893,8 +893,8 @@ typedef struct PLpgSQL_stmt_execsql
    int         lineno;
    unsigned int stmtid;
    PLpgSQL_expr *sqlstmt;
-   bool        mod_stmt;       /* is the stmt INSERT/UPDATE/DELETE?  Note:
-                                * mod_stmt is set when we plan the query */
+   bool        mod_stmt;       /* is the stmt INSERT/UPDATE/DELETE? */
+   bool        mod_stmt_set;   /* is mod_stmt valid yet? */
    bool        into;           /* INTO supplied? */
    bool        strict;         /* INTO STRICT flag */
    PLpgSQL_variable *target;   /* INTO target (record or row) */

diff --git a/src/pl/plpgsql/src/sql/plpgsql_simple.sql b/src/pl/plpgsql/src/sql/plpgsql_simple.sql
index 8a9576807348d6449ff52e0f777de9c4ad34deca..57020d22d60a9b090866314bf3bde5b60aeca376 100644 (file)
--- a/src/pl/plpgsql/src/sql/plpgsql_simple.sql
+++ b/src/pl/plpgsql/src/sql/plpgsql_simple.sql
@@ -59,3 +59,24 @@ select simplecaller();
 \c -
 
 select simplecaller();
+
+
+-- Check case where first attempt to determine if it's simple fails
+
+create function simplesql() returns int language sql
+as $$select 1 / 0$$;
+
+create or replace function simplecaller() returns int language plpgsql
+as $$
+declare x int;
+begin
+  select simplesql() into x;
+  return x;
+end$$;
+
+select simplecaller();  -- division by zero occurs during simple-expr check
+
+create or replace function simplesql() returns int language sql
+as $$select 2 + 2$$;
+
+select simplecaller();