Re: BUG #18722: Processing arrays with plpgsql raises errors - Mailing list pgsql-bugs

PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE FUNCTION make_ia() RETURNS int[] LANGUAGE plpgsql AS
>   'declare x int[]; begin x := array[0]; return x; end';
> CREATE FUNCTION ia_eq(int[], int[]) RETURNS boolean LANGUAGE plpgsql AS
> 'begin return array_eq($1, $2); end'; 
> CREATE OPERATOR = (procedure = ia_eq, leftarg = int[], rightarg = int[]);
> SELECT NULLIF(make_ia(), array[1]::int[]);

> fails with:
> ERROR:  cache lookup failed for type 2139062143

Nice catch!  What is happening here is that make_ia returns a
read/write pointer to an expanded array object.  The EEOP_NULLIF
code passes that pointer straight on to the equality function.
Which in this case is a plpgsql function that will suppose it
can take ownership of the expanded object, resulting in said
object being freed before return.  (Neither function has done
anything wrong.)  The problem is that EEOP_NULLIF then returns
the original Datum pointer, which is now pointing at garbage.
The different failures you get depending on what is done next
with the Datum are not too surprising.

What EEOP_NULLIF needs to do is pass a read-only pointer to the
equality function, so that the object is not modified and remains
available to return if we want to do so.

Attached is a quick WIP patch to handle that.  It is missing a test
case, but the real omission is that llvm_compile_expr()'s EEOP_NULLIF
handling also needs to be fixed, and I'm pretty unsure how to do that.

I'm wondering now if any of our other conditional expressions have
similar bugs ...

            regards, tom lane

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 8f7a534005..69d36f70b3 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1191,6 +1191,14 @@ ExecInitExprRec(Expr *node, ExprState *state,
                              op->args, op->opfuncid, op->inputcollid,
                              state);

+                /*
+                 * If first argument is of varlena type, we'll need to ensure
+                 * that the value passed to the comparison function is a
+                 * read-only pointer.
+                 */
+                scratch.d.func.make_ro =
+                    (get_typlen(exprType((Node *) linitial(op->args))) == -1);
+
                 /*
                  * Change opcode of call instruction to EEOP_NULLIF.
                  *
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 30c5a19aad..42da962178 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1308,12 +1308,24 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
              * The arguments are already evaluated into fcinfo->args.
              */
             FunctionCallInfo fcinfo = op->d.func.fcinfo_data;
+            Datum        save_arg0 = fcinfo->args[0].value;

             /* if either argument is NULL they can't be equal */
             if (!fcinfo->args[0].isnull && !fcinfo->args[1].isnull)
             {
                 Datum        result;

+                /*
+                 * If first argument is of varlena type, it might be an
+                 * expanded datum.  We need to ensure that the value passed to
+                 * the comparison function is a read-only pointer.  However,
+                 * if we end by returning the first argument, that will be the
+                 * original read-write pointer if it was read-write.
+                 */
+                if (op->d.func.make_ro)
+                    fcinfo->args[0].value =
+                        MakeExpandedObjectReadOnlyInternal(save_arg0);
+
                 fcinfo->isnull = false;
                 result = op->d.func.fn_addr(fcinfo);

@@ -1328,7 +1340,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
             }

             /* Arguments aren't equal, so return the first one */
-            *op->resvalue = fcinfo->args[0].value;
+            *op->resvalue = save_arg0;
             *op->resnull = fcinfo->args[0].isnull;

             EEO_NEXT();
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index cd97dfa062..56fb0d0adb 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -365,6 +365,7 @@ typedef struct ExprEvalStep
             /* faster to access without additional indirection: */
             PGFunction    fn_addr;    /* actual call address */
             int            nargs;    /* number of arguments */
+            bool        make_ro;    /* make arg0 R/O (used only for NULLIF) */
         }            func;

         /* for EEOP_BOOL_*_STEP */