Обсуждение: BUG #17053: Memory corruption in parser on prepared query reuse

The following bug has been logged on the website:

Bug reference:      17053
Logged by:          Charles Samborski
Email address:      demurgos@demurgos.net
PostgreSQL version: 13.3
Operating system:   Linux 5.12 (Arch Linux)
Description:

I found a bug in Postgres where I can reliably trigger the following error:
"unrecognized node type: X", where X can be anything and changes across
program executions. For example, I can get "unrecognized node type: 0",
"nrecognized node type: 184", "unrecognized node type: 196608" and many
others (including negative values). This implies that a node type is read
from a corrupted memory location.

The following repo has C and Rust programs exhibiting this behavior:
https://github.com/demurgos/pg_unrecognized_node.

Here is the C program:

```
#include <stdio.h>
#include <stdlib.h>
#include "libpq-fe.h"

int
main(int argc, char **argv)
{
    PGconn        *conn;
    PGresult      *res;

    conn = PQconnectdb("");

    PQexec(conn, "DROP DOMAIN IF EXISTS schema_meta");
    PQexec(conn, "CREATE TYPE raw_schema_meta AS (version int4)");
    PQprepare(conn, "q1", "CREATE DOMAIN schema_meta AS raw_schema_meta CHECK
((value).version IS NOT NULL AND (value).version >= 1)", 0, NULL);
    PQexecPrepared(conn, "q1", 0, NULL, 0, 0, 0);
    PQexec(conn, "DROP DOMAIN IF EXISTS schema_meta");
    res = PQexecPrepared(conn, "q1", 0, NULL, 0, 0, 0);

    fprintf(stdout, "%s", PQresultErrorMessage(res));

    PQfinish(conn);

    return 0;
}
```

You can compile it with `gcc -lpq -o main main.c` and run it on fresh DB by
passing the credentials through the environment, e.g.: `PGUSER=test
PGPASSWORD=test PGDATABASE=test ./main`

I investigated this issue with the help of some people from IRC and would
like to thank them: ioguix, johto and Zr40.

The code is fairly short, the core of the issue is that the prepared query
`q1` is executed twice and it somehow messes up with the parser because of
the `CHECK` clause.

PG Bug reporting form <noreply@postgresql.org> writes:
> The code is fairly short, the core of the issue is that the prepared query
> `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 = (NullTest *) expr;

                n->arg = (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

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.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 = 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 = cookDefault(pstate, constr->raw_expr,
+                    defaultExpr = cookDefault(pstate,
+                                              copyObject(constr->raw_expr),
                                               basetypeoid,
                                               basetypeMod,
                                               domainName,
@@ -2623,10 +2625,10 @@ AlterDomainDefault(List *names, Node *defaultRaw)
         pstate = 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 = cookDefault(pstate, defaultRaw,
+        defaultExpr = cookDefault(pstate, copyObject(defaultRaw),
                                   typTup->typbasetype,
                                   typTup->typtypmod,
                                   NameStr(typTup->typname),
@@ -3508,7 +3510,12 @@ domainAddConstraint(Oid domainOid, Oid domainNamespace, Oid baseTypeOid,
     pstate->p_pre_columnref_hook = replace_domain_constraint_value;
     pstate->p_ref_hook_state = (void *) domVal;

-    expr = 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 = transformExpr(pstate, copyObject(constr->raw_expr),
+                         EXPR_KIND_DOMAIN_CHECK);

     /*
      * Make sure it yields a boolean result.

I am not familiar with how Postgres tracks its bugs. How can I check the
status of this bug #17053? Is it still pending?

> 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.

Is the quick-fix already applied or do you want to wait for these
larger changes? I was hoping to drop my workarounds once the fixes for
this bug land.

Regards,
Charles Samborski

Charles Samborski <demurgos@demurgos.net> writes:
> I am not familiar with how Postgres tracks its bugs. How can I check the
> status of this bug #17053? Is it still pending?

The fix was pushed in the releases made earlier this month.

            regards, tom lane