From 77cd0dc7e05a91be5deaed7af37eb055c1f080ed Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 22 Dec 2016 15:01:27 -0500 Subject: [PATCH] Fix handling of expanded objects in CoerceToDomain and CASE execution. When the input value to a CoerceToDomain expression node is a read-write expanded datum, we should pass a read-only pointer to any domain CHECK expressions and then return the original read-write pointer as the expression result. Previously we were blindly passing the same pointer to all the consumers of the value, making it possible for a function in CHECK to modify or even delete the expanded value. (Since a plpgsql function will absorb a passed-in read-write expanded array as a local variable value, it will in fact delete the value on exit.) A similar hazard of passing the same read-write pointer to multiple consumers exists in domain_check() and in ExecEvalCase, so fix those too. The fix requires adding MakeExpandedObjectReadOnly calls at the appropriate places, which is simple enough except that we need to get the data type's typlen from somewhere. For the domain cases, solve this by redefining DomainConstraintRef.tcache as okay for callers to access; there wasn't any reason for the original convention against that, other than not wanting the API of typcache.c to be any wider than it had to be. For CASE, there's no good solution except to add a syscache lookup during executor start. Per bug #14472 from Marcos Castedo. Back-patch to 9.5 where expanded values were introduced. Discussion: https://postgr.es/m/15225.1482431619@sss.pgh.pa.us --- src/backend/executor/execQual.c | 25 +++++++++++++++++++----- src/backend/utils/adt/domains.c | 10 ++++++++-- src/include/nodes/execnodes.h | 1 + src/include/utils/typcache.h | 2 +- src/test/regress/expected/case.out | 26 +++++++++++++++++++++++++ src/test/regress/expected/plpgsql.out | 20 +++++++++++++++++++ src/test/regress/sql/case.sql | 28 +++++++++++++++++++++++++++ src/test/regress/sql/plpgsql.sql | 23 ++++++++++++++++++++++ 8 files changed, 127 insertions(+), 8 deletions(-) diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c index cbb76d1f1c..df5abd23c0 100644 --- a/src/backend/executor/execQual.c +++ b/src/backend/executor/execQual.c @@ -2987,12 +2987,18 @@ ExecEvalCase(CaseExprState *caseExpr, ExprContext *econtext, if (caseExpr->arg) { + Datum arg_value; bool arg_isNull; - econtext->caseValue_datum = ExecEvalExpr(caseExpr->arg, - econtext, - &arg_isNull, - NULL); + arg_value = ExecEvalExpr(caseExpr->arg, + econtext, + &arg_isNull, + NULL); + /* Since caseValue_datum may be read multiple times, force to R/O */ + econtext->caseValue_datum = + MakeExpandedObjectReadOnly(arg_value, + arg_isNull, + caseExpr->argtyplen); econtext->caseValue_isNull = arg_isNull; } @@ -4053,11 +4059,18 @@ ExecEvalCoerceToDomain(CoerceToDomainState *cstate, ExprContext *econtext, * nodes. We must save and restore prior setting of * econtext's domainValue fields, in case this node is * itself within a check expression for another domain. + * + * Also, if we are working with a read-write expanded + * datum, be sure that what we pass to CHECK expressions + * is a read-only pointer; else called functions might + * modify or even delete the expanded object. */ save_datum = econtext->domainValue_datum; save_isNull = econtext->domainValue_isNull; - econtext->domainValue_datum = result; + econtext->domainValue_datum = + MakeExpandedObjectReadOnly(result, *isNull, + cstate->constraint_ref->tcache->typlen); econtext->domainValue_isNull = *isNull; conResult = ExecEvalExpr(con->check_expr, @@ -4865,6 +4878,8 @@ ExecInitExpr(Expr *node, PlanState *parent) } cstate->args = outlist; cstate->defresult = ExecInitExpr(caseexpr->defresult, parent); + if (caseexpr->arg) + cstate->argtyplen = get_typlen(exprType((Node *) caseexpr->arg)); state = (ExprState *) cstate; } break; diff --git a/src/backend/utils/adt/domains.c b/src/backend/utils/adt/domains.c index 19ee4ce9d1..07f52c55d8 100644 --- a/src/backend/utils/adt/domains.c +++ b/src/backend/utils/adt/domains.c @@ -35,6 +35,7 @@ #include "executor/executor.h" #include "lib/stringinfo.h" #include "utils/builtins.h" +#include "utils/expandeddatum.h" #include "utils/lsyscache.h" #include "utils/syscache.h" #include "utils/typcache.h" @@ -157,9 +158,14 @@ domain_check_input(Datum value, bool isnull, DomainIOData *my_extra) * Set up value to be returned by CoerceToDomainValue * nodes. Unlike ExecEvalCoerceToDomain, this econtext * couldn't be shared with anything else, so no need to - * save and restore fields. + * save and restore fields. But we do need to protect the + * passed-in value against being changed by called + * functions. (It couldn't be a R/W expanded object for + * most uses, but that seems possible for domain_check().) */ - econtext->domainValue_datum = value; + econtext->domainValue_datum = + MakeExpandedObjectReadOnly(value, isnull, + my_extra->constraint_ref.tcache->typlen); econtext->domainValue_isNull = isnull; conResult = ExecEvalExprSwitchContext(con->check_expr, diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index a4ea1b901a..0f99ef16e3 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -873,6 +873,7 @@ typedef struct CaseExprState ExprState *arg; /* implicit equality comparison argument */ List *args; /* the arguments (list of WHEN clauses) */ ExprState *defresult; /* the default result (ELSE clause) */ + int16 argtyplen; /* if arg is provided, its typlen */ } CaseExprState; /* ---------------- diff --git a/src/include/utils/typcache.h b/src/include/utils/typcache.h index c72efcccb1..d187650ebe 100644 --- a/src/include/utils/typcache.h +++ b/src/include/utils/typcache.h @@ -131,9 +131,9 @@ typedef struct DomainConstraintRef { List *constraints; /* list of DomainConstraintState nodes */ MemoryContext refctx; /* context holding DomainConstraintRef */ + TypeCacheEntry *tcache; /* typcache entry for domain type */ /* Management data --- treat these fields as private to typcache.c */ - TypeCacheEntry *tcache; /* owning typcache entry */ DomainConstraintCache *dcc; /* current constraints, or NULL if none */ MemoryContextCallback callback; /* used to release refcount when done */ } DomainConstraintRef; diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out index 5f6aa16d31..09d5516fb5 100644 --- a/src/test/regress/expected/case.out +++ b/src/test/regress/expected/case.out @@ -338,6 +338,32 @@ SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo' is not foo (1 row) +ROLLBACK; +-- Test multiple evaluation of a CASE arg that is a read/write object (#14472) +-- Wrap this in a single transaction so the transient '=' operator doesn't +-- cause problems in concurrent sessions +BEGIN; +CREATE DOMAIN arrdomain AS int[]; +CREATE FUNCTION make_ad(int,int) returns arrdomain as + 'declare x arrdomain; + begin + x := array[$1,$2]; + return x; + end' language plpgsql volatile; +CREATE FUNCTION ad_eq(arrdomain, arrdomain) returns boolean as + 'begin return array_eq($1, $2); end' language plpgsql; +CREATE OPERATOR = (procedure = ad_eq, + leftarg = arrdomain, rightarg = arrdomain); +SELECT CASE make_ad(1,2) + WHEN array[2,4]::arrdomain THEN 'wrong' + WHEN array[2,5]::arrdomain THEN 'still wrong' + WHEN array[1,2]::arrdomain THEN 'right' + END; + case +------- + right +(1 row) + ROLLBACK; -- -- Clean up diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out index 147fb9f2bb..e28be3bb8b 100644 --- a/src/test/regress/expected/plpgsql.out +++ b/src/test/regress/expected/plpgsql.out @@ -5662,3 +5662,23 @@ end; $$; ERROR: value for domain plpgsql_domain violates check constraint "plpgsql_domain_check" CONTEXT: PL/pgSQL function inline_code_block line 4 at assignment +-- Test handling of expanded array passed to a domain constraint (bug #14472) +create function plpgsql_arr_domain_check(val int[]) returns boolean as $$ +begin return val[1] > 0; end +$$ language plpgsql immutable; +create domain plpgsql_arr_domain as int[] check(plpgsql_arr_domain_check(value)); +do $$ +declare v_test plpgsql_arr_domain; +begin + v_test := array[1]; + v_test := v_test || 2; +end; +$$; +do $$ +declare v_test plpgsql_arr_domain := array[1]; +begin + v_test := 0 || v_test; -- fail +end; +$$; +ERROR: value for domain plpgsql_arr_domain violates check constraint "plpgsql_arr_domain_check" +CONTEXT: PL/pgSQL function inline_code_block line 4 at assignment diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql index c860fae258..a7ae7b4a9e 100644 --- a/src/test/regress/sql/case.sql +++ b/src/test/regress/sql/case.sql @@ -200,6 +200,34 @@ SELECT CASE volfoo('bar') WHEN 'foo'::foodomain THEN 'is foo' ELSE 'is not foo' ROLLBACK; +-- Test multiple evaluation of a CASE arg that is a read/write object (#14472) +-- Wrap this in a single transaction so the transient '=' operator doesn't +-- cause problems in concurrent sessions +BEGIN; + +CREATE DOMAIN arrdomain AS int[]; + +CREATE FUNCTION make_ad(int,int) returns arrdomain as + 'declare x arrdomain; + begin + x := array[$1,$2]; + return x; + end' language plpgsql volatile; + +CREATE FUNCTION ad_eq(arrdomain, arrdomain) returns boolean as + 'begin return array_eq($1, $2); end' language plpgsql; + +CREATE OPERATOR = (procedure = ad_eq, + leftarg = arrdomain, rightarg = arrdomain); + +SELECT CASE make_ad(1,2) + WHEN array[2,4]::arrdomain THEN 'wrong' + WHEN array[2,5]::arrdomain THEN 'still wrong' + WHEN array[1,2]::arrdomain THEN 'right' + END; + +ROLLBACK; + -- -- Clean up -- diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql index 49223ff2b9..c41ad20778 100644 --- a/src/test/regress/sql/plpgsql.sql +++ b/src/test/regress/sql/plpgsql.sql @@ -4450,3 +4450,26 @@ begin v_test := 0; -- fail end; $$; + +-- Test handling of expanded array passed to a domain constraint (bug #14472) + +create function plpgsql_arr_domain_check(val int[]) returns boolean as $$ +begin return val[1] > 0; end +$$ language plpgsql immutable; + +create domain plpgsql_arr_domain as int[] check(plpgsql_arr_domain_check(value)); + +do $$ +declare v_test plpgsql_arr_domain; +begin + v_test := array[1]; + v_test := v_test || 2; +end; +$$; + +do $$ +declare v_test plpgsql_arr_domain := array[1]; +begin + v_test := 0 || v_test; -- fail +end; +$$; -- 2.40.0