Skip to content

Commit 6d301d9

Browse files
tglsfdctglsfdc
committed
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
1 parent 6d8ae3f commit 6d301d9

File tree

3 files changed

+123
-3
lines changed

3 files changed

+123
-3
lines changed

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

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.261 2010/07/06 19:19:01 momjian Exp $
11+
* $PostgreSQL: pgsql/src/pl/plpgsql/src/pl_exec.c,v 1.261.2.1 2010/08/09 18:50:20 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1106,6 +1106,9 @@ exec_stmt_block(PLpgSQL_execstate *estate, PLpgSQL_stmt_block *block)
11061106
*/
11071107
SPI_restore_connection();
11081108

1109+
/* Must clean up the econtext too */
1110+
exec_eval_cleanup(estate);
1111+
11091112
/* Look for a matching exception handler */
11101113
foreach(e, block->exceptions->exc_list)
11111114
{
@@ -2693,6 +2696,9 @@ plpgsql_estate_setup(PLpgSQL_execstate *estate,
26932696
*
26942697
* NB: the result of the evaluation is no longer valid after this is done,
26952698
* unless it is a pass-by-value datatype.
2699+
*
2700+
* NB: if you change this code, see also the hacks in exec_assign_value's
2701+
* PLPGSQL_DTYPE_ARRAYELEM case.
26962702
* ----------
26972703
*/
26982704
static void
@@ -3456,6 +3462,10 @@ exec_assign_expr(PLpgSQL_execstate *estate, PLpgSQL_datum *target,
34563462

34573463
/* ----------
34583464
* exec_assign_value Put a value into a target field
3465+
*
3466+
* Note: in some code paths, this may leak memory in the eval_econtext;
3467+
* we assume that will be cleaned up later by exec_eval_cleanup. We cannot
3468+
* call exec_eval_cleanup here for fear of destroying the input Datum value.
34593469
* ----------
34603470
*/
34613471
static void
@@ -3706,6 +3716,9 @@ exec_assign_value(PLpgSQL_execstate *estate,
37063716

37073717
case PLPGSQL_DTYPE_ARRAYELEM:
37083718
{
3719+
/*
3720+
* Target is an element of an array
3721+
*/
37093722
int nsubscripts;
37103723
int i;
37113724
PLpgSQL_expr *subscripts[MAXDIM];
@@ -3721,10 +3734,19 @@ exec_assign_value(PLpgSQL_execstate *estate,
37213734
coerced_value;
37223735
ArrayType *oldarrayval;
37233736
ArrayType *newarrayval;
3737+
SPITupleTable *save_eval_tuptable;
3738+
3739+
/*
3740+
* We need to do subscript evaluation, which might require
3741+
* evaluating general expressions; and the caller might have
3742+
* done that too in order to prepare the input Datum. We
3743+
* have to save and restore the caller's SPI_execute result,
3744+
* if any.
3745+
*/
3746+
save_eval_tuptable = estate->eval_tuptable;
3747+
estate->eval_tuptable = NULL;
37243748

37253749
/*
3726-
* Target is an element of an array
3727-
*
37283750
* To handle constructs like x[1][2] := something, we have to
37293751
* be prepared to deal with a chain of arrayelem datums. Chase
37303752
* back to find the base array datum, and save the subscript
@@ -3778,8 +3800,23 @@ exec_assign_value(PLpgSQL_execstate *estate,
37783800
ereport(ERROR,
37793801
(errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
37803802
errmsg("array subscript in assignment must not be null")));
3803+
3804+
/*
3805+
* Clean up in case the subscript expression wasn't simple.
3806+
* We can't do exec_eval_cleanup, but we can do this much
3807+
* (which is safe because the integer subscript value is
3808+
* surely pass-by-value), and we must do it in case the
3809+
* next subscript expression isn't simple either.
3810+
*/
3811+
if (estate->eval_tuptable != NULL)
3812+
SPI_freetuptable(estate->eval_tuptable);
3813+
estate->eval_tuptable = NULL;
37813814
}
37823815

3816+
/* Now we can restore caller's SPI_execute result if any. */
3817+
Assert(estate->eval_tuptable == NULL);
3818+
estate->eval_tuptable = save_eval_tuptable;
3819+
37833820
/* Coerce source value to match array element type. */
37843821
coerced_value = exec_simple_cast_value(value,
37853822
valtype,

‎src/test/regress/expected/plpgsql.out

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3916,6 +3916,49 @@ SELECT * FROM leaker_1(true);
39163916

39173917
DROP FUNCTION leaker_1(bool);
39183918
DROP FUNCTION leaker_2(bool);
3919+
-- Test for appropriate cleanup of non-simple expression evaluations
3920+
-- (bug in all versions prior to August 2010)
3921+
CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$
3922+
DECLARE
3923+
arr text[];
3924+
lr text;
3925+
i integer;
3926+
BEGIN
3927+
arr := array[array['foo','bar'], array['baz', 'quux']];
3928+
lr := 'fool';
3929+
i := 1;
3930+
-- use sub-SELECTs to make expressions non-simple
3931+
arr[(SELECT i)][(SELECT i+1)] := (SELECT lr);
3932+
RETURN arr;
3933+
END;
3934+
$$ LANGUAGE plpgsql;
3935+
SELECT nonsimple_expr_test();
3936+
nonsimple_expr_test
3937+
-------------------------
3938+
{{foo,fool},{baz,quux}}
3939+
(1 row)
3940+
3941+
DROP FUNCTION nonsimple_expr_test();
3942+
CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$
3943+
declare
3944+
i integer NOT NULL := 0;
3945+
begin
3946+
begin
3947+
i := (SELECT NULL::integer); -- should throw error
3948+
exception
3949+
WHEN OTHERS THEN
3950+
i := (SELECT 1::integer);
3951+
end;
3952+
return i;
3953+
end;
3954+
$$ LANGUAGE plpgsql;
3955+
SELECT nonsimple_expr_test();
3956+
nonsimple_expr_test
3957+
---------------------
3958+
1
3959+
(1 row)
3960+
3961+
DROP FUNCTION nonsimple_expr_test();
39193962
-- Test handling of string literals.
39203963
set standard_conforming_strings = off;
39213964
create or replace function strtest() returns text as $$

‎src/test/regress/sql/plpgsql.sql

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3125,6 +3125,46 @@ SELECT * FROM leaker_1(true);
31253125
DROP FUNCTION leaker_1(bool);
31263126
DROP FUNCTION leaker_2(bool);
31273127

3128+
-- Test for appropriate cleanup of non-simple expression evaluations
3129+
-- (bug in all versions prior to August 2010)
3130+
3131+
CREATE FUNCTION nonsimple_expr_test() RETURNS text[] AS $$
3132+
DECLARE
3133+
arr text[];
3134+
lr text;
3135+
i integer;
3136+
BEGIN
3137+
arr := array[array['foo','bar'], array['baz', 'quux']];
3138+
lr := 'fool';
3139+
i := 1;
3140+
-- use sub-SELECTs to make expressions non-simple
3141+
arr[(SELECT i)][(SELECT i+1)] := (SELECT lr);
3142+
RETURN arr;
3143+
END;
3144+
$$ LANGUAGE plpgsql;
3145+
3146+
SELECT nonsimple_expr_test();
3147+
3148+
DROP FUNCTION nonsimple_expr_test();
3149+
3150+
CREATE FUNCTION nonsimple_expr_test() RETURNS integer AS $$
3151+
declare
3152+
i integer NOT NULL := 0;
3153+
begin
3154+
begin
3155+
i := (SELECT NULL::integer); -- should throw error
3156+
exception
3157+
WHEN OTHERS THEN
3158+
i := (SELECT 1::integer);
3159+
end;
3160+
return i;
3161+
end;
3162+
$$ LANGUAGE plpgsql;
3163+
3164+
SELECT nonsimple_expr_test();
3165+
3166+
DROP FUNCTION nonsimple_expr_test();
3167+
31283168
-- Test handling of string literals.
31293169

31303170
set standard_conforming_strings = off;

0 commit comments

Comments
 (0)