From ad3e07c156544f31d02e55e95ba7954ea87a2ee8 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 10 Aug 2022 13:37:25 -0400 Subject: [PATCH] Fix handling of R/W expanded datums that are passed to SQL functions. fmgr_sql must make expanded-datum arguments read-only, because it's possible that the function body will pass the argument to more than one callee function. If one of those functions takes the datum's R/W property as license to scribble on it, then later callees will see an unexpected value, leading to wrong answers. From a performance standpoint, it'd be nice to skip this in the common case that the argument value is passed to only one callee. However, detecting that seems fairly hard, and certainly not something that I care to attempt in a back-patched bug fix. Per report from Adam Mackler. This has been broken since we invented expanded datums, so back-patch to all supported branches. Discussion: https://postgr.es/m/WScDU5qfoZ7PB2gXwNqwGGgDPmWzz08VdydcPFLhOwUKZcdWbblbo-0Lku-qhuEiZoXJ82jpiQU4hOjOcrevYEDeoAvz6nR0IU4IHhXnaCA=@mackler.email Discussion: https://postgr.es/m/187436.1660143060@sss.pgh.pa.us --- src/backend/executor/functions.c | 19 +++++++++++++++++-- .../regress/expected/create_function_sql.out | 19 ++++++++++++++++++- src/test/regress/sql/create_function_sql.sql | 13 +++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c index f9460ae506c..d856a325e82 100644 --- a/src/backend/executor/functions.c +++ b/src/backend/executor/functions.c @@ -941,6 +941,7 @@ postquel_sub_params(SQLFunctionCachePtr fcache, if (nargs > 0) { ParamListInfo paramLI; + Oid *argtypes = fcache->pinfo->argtypes; if (fcache->paramLI == NULL) { @@ -957,10 +958,24 @@ postquel_sub_params(SQLFunctionCachePtr fcache, { ParamExternData *prm = ¶mLI->params[i]; - prm->value = fcinfo->args[i].value; + /* + * If an incoming parameter value is a R/W expanded datum, we + * force it to R/O. We'd be perfectly entitled to scribble on it, + * but the problem is that if the parameter is referenced more + * than once in the function, earlier references might mutate the + * value seen by later references, which won't do at all. We + * could do better if we could be sure of the number of Param + * nodes in the function's plans; but we might not have planned + * all the statements yet, nor do we have plan tree walker + * infrastructure. (Examining the parse trees is not good enough, + * because of possible function inlining during planning.) + */ prm->isnull = fcinfo->args[i].isnull; + prm->value = MakeExpandedObjectReadOnly(fcinfo->args[i].value, + prm->isnull, + get_typlen(argtypes[i])); prm->pflags = 0; - prm->ptype = fcache->pinfo->argtypes[i]; + prm->ptype = argtypes[i]; } } else diff --git a/src/test/regress/expected/create_function_sql.out b/src/test/regress/expected/create_function_sql.out index a31daffbf34..50aca5940ff 100644 --- a/src/test/regress/expected/create_function_sql.out +++ b/src/test/regress/expected/create_function_sql.out @@ -666,6 +666,22 @@ SELECT * FROM voidtest5(3); ----------- (0 rows) +-- Regression tests for bugs: +-- Check that arguments that are R/W expanded datums aren't corrupted by +-- multiple uses. This test knows that array_append() returns a R/W datum +-- and will modify a R/W array input in-place. We use SETOF to prevent +-- inlining of the SQL function. +CREATE FUNCTION double_append(anyarray, anyelement) RETURNS SETOF anyarray +LANGUAGE SQL IMMUTABLE AS +$$ SELECT array_append($1, $2) || array_append($1, $2) $$; +SELECT double_append(array_append(ARRAY[q1], q2), q3) + FROM (VALUES(1,2,3), (4,5,6)) v(q1,q2,q3); + double_append +--------------- + {1,2,3,1,2,3} + {4,5,6,4,5,6} +(2 rows) + -- Things that shouldn't work: CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL AS 'SELECT ''not an integer'';'; @@ -692,7 +708,7 @@ CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL ERROR: only one AS item needed for language "sql" -- Cleanup DROP SCHEMA temp_func_test CASCADE; -NOTICE: drop cascades to 29 other objects +NOTICE: drop cascades to 30 other objects DETAIL: drop cascades to function functest_a_1(text,date) drop cascades to function functest_a_2(text[]) drop cascades to function functest_a_3() @@ -722,5 +738,6 @@ drop cascades to function voidtest2(integer,integer) drop cascades to function voidtest3(integer) drop cascades to function voidtest4(integer) drop cascades to function voidtest5(integer) +drop cascades to function double_append(anyarray,anyelement) DROP USER regress_unpriv_user; RESET search_path; diff --git a/src/test/regress/sql/create_function_sql.sql b/src/test/regress/sql/create_function_sql.sql index cc0ccd8db12..89e9af3a499 100644 --- a/src/test/regress/sql/create_function_sql.sql +++ b/src/test/regress/sql/create_function_sql.sql @@ -385,6 +385,19 @@ CREATE FUNCTION voidtest5(a int) RETURNS SETOF VOID LANGUAGE SQL AS $$ SELECT generate_series(1, a) $$ STABLE; SELECT * FROM voidtest5(3); +-- Regression tests for bugs: + +-- Check that arguments that are R/W expanded datums aren't corrupted by +-- multiple uses. This test knows that array_append() returns a R/W datum +-- and will modify a R/W array input in-place. We use SETOF to prevent +-- inlining of the SQL function. +CREATE FUNCTION double_append(anyarray, anyelement) RETURNS SETOF anyarray +LANGUAGE SQL IMMUTABLE AS +$$ SELECT array_append($1, $2) || array_append($1, $2) $$; + +SELECT double_append(array_append(ARRAY[q1], q2), q3) + FROM (VALUES(1,2,3), (4,5,6)) v(q1,q2,q3); + -- Things that shouldn't work: CREATE FUNCTION test1 (int) RETURNS int LANGUAGE SQL