Add defenses against putting expanded objects into Const nodes.

tglsfdc · tglsfdc · commit b99551832e79 · 2016-01-21T12:56:08.000-05:00
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 2b0c86b. (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 65c3d05 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
 {

Original file line number	Diff line number	Diff line change
`@@ -4886,7 +4886,8 @@ evaluate_expr(Expr *expr, Oid result_type, int32 result_typmod,`
`4886`	`4886`	`*`
`4887`	`4887`	`* Also, if it's varlena, forcibly detoast it. This protects us against`
`4888`	`4888`	`* storing TOAST pointers into plans that might outlive the referenced`
`4889`		`- * data.`
	`4889`	`+ * data. (makeConst would handle detoasting anyway, but it's worth a few`
	`4890`	`+ * extra lines here so that we can do the copy and detoast in one step.)`
`4890`	`4891`	`*/`
`4891`	`4892`	`if (!const_is_null)`
`4892`	`4893`	`{`