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 b42251c99b87f89d92568c40ae5ed75935abdb18..f625c32df7aed9f39e6c09ebdf5f471c88244f68 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 ace8b38b195d239a32978ae2f5df63597e46ce22..dff115e8dc2a40960d7f68e650fc9970a5890ae3 100644 (file)
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4886,7 +4886,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 d63d13602fb989c8814d8036086fcf91f061d52e..d277fd6200d310270531d381c1f12222396d240f 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 468fd4f237e100bde0a2ee56f6473a770cee8079..a8bb4721f3048253188f5bd8ccbf4e0b15df05b4 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 db1d4b9cfa60acee718ad64f6b36e4379c49b828..f942378363489a807096170aacaae4076847822a 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
 {