Add defenses against putting expanded objects into Const nodes.

Putting a reference to an expanded-format value into a Const node would be
a bad idea for a couple of reasons.  It'd be possible for the supposedly
immutable Const to change value, if something modified the referenced
variable ... in fact, if the Const's reference were R/W, any function that
has the Const as argument might itself change it at runtime.  Also, because
datumIsEqual() is pretty simplistic, the Const might fail to compare equal
to other Consts that it should compare equal to, notably including copies
of itself.  This could lead to unexpected planner behavior, such as "could
not find pathkey item to sort" errors or inferior plans.

I have not been able to find any way to get an expanded value into a Const
within the existing core code; but Paul Ramsey was able to trigger the
problem by writing a datatype input function that returns an expanded
value.

The best fix seems to be to establish a rule that varlena values being
placed into Const nodes should be passed through pg_detoast_datum().
That will do nothing (and cost little) in normal cases, but it will flatten
expanded values and thereby avoid the above problems.  Also, it will
convert short-header or compressed values into canonical format, which will
avoid possible unexpected lack-of-equality issues for those cases too.
And it provides a last-ditch defense against putting a toasted value into
a Const, which we already knew was dangerous, cf commit 2b0c86b66563cf2f.
(In the light of this discussion, I'm no longer sure that that commit
provided 100% protection against such cases, but this fix should do it.)

The test added in commit 65c3d05e18e7c530 to catch datatype input functions
with unstable results would fail for functions that returned expanded
values; but it seems a bit uncharitable to deem a result unstable just
because it's expressed in expanded form, so revise the coding so that we
check for bitwise equality only after applying pg_detoast_datum().  That's
a sufficient condition anyway given the new rule about detoasting when
forming a Const.

Back-patch to 9.5 where the expanded-object facility was added.  It's
possible that this should go back further; but in the absence of clear
evidence that there's any live bug in older branches, I'll refrain for now.

diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c
index 4be89f63ae0fe03ccf2d8f5b43a9260f42cb59cf..437abad06afa9cc119822d736a093a8d5c8a6ea6 100644 (file)
--- a/src/backend/nodes/makefuncs.c
+++ b/src/backend/nodes/makefuncs.c
@@ -17,6 +17,7 @@
 
 #include "catalog/pg_class.h"
 #include "catalog/pg_type.h"
+#include "fmgr.h"
 #include "nodes/makefuncs.h"
 #include "nodes/nodeFuncs.h"
 #include "utils/lsyscache.h"
