Fix 32-bit build dangling pointer issue in WindowAgg

david-rowley · david-rowley · commit 2a535620cec5 · 2022-12-07T00:10:21.000+13:00
9d9c02c added window "run conditions", which allows the evaluation of monotonic window functions to be skipped when the run condition is no longer true. Prior to this commit, once the run condition was no longer true and we stopped evaluating the window functions, we simply just left the ecxt_aggvalues[] and ecxt_aggnulls[] arrays alone to store whatever value was stored there the last time the window function was evaluated. Leaving a stale value in there isn't really a problem on 64-bit builds as all of the window functions which we recognize as monotonic all return int8, which is passed by value on 64-bit builds. However, on 32-bit builds, this was a problem as the value stored in the ecxt_values[] element would be a by-ref value and it would be pointing to some memory which would get reset once the tuple context is destroyed. Since the WindowAgg node will output these values in the resulting tupleslot, this could be problematic for the top-level WindowAgg node which must look at these values to filter out the rows that don't meet its filter condition. Here we fix this by just zeroing the ecxt_aggvalues[] and setting the ecxt_aggnulls[] array to true when the run condition first becomes false. This results in the WindowAgg's output having NULLs for the WindowFunc's columns rather than the stale or pointer pointing to possibly freed memory. These tuples with the NULLs can only make it as far as the top-level WindowAgg node before they're filtered out. To ensure that these tuples *are* always filtered out, we now insist that OpExprs making up the run condition are strict OpExprs. Currently, all the window functions which the planner recognizes as monotonic return INT8 and the operator which is used for the run condition must be a member of a btree opclass. In reality, these restrictions exclude nothing that's built-in to Postgres and are unlikely to exclude anyone's custom operators due to the requirement that the operator is part of a btree opclass. It would be unusual if those were not strict. Reported-by: Sergey Shinderuk, using valgrind Reviewed-by: Richard Guo, Sergey Shinderuk Discussion: https://postgr.es/m/29184c50-429a-ebd7-f1fb-0589c6723a35@postgrespro.ru Backpatch-through: 15, where 9d9c02c was added
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
@@ -2305,7 +2305,27 @@ ExecWindowAgg(PlanState *pstate)
 						continue;
 					}
 					else
+					{
 						winstate->status = WINDOWAGG_PASSTHROUGH;
+
+						/*
+						 * If we're not the top-window, we'd better NULLify
+						 * the aggregate results.  In pass-through mode we no
+						 * longer update these and this avoids the old stale
+						 * results lingering.  Some of these might be byref
+						 * types so we can't have them pointing to free'd
+						 * memory.  The planner insisted that quals used in
+						 * the runcondition are strict, so the top-level
+						 * WindowAgg will filter these NULLs out in the filter
+						 * clause.
+						 */
+						numfuncs = winstate->numfuncs;
+						for (i = 0; i < numfuncs; i++)
+						{
+							econtext->ecxt_aggvalues[i] = (Datum) 0;
+							econtext->ecxt_aggnulls[i] = true;
+						}
+					}
 				}
 				else
 				{
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
@@ -2399,6 +2399,18 @@ check_and_push_window_quals(Query *subquery, RangeTblEntry *rte, Index rti,
 	if (list_length(opexpr->args) != 2)
 		return true;
 
+	/*
+	 * Currently, we restrict this optimization to strict OpExprs.  The reason
+	 * for this is that during execution, once the runcondition becomes false,
+	 * we stop evaluating WindowFuncs.  To avoid leaving around stale window
+	 * function result values, we set them to NULL.  Having only strict
+	 * OpExprs here ensures that we properly filter out the tuples with NULLs
+	 * in the top-level WindowAgg.
+	 */
+	set_opfuncid(opexpr);
+	if (!func_strict(opexpr->opfuncid))
+		return true;
+
 	/*
 	 * Check for plain Vars that reference window functions in the subquery.
 	 * If we find any, we'll ask find_window_run_conditions() if 'opexpr' can
