[HACKERS] Read/write expanded objects versus domains and CASE

I looked into the problem reported in bug #14472,
https://www.postgresql.org/message-id/20161221214744.25622.71454@wrigleys.postgresql.org
Although the submitter thought it was related to bug #14414, it isn't
particularly.  The failure scenario is that the input value to a
CoerceToDomain execution node is a read-write expanded datum.  We were
blindly passing that to any CHECK constraint expressions for the domain
type, which leaves called functions at liberty to modify or even delete
the expanded object.  Correct behavior is to pass a read-only pointer to
the CHECK expressions and then return the original read-write pointer as
the expression result.

I nosed around for other occurrences of the same problem and soon
realized that CASE with an "arg" expression has a similar issue,
since the "arg" value may get passed to multiple test expressions.
It'd be substantially harder to make a test case for that in the
current state of the code --- to get a failure, you'd need a
plpgsql function to be the equality operator for some data type ---
but it's surely not impossible.

Also, domain_check() could in principle be called with a r/w datum,
so it should also protect against this.

The fix for this is nominally simple, to call MakeExpandedObjectReadOnly
at the appropriate places; but that requires having the data type's typlen
at hand, which we don't in these places.

In the domain cases, the typlen is cheaply accessible through the domain's
typcache entry, which we could get at as long as we don't mind using
DomainConstraintRef.tcache, which had been documented as private to
typcache.c.  I don't see any particularly strong reason not to allow
callers to use it, though.

In the CASE case, there seems no help for it except to expend a
get_typlen() syscache lookup during executor setup.  It's kind of annoying
to do that to support a corner case that may very well never occur in the
field, but I don't see another alternative.

So I'm proposing the attached patch (sans test cases as yet).
Any objections?

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 743e7d6..ab09691 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** ExecEvalCase(CaseExprState *caseExpr, Ex
*** 2992,3003 ****

      if (caseExpr->arg)
      {
          bool        arg_isNull;

!         econtext->caseValue_datum = ExecEvalExpr(caseExpr->arg,
!                                                  econtext,
!                                                  &arg_isNull,
!                                                  NULL);
          econtext->caseValue_isNull = arg_isNull;
      }

--- 2992,3009 ----

      if (caseExpr->arg)
      {
+         Datum        arg_value;
          bool        arg_isNull;

!         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;
      }

*************** ExecEvalCoerceToDomain(CoerceToDomainSta
*** 4127,4137 ****
                       * 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.
                       */
                      save_datum = econtext->domainValue_datum;
                      save_isNull = econtext->domainValue_isNull;

!                     econtext->domainValue_datum = result;
                      econtext->domainValue_isNull = *isNull;

                      conResult = ExecEvalExpr(con->check_expr,
--- 4133,4150 ----
                       * 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 =
!                         MakeExpandedObjectReadOnly(result, *isNull,
!                                      cstate->constraint_ref->tcache->typlen);
                      econtext->domainValue_isNull = *isNull;

                      conResult = ExecEvalExpr(con->check_expr,
*************** ExecInitExpr(Expr *node, PlanState *pare
*** 4939,4944 ****
--- 4952,4959 ----
                  }
                  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 26bbbb5..1cd80ae 100644
*** a/src/backend/utils/adt/domains.c
--- b/src/backend/utils/adt/domains.c
***************
*** 35,40 ****
--- 35,41 ----
  #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"
*************** domain_check_input(Datum value, bool isn
*** 166,174 ****
                       * 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.
                       */
!                     econtext->domainValue_datum = value;
                      econtext->domainValue_isNull = isnull;

                      conResult = ExecEvalExprSwitchContext(con->check_expr,
--- 167,180 ----
                       * 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.  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 =
!                         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 5c3b868..cc0821b 100644
*** a/src/include/nodes/execnodes.h
--- b/src/include/nodes/execnodes.h
*************** typedef struct CaseExprState
*** 889,894 ****
--- 889,895 ----
      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 c72efcc..d187650 100644
*** a/src/include/utils/typcache.h
--- b/src/include/utils/typcache.h
*************** typedef struct DomainConstraintRef
*** 131,139 ****
  {
      List       *constraints;    /* list of DomainConstraintState nodes */
      MemoryContext refctx;        /* context holding DomainConstraintRef */

      /* 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;
--- 131,139 ----
  {
      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 */
      DomainConstraintCache *dcc; /* current constraints, or NULL if none */
      MemoryContextCallback callback;        /* used to release refcount when done */
  } DomainConstraintRef;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers