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 26d7972f837d91db55d4a6cac1811fb0b0b8d4d7..4b57f538baffb55d193b94dd6e2176ee9c95e6d5 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1169,6 +1169,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 87c7603f2b8d6e77e274db7812a591c2ed9d66ef..55d42cd101d428de89541baa01f92652d1ccc698 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1273,12 +1273,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);
 
@@ -1293,7 +1305,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 95836af1e3a04c85fdd042c055b0c0a40be3fd30..f567356ed199cd6c77a3a8d0050a8ff960ff5d94 100644 (file)
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1550,6 +1550,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);
@@ -1566,7 +1569,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]);
@@ -1574,12 +1576,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 144728e23d3a23e895cc05009634305ee8d7a1bd..77fda6b0f8e2566cc874268a4cba5830a4bf0317 100644 (file)
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -345,6 +345,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 f5136c17abbf0eaed00ab76ece3d9ee933dd9190..efee7fc43173b4c52f6d65fb0b14936191edfcd6 100644 (file)
--- a/src/test/regress/expected/case.out
+++ b/src/test/regress/expected/case.out
@@ -397,6 +397,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 83fe43be6b84f1b96db4285e2e9eeca037ff261f..388d4c6f52835ebee50168b0abd4b18cabfcfde9 100644 (file)
--- a/src/test/regress/sql/case.sql
+++ b/src/test/regress/sql/case.sql
@@ -242,6 +242,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)