From: Tom Lane <tgl@sss.pgh.pa.us>
To: demurgos@demurgos.net
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17053: Memory corruption in parser on prepared query reuse
In-reply-to: <17053-3ca3f501bbc212b4@postgresql.org>
References: <17053-3ca3f501bbc212b4@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Wed, 09 Jun 2021 20:13:17 -0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1509459.1623274501.0@sss.pgh.pa.us>
Date: Wed, 09 Jun 2021 17:41:36 -0400
Message-ID: <1509789.1623274896@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1509789.1623274896%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1509459.1623274501.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

PG Bug reporting form <noreply@postgresql.org> writes:
> The code is fairly short, the core of the issue is that the prepared que=
ry
> `q1` is executed twice and it somehow messes up with the parser because =
of
> the `CHECK` clause.

Thanks for the report!  The core of the problem is that where
transformExpr does this:

        case T_NullTest:
            {
                NullTest   *n =3D (NullTest *) expr;

                n->arg =3D (Expr *) transformExprRecurse(pstate, (Node *) =
n->arg);

in the case of a prepared statement, it's scribbling on the same NullTest
node that's in the plan cache.  So the next time we try to execute that
cache entry, the arg pointer is now pointing at garbage, or at least not
at the untransformed argument it should be pointing at.

Ideally, maybe, the parser wouldn't ever modify its input.  However,
this code fragment has got LOTS of company, so making that happen has
never seemed too practical.  So what we generally do, before invoking
the parser (or planner), is a quick copyObject() on any node tree that
might be coming from cache.

The proximate cause of this problem is that CREATE/ALTER DOMAIN did
not get that memo.  Looking around in typecmds.c, I noted that
CREATE DOMAIN ... DEFAULT ... and ALTER DOMAIN SET DEFAULT have the
same kind of bug.

The attached patch seems to fix it.  I wonder though if we ought to
move the copying to someplace more centralized.  I cannot escape the
suspicion that there are more of these creepy-crawlies in other
poorly-tested utility statements.

BTW, a note for future testing: you can set up this failure using
pgbench, no custom client code required.  I tested stuff like this:

$ cat bug17053b.sql
DROP DOMAIN IF EXISTS schema_meta;
CREATE DOMAIN schema_meta AS bool DEFAULT (42 IS NOT NULL);
$ pgbench -n -M prepared -f bug17053b.sql regression
NOTICE:  type "schema_meta" does not exist, skipping
pgbench: error: client 0 script 0 aborted in command 1 query 0: ERROR:  unrecognized node type: 2139062143

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="copy-raw-expressions-in-domain-DDL.patch";
	charset="us-ascii"
Content-ID: <1509459.1623274501.2@sss.pgh.pa.us>
Content-Description: copy-raw-expressions-in-domain-DDL.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecm=
ds.c
index 58ec65c6af..9a1b261d85 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -891,10 +891,12 @@ DefineDomain(CreateDomainStmt *stmt)
 					pstate =3D make_parsestate(NULL);
 =

 					/*
-					 * Cook the constr->raw_expr into an expression. Note:
-					 * name is strictly for error message
+					 * Cook the constr->raw_expr into an expression; copy it
+					 * in case the input is in plan cache.  Note: name is used
+					 * only for error messages.
 					 */
-					defaultExpr =3D cookDefault(pstate, constr->raw_expr,
+					defaultExpr =3D cookDefault(pstate,
+											  copyObject(constr->raw_expr),
 											  basetypeoid,
 											  basetypeMod,
 											  domainName,
@@ -2623,10 +2625,10 @@ AlterDomainDefault(List *names, Node *defaultRaw)
 		pstate =3D make_parsestate(NULL);
 =

 		/*
-		 * Cook the colDef->raw_expr into an expression. Note: Name is
-		 * strictly for error message
+		 * Cook the raw default into an expression; copy it in case the input
+		 * is in plan cache.  Note: name is used only for error messages.
 		 */
-		defaultExpr =3D cookDefault(pstate, defaultRaw,
+		defaultExpr =3D cookDefault(pstate, copyObject(defaultRaw),
 								  typTup->typbasetype,
 								  typTup->typtypmod,
 								  NameStr(typTup->typname),
@@ -3508,7 +3510,12 @@ domainAddConstraint(Oid domainOid, Oid domainNamesp=
ace, Oid baseTypeOid,
 	pstate->p_pre_columnref_hook =3D replace_domain_constraint_value;
 	pstate->p_ref_hook_state =3D (void *) domVal;
 =

-	expr =3D transformExpr(pstate, constr->raw_expr, EXPR_KIND_DOMAIN_CHECK)=
;
+	/*
+	 * Transform the expression; first we must copy the input, in case it's =
in
+	 * plan cache.
+	 */
+	expr =3D transformExpr(pstate, copyObject(constr->raw_expr),
+						 EXPR_KIND_DOMAIN_CHECK);
 =

 	/*
 	 * Make sure it yields a boolean result.

------- =_aaaaaaaaaa0--