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

diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index 23545896d4dd89e039fd4e4b4a3f874bb662daf7..f63c8124e687ca7c670cd8899f0a47f98ee91391 100644 (file)
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -906,6 +906,7 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
        if (nargs > 0)
        {
                ParamListInfo paramLI;
+               Oid                *argtypes = fcache->pinfo->argtypes;
                int                     i;
 
                if (fcache->paramLI == NULL)
@@ -933,10 +934,24 @@ postquel_sub_params(SQLFunctionCachePtr fcache,
                {
                        ParamExternData *prm = &paramLI->params[i];
 
-                       prm->value = fcinfo->arg[i];
+                       /*
+                        * 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->argnull[i];
+                       prm->value = MakeExpandedObjectReadOnly(fcinfo->arg[i],
+                                                                                                       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_3.out b/src/test/regress/expected/create_function_3.out
index 3301885fc82f398c6bf12f35adee1b07ff617f3b..6482e70a812f0556fafa3790b7517b1331578d93 100644 (file)
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -340,10 +340,26 @@ 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)
+
 -- Cleanup
 \set VERBOSITY terse \\ -- suppress cascade details
 DROP SCHEMA temp_func_test CASCADE;
-NOTICE:  drop cascades to 21 other objects
+NOTICE:  drop cascades to 22 other objects
 \set VERBOSITY default
 DROP USER regress_unpriv_user;
 RESET search_path;

diff --git a/src/test/regress/sql/create_function_3.sql b/src/test/regress/sql/create_function_3.sql
index 24bb900990afe2ea045c374e34aa807b24a02a0d..a2b83ef14b9e27e3060d3d1555c7c7797b619da2 100644 (file)
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -219,6 +219,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);
+
 -- Cleanup
 \set VERBOSITY terse \\ -- suppress cascade details
 DROP SCHEMA temp_func_test CASCADE;