From 6d301d938f7d7cb19f730eff44e65bc8addde68a Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 9 Aug 2010 18:50:20 +0000 Subject: [PATCH] Fix incorrect logic in plpgsql for cleanup after evaluation of non-simple expressions. We need to deal with this when handling subscripts in an array assignment, and also when catching an exception. In an Assert-enabled build these omissions led to Assert failures, but I think in a normal build the only consequence would be short-term memory leakage; which may explain why this wasn't reported from the field long ago. Back-patch to all supported versions. 7.4 doesn't have exceptions, but otherwise these bugs go all the way back. Heikki Linnakangas and Tom Lane --- src/pl/plpgsql/src/pl_exec.c | 43 ++++++++++++++++++++++++++++++++--- src/test/regress/expected/plpgsql.out | 43 +++++++++++++++++++++++++++++++++++ src/test/regress/sql/plpgsql.sql | 40 ++++++++++++++++++++++++++++++++ 3 files changed, 123 insertions(+), 3 deletions(-) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index e1f48e3d75..cce730ea9c 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -8,7 +8,7 @@ * * * IDENTIFICATION - * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.261 2010/07/06 19:19:01 momjian Exp $ + * $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.261.2.1 2010/08/09 18:50:20 tgl Exp $ * *------------------------------------------------------------------------- */ @@ -1106,6 +1106,9 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block) */ SPI_restore_connection(); + /* Must clean up the econtext too */ + exec_eval_cleanup(estate); + /* Look for a matching exception handler */ foreach(e, block->exceptions->exc_list) { @@ -2693,6 +2696,9 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate, * * NB: the result of the evaluation is no longer valid after this is done, * unless it is a pass-by-value datatype. + * + * NB: if you change this code, see also the hacks in exec_assign_value's + * PLPGSQL_DTYPE_ARRAYELEM case. * ---------- */ static void @@ -3456,6 +3462,10 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target, /* ---------- * exec_assign_value Put a value into a target field + * + * Note: in some code paths, this may leak memory in the eval_econtext; + * we assume that will be cleaned up later by exec_eval_cleanup. We cannot + * call exec_eval_cleanup here for fear of destroying the input Datum value. * ---------- */ static void @@ -3706,6 +3716,9 @@ exec_assign_value(PLpgSQL_execstate *estate, case PLPGSQL_DTYPE_ARRAYELEM: { + /* + * Target is an element of an array + */ int nsubscripts; int i; PLpgSQL_expr *subscripts[MAXDIM]; @@ -3721,10 +3734,19 @@ exec_assign_value(PLpgSQL_execstate *estate, coerced_value; ArrayType *oldarrayval; ArrayType *newarrayval; + SPITupleTable *save_eval_tuptable; + + /* + * We need to do subscript evaluation, which might require + * evaluating general expressions; and the caller might have + * done that too in order to prepare the input Datum. We + * have to save and restore the caller's SPI_execute result, + * if any. + */ + save_eval_tuptable = estate->eval_tuptable; + estate->eval_tuptable = NULL; /* - * Target is an element of an array - * * To handle constructs like x[1][2] := something, we have to * be prepared to deal with a chain of arrayelem datums. Chase * back to find the base array datum, and save the subscript @@ -3778,8 +3800,23 @@ exec_assign_value(PLpgSQL_execstate *estate, ereport(ERROR, (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED), errmsg("array subscript in assignment must not be null"))); + + /* + * Clean up in case the subscript expression wasn't simple. + * We can't do exec_eval_cleanup, but we can do this much + * (which is safe because the integer subscript value is + * surely pass-by-value), and we must do it in case the + * next subscript expression isn't simple either. + */ + if (estate->eval_tuptable != NULL) + SPI_freetuptable(estate->eval_tuptable); + estate->eval_tuptable = NULL; } + /* Now we can restore caller's SPI_execute result if any. */ + Assert(estate->eval_tuptable == NULL); + estate->eval_tuptable = save_eval_tuptable; + /* Coerce source value to match array element type. */ coerced_value = exec_simple_cast_value(value, valtype, diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index a22e2bfd0f..8d20cd9048 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -3916,6 +3916,49 @@ SELECT * FROM leaker_1(true); DROP FUNCTION leaker_1(bool); DROP FUNCTION leaker_2(bool); +-- Test for appropriate cleanup of non-simple expression evaluations +-- (bug in all versions prior to August 2010) +CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$ +DECLARE + arr text[]; + lr text; + i integer; +BEGIN + arr := array[array['foo','bar'], array['baz', 'quux']]; + lr := 'fool'; + i := 1; + -- use sub-SELECTs to make expressions non-simple + arr[(SELECT i)][(SELECT i+1)] := (SELECT lr); + RETURN arr; +END; +$$ LANGUAGE plpgsql; +SELECT nonsimple_expr_test(); + nonsimple_expr_test +------------------------- + {{foo,fool},{baz,quux}} +(1 row) + +DROP FUNCTION nonsimple_expr_test(); +CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$ +declare + i integer NOT NULL := 0; +begin + begin + i := (SELECT NULL::integer); -- should throw error + exception + WHEN OTHERS THEN + i := (SELECT 1::integer); + end; + return i; +end; +$$ LANGUAGE plpgsql; +SELECT nonsimple_expr_test(); + nonsimple_expr_test +--------------------- + 1 +(1 row) + +DROP FUNCTION nonsimple_expr_test(); -- Test handling of string literals. set standard_conforming_strings = off; create or replace function strtest() returns text as $$ diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 612e92d21b..74f95a3415 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -3125,6 +3125,46 @@ SELECT * FROM leaker_1(true); DROP FUNCTION leaker_1(bool); DROP FUNCTION leaker_2(bool); +-- Test for appropriate cleanup of non-simple expression evaluations +-- (bug in all versions prior to August 2010) + +CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$ +DECLARE + arr text[]; + lr text; + i integer; +BEGIN + arr := array[array['foo','bar'], array['baz', 'quux']]; + lr := 'fool'; + i := 1; + -- use sub-SELECTs to make expressions non-simple + arr[(SELECT i)][(SELECT i+1)] := (SELECT lr); + RETURN arr; +END; +$$ LANGUAGE plpgsql; + +SELECT nonsimple_expr_test(); + +DROP FUNCTION nonsimple_expr_test(); + +CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$ +declare + i integer NOT NULL := 0; +begin + begin + i := (SELECT NULL::integer); -- should throw error + exception + WHEN OTHERS THEN + i := (SELECT 1::integer); + end; + return i; +end; +$$ LANGUAGE plpgsql; + +SELECT nonsimple_expr_test(); + +DROP FUNCTION nonsimple_expr_test(); + -- Test handling of string literals. set standard_conforming_strings = off; -- 2.11.0