Don't free a cached RI plan that may be in use Currently, ri_FetchPreparedPlan() immediately frees a cached SPI plan if SPI_plan_is_valid() returns false, without tracking whether the plan might be in use. That can cause _SPI_execute_plan() to read freed memory. This happens, for example, when an RI check's or action's query runs some user-defined code such as a cast function or BEFORE ROW trigger function which may perform its own DML using its own query that is not restricted from firing its own triggers, some of which may be RI triggers for the same constraint as the outer RI check and hence use the same cached plan. If an invalidation arrives that marks the cached plans referenced by the outer check's SPI plan invalid between the outer check having called _SPI_execute_plan() and the inner check's ri_FetchPreparedPlan(), the latter will free the SPI plan as mentioned above.
Lucas Jeffrey reported an instance of this bug involving a self-referencing ON DELETE CASCADE foreign key. The RI action's DELETE fires a BEFORE DELETE trigger on the rows it deletes, which invalidates the action's plan with GRANT and REVOKE on the table and then runs a DELETE of its own, and the RI action fired by that DELETE is the inner check described above. Separately, Tom Lane reported that buildfarm member trilobite, which runs with CLOBBER_CACHE_ALWAYS, hit this in the regression tests on master after a change there made more FK checks go through SPI. Fix by keeping a stack of the plans that every ri_PerformCheck() call in progress is executing, pushed before SPI_execute_snapshot() and popped whether it returns or throws an error, the latter using PG_FINALLY(). ri_FetchPreparedPlan() now frees an invalid plan on the spot when it is not on that stack, as before, since nothing can be executing it. Otherwise it only detaches the plan from its hash entry and queues it on a list. Whenever a check finishes, the queued plans that are no longer on the stack are freed. So a plan is held only while a check is actually executing it, however many invalidations happen meanwhile. A new AtEOXact_RI() frees whatever is left. Add the test from Lucas's original patch, which reproduces the crash on an ordinary cassert build, and a test that raises errors from inside a check's query, some of them inside subtransactions that are then rolled back, to check that the stack stays exact. Author: Lucas Jeffrey <[email protected]> Co-authored-by: Trakshan Mishra <[email protected]> Co-authored-by: Amit Langote <[email protected]> Reported-by: Lucas Jeffrey <[email protected]> Reported-by: Tom Lane <[email protected]> Reviewed-by: Tomas Vondra <[email protected]> Discussion: https://postgr.es/m/[email protected] Discussion: https://postgr.es/m/caghzy7s0xfxdk3amd5c4qhps1o9dysvzpr+-d9ohae4iv_v...@mail.gmail.com Backpatch-through: 14 Branch ------ REL_15_STABLE Details ------- https://git.postgresql.org/pg/commitdiff/3cb89e214b428bca09eb8de9afd1401d392e2977 Modified Files -------------- src/backend/access/transam/xact.c | 3 + src/backend/utils/adt/ri_triggers.c | 207 ++++++++++++++++++++++++++++-- src/include/commands/trigger.h | 1 + src/test/regress/expected/foreign_key.out | 156 ++++++++++++++++++++++ src/test/regress/sql/foreign_key.sql | 125 ++++++++++++++++++ 5 files changed, 484 insertions(+), 8 deletions(-)