@@ -302,6 +303,14 @@ makeConst(Oid consttype,
 {
    Const      *cnst = makeNode(Const);
 
+   /*
+    * If it's a varlena value, force it to be in non-expanded (non-toasted)
+    * format; this avoids any possible dependency on external values and
+    * improves consistency of representation, which is important for equal().
+    */
+   if (!constisnull && constlen == -1)
+       constvalue = PointerGetDatum(PG_DETOAST_DATUM(constvalue));
+
    cnst->consttype = consttype;
    cnst->consttypmod = consttypmod;
    cnst->constcollid = constcollid;

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index c72dbef1c834431d8aaa18dbd71f7772e546104e..e6d83d4fd937f0cd2cddf2214b53b4da117903b8 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4655,7 +4655,8 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,
     *
     * Also, if it's varlena, forcibly detoast it.  This protects us against
     * storing TOAST pointers into plans that might outlive the referenced
-    * data.
+    * data.  (makeConst would handle detoasting anyway, but it's worth a few
+    * extra lines here so that we can do the copy and detoast in one step.)
     */
    if (!const_is_null)
    {

diff --git a/src/backend/parser/parse_coerce.c b/src/backend/parser/parse_coerce.c
index f6e7be4e71020b699f6c7721e0bb0c9bf3cfb79f..589183c277ee3878d413c9c7814320214b9bd8f8 100644 (file)
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -26,6 +26,7 @@
 #include "parser/parse_relation.h"
 #include "parser/parse_type.h"
 #include "utils/builtins.h"
+#include "utils/datum.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 #include "utils/typcache.h"
@@ -308,6 +309,43 @@ coerce_type(ParseState *pstate, Node *node,
                                                 NULL,
                                                 inputTypeMod);
 
+       /*
+        * If it's a varlena value, force it to be in non-expanded
+        * (non-toasted) format; this avoids any possible dependency on
+        * external values and improves consistency of representation.
+        */
+       if (!con->constisnull && newcon->constlen == -1)
+           newcon->constvalue =
+               PointerGetDatum(PG_DETOAST_DATUM(newcon->constvalue));
+
+#ifdef RANDOMIZE_ALLOCATED_MEMORY
+
+       /*
+        * For pass-by-reference data types, repeat the conversion to see if
+        * the input function leaves any uninitialized bytes in the result. We
+        * can only detect that reliably if RANDOMIZE_ALLOCATED_MEMORY is
+        * enabled, so we don't bother testing otherwise.  The reason we don't
+        * want any instability in the input function is that comparison of
+        * Const nodes relies on bytewise comparison of the datums, so if the
+        * input function leaves garbage then subexpressions that should be
+        * identical may not get recognized as such.  See pgsql-hackers
+        * discussion of 2008-04-04.
+        */
+       if (!con->constisnull && !newcon->constbyval)
+       {
+           Datum       val2;
+
+           val2 = stringTypeDatum(baseType,
+                                  DatumGetCString(con->constvalue),
+                                  inputTypeMod);
+           if (newcon->constlen == -1)
+               val2 = PointerGetDatum(PG_DETOAST_DATUM(val2));
+           if (!datumIsEqual(newcon->constvalue, val2, false, newcon->constlen))
+               elog(WARNING, "type %s has unstable input conversion for \"%s\"",
+                  typeTypeName(baseType), DatumGetCString(con->constvalue));
+       }
+#endif
+
        cancel_parser_errposition_callback(&pcbstate);
 
        result = (Node *) newcon;

diff --git a/src/backend/parser/parse_type.c b/src/backend/parser/parse_type.c
index 661663994eec09e94662f19772a9ceeae226e154..9db3553d80a54a58535474f4923c2fea436c3b88 100644 (file)
--- a/src/backend/parser/parse_type.c
+++ b/src/backend/parser/parse_type.c
@@ -23,7 +23,6 @@
 #include "parser/parse_type.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
-#include "utils/datum.h"
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
@@ -639,36 +638,8 @@ stringTypeDatum(Type tp, char *string, int32 atttypmod)
    Form_pg_type typform = (Form_pg_type) GETSTRUCT(tp);
    Oid         typinput = typform->typinput;
    Oid         typioparam = getTypeIOParam(tp);
-   Datum       result;
 
-   result = OidInputFunctionCall(typinput, string,
-                                 typioparam, atttypmod);
-
-#ifdef RANDOMIZE_ALLOCATED_MEMORY
-
-   /*
-    * For pass-by-reference data types, repeat the conversion to see if the
-    * input function leaves any uninitialized bytes in the result.  We can
-    * only detect that reliably if RANDOMIZE_ALLOCATED_MEMORY is enabled, so
-    * we don't bother testing otherwise.  The reason we don't want any
-    * instability in the input function is that comparison of Const nodes
-    * relies on bytewise comparison of the datums, so if the input function
-    * leaves garbage then subexpressions that should be identical may not get
-    * recognized as such.  See pgsql-hackers discussion of 2008-04-04.
-    */
-   if (string && !typform->typbyval)
-   {
-       Datum       result2;
-
-       result2 = OidInputFunctionCall(typinput, string,
-                                      typioparam, atttypmod);
-       if (!datumIsEqual(result, result2, typform->typbyval, typform->typlen))
-           elog(WARNING, "type %s has unstable input conversion for \"%s\"",
-                NameStr(typform->typname), string);
-   }
-#endif
-
-   return result;
+   return OidInputFunctionCall(typinput, string, typioparam, atttypmod);
 }
 
 /* given a typeid, return the type's typrelid (associated relation, if any) */

diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 60c1ca2c8dc1c404ba348aefd341ca01eb225ed2..c8b1f907a8f5095ce4d19ecf640a75d500bd8840 100644 (file)
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -165,6 +165,11 @@ typedef struct Var
 
 /*
  * Const
+ *
+ * Note: for varlena data types, we make a rule that a Const node's value
+ * must be in non-extended form (4-byte header, no compression or external
+ * references).  This ensures that the Const node is self-contained and makes
+ * it more likely that equal() will see logically identical values as equal.
  */
 typedef struct Const
 {