From b99551832e79c915e4d877cf0a072120bd248748 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 21 Jan 2016 12:55:59 -0500 Subject: [PATCH] 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. --- src/backend/nodes/makefuncs.c | 9 +++++++ src/backend/optimizer/util/clauses.c | 3 ++- src/backend/parser/parse_coerce.c | 38 ++++++++++++++++++++++++++++ src/backend/parser/parse_type.c | 31 +---------------------- src/include/nodes/primnodes.h | 5 ++++ 5 files changed, 55 insertions(+), 31 deletions(-) diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index b42251c99b..f625c32df7 100644 --- 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 ace8b38b19..dff115e8dc 100644 --- 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 d63d13602f..d277fd6200 100644 --- 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 468fd4f237..a8bb4721f3 100644 --- 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 db1d4b9cfa..f942378363 100644 --- 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 {