Fix NULLIF()'s handling of read-write expanded objects.

If passed a read-write expanded object pointer, the EEOP_NULLIF
code would hand that same pointer to the equality function
and then (unless equality was reported) also return the same
pointer as its value.  This is no good, because a function that
receives a read-write expanded object pointer is fully entitled
to scribble on or even delete the object, thus corrupting the
NULLIF output.  (This problem is likely unobservable with the
equality functions provided in core Postgres, but it's easy to
demonstrate with one coded in plpgsql.)

To fix, make sure the pointer passed to the equality function
is read-only.  We can still return the original read-write
pointer as the NULLIF result, allowing optimization of later
operations.

Per bug #18722 from Alexander Lakhin.  This has been wrong
since we invented expanded objects, so back-patch to all
supported branches.

Discussion: https://postgr.es/m/18722-fd9e645448cc78b4@postgresql.org

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 8b64ba8aefef2d3fd72e9c45465748bd955c0984..458a0c388ac7a49839c92f4f3c9cbc1fffa240db 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -964,6 +964,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 21cbf513f419e3b9ae08bf92b09b52c37feda24c..8c1227ee11e372d3b35920975d2b325c34ade2bc 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1226,12 +1226,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);
 
@@ -1246,7 +1258,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/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index d1d5d42e8ec8ca72693d468eee9e255fe554181d..c0e7d036fbabc84ad3305ff04f278af1f69b5e0e 100644 (file)
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1519,6 +1519,9 @@ llvm_compile_expr(ExprState *state)
 
                    v_fcinfo = l_ptr_const(fcinfo, l_ptr(StructFunctionCallInfoData));
 
+                   /* save original arg[0] */
+                   v_arg0 = l_funcvalue(b, v_fcinfo, 0);
+
                    /* if either argument is NULL they can't be equal */
                    v_argnull0 = l_funcnull(b, v_fcinfo, 0);
                    v_argnull1 = l_funcnull(b, v_fcinfo, 1);
@@ -1535,7 +1538,6 @@ llvm_compile_expr(ExprState *state)
 
                    /* one (or both) of the arguments are null, return arg[0] */
                    LLVMPositionBuilderAtEnd(b, b_hasnull);
-                   v_arg0 = l_funcvalue(b, v_fcinfo, 0);
                    LLVMBuildStore(b, v_argnull0, v_resnullp);
                    LLVMBuildStore(b, v_arg0, v_resvaluep);
                    LLVMBuildBr(b, opblocks[opno + 1]);
@@ -1543,12 +1545,35 @@ llvm_compile_expr(ExprState *state)
                    /* build block to invoke function and check result */
                    LLVMPositionBuilderAtEnd(b, b_nonull);
 
+                   /*
+                    * 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)
+                   {
+                       LLVMValueRef v_params[1];
+                       LLVMValueRef v_arg0_ro;
+
+                       v_params[0] = v_arg0;
+                       v_arg0_ro =
+                           l_call(b,
+                                  llvm_pg_var_func_type("MakeExpandedObjectReadOnlyInternal"),
+                                  llvm_pg_func(mod, "MakeExpandedObjectReadOnlyInternal"),
+                                  v_params, lengthof(v_params), "");
+                       LLVMBuildStore(b, v_arg0_ro,
+                                      l_funcvaluep(b, v_fcinfo, 0));
+                   }
+
                    v_retval = BuildV1Call(context, b, mod, fcinfo, &v_fcinfo_isnull);
 
                    /*
-                    * If result not null, and arguments are equal return null
-                    * (same result as if there'd been NULLs, hence reuse
-                    * b_hasnull).
+                    * If result not null and arguments are equal return null,
+                    * else return arg[0] (same result as if there'd been
+                    * NULLs, hence reuse b_hasnull).
                     */
                    v_argsequal = LLVMBuildAnd(b,
                                               LLVMBuildICmp(b, LLVMIntEQ,

diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index afbab95adcd1b441457bf602ef4679267d5c1fb2..fa3beec2e8ed8a659affedee9145b4d29fb91f79 100644 (file)
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -339,6 +339,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 */

diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index c0c8acf035adb156cca3f683a6be258337d97b39..c3d41d7553a2452332caa210cb504e6348d958db 100644 (file)
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -372,6 +372,14 @@ SELECT CASE make_ad(1,2)
  right
 (1 row)
 
+-- While we're here, also test handling of a NULLIF arg that is a read/write
+-- object (bug #18722)
+SELECT NULLIF(make_ad(1,2), array[2,3]::arrdomain);
+ nullif 
+--------
+ {1,2}
+(1 row)
+
 ROLLBACK;
 -- Test interaction of CASE with ArrayCoerceExpr (bug #15471)
 BEGIN;

diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sql
index 17436c524a7aee978c93747e7227fc6e99ea64d3..f5ad4afb5ca525f3198adb9d35a19c41261af342 100644 (file)
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -231,6 +231,11 @@ SELECT CASE make_ad(1,2)
   WHEN array[1,2]::arrdomain THEN 'right'
   END;
 
+-- While we're here, also test handling of a NULLIF arg that is a read/write
+-- object (bug #18722)
+
+SELECT NULLIF(make_ad(1,2), array[2,3]::arrdomain);
+
 ROLLBACK;
 
 -- Test interaction of CASE with ArrayCoerceExpr (bug #15471)