Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c)

Hi Lucas,

I picked this up from the PG20-2 commitfest (#6825) as a first-time
reviewer. Summary up front: the bug is real and reproducible, the patch
does fix it, but I think the refcount bookkeeping needs another round.

Test environment:
master @ 9e17d25e79d
Linux x86_64, gcc 15.2.0
meson, --buildtype=debug -Dcassert=true

== Submission review ==

v3-0001 (the isolation test) applies cleanly.

v2-0002 (the fix) does *not* apply to current master. It conflicts in
the "Local data" block around ri_triggers.c:251 -- the RI fast-path work
that landed since you posted (c62b330912e, 2c45694a240, e2c812f1475 and
neighbours) restructured that area. "git apply -3" resolves it without
a real conflict, so this is just a rebase, but a v4 on top of current
master would help the next reviewer and cfbot.

"git apply" reports 12 whitespace errors across the two patches: 4 in
the .spec file and 8 in ri_triggers.c. Several are tabs immediately
after an opening brace, e.g.

ri_PreparedPlanExecutionStarted(SPIPlanPtr plan)
{<tab>

pgindent should clear these.

== Feature test ==

I can confirm the crash on unpatched master. Applying only v3-0001 and
running the new isolation test:

client backend (PID 23625) was terminated by signal 11: Segmentation fault
DETAIL: Failed process was running: DELETE FROM
crash_reentrancia_tabla_autoreferencial WHERE id = 1;
LOG: terminating any other active server processes
LOG: all server processes terminated; reinitializing

With v2-0002 applied the same test passes and the server stays up. So
the patch does address the reported crash. Thanks for the clear
reproducer -- the advisory-lock handshake to line up the invalidation
was a nice touch.

== Coding review ==

1. The refcount is leaked whenever the RI query throws.

In ri_PerformCheck() the increment and decrement bracket
SPI_execute_snapshot() with no PG_TRY/PG_FINALLY:

ri_PreparedPlanExecutionStarted(qplan);
spi_result = SPI_execute_snapshot(qplan, ...);
ri_PreparedPlanExecutionFinished(qplan);

Any ereport(ERROR) from inside the RI query longjmps past the Finished()
call, so the count is never given back. This is not an exotic path -- a
BEFORE DELETE trigger on the referencing table that raises will do it,
and so will statement_timeout, query cancel or a deadlock during the
cascade.

I instrumented the hash table locally to check, using a parent/child
pair with ON DELETE CASCADE where the child has a BEFORE DELETE trigger
that raises, and ten cascade deletes each caught by an EXCEPTION block:

RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=1
RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=2
RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=3
...
RI_REFCOUNT_DEBUG entries=1 plan=0x60f5822cfd30 refcount=10

The count rises monotonically and never comes back down. Once that has
happened ri_PreparedPlanCanRelease() returns false for that plan
forever, so ri_FetchPreparedPlan() will never SPI_freeplan() it: on the
next invalidation it sets entry->plan = NULL and the plan is orphaned in
CacheMemoryContext for the life of the backend.

A PG_TRY/PG_FINALLY around the execute, or tying the decrement to
resource-owner or subtransaction cleanup, would fix this.

2. Entries are never removed when the refcount drops to zero on a plan
that is still valid.

ri_PreparedPlanExecutionFinished() only does HASH_REMOVE inside

if (entry->refcount == 0 && !SPI_plan_is_valid(plan))

which is the uncommon case. Normally the entry stays behind with
refcount 0 forever. I think the removal should happen whenever the
count reaches 0, independently of plan validity.

3. Keying the hash table on the raw SPIPlanPtr looks fragile.

Because entries outlive the plans they describe (point 2), the table
accumulates entries keyed on pointers that have since been freed. That
would be harmless if addresses were never reused, but they are. Driving
40 plan invalidations through ALTER TABLE, I only ever saw three
distinct plan addresses, cycling:

entries=1 plan=0x60f5822cf900
entries=2 plan=0x60f5822ce8d0
entries=3 plan=0x60f5822ce0b0
... then those same three addresses repeatedly, entries stuck at 3

So a freshly created plan routinely lands on an address that already has
an entry and inherits whatever refcount it was left holding.

I want to be careful not to overstate this: in every path I could
actually reach, the inherited value was 0, and I could not turn this
into a demonstrable failure. So treat it as a design concern rather
than a proven bug. But combined with point 1, which does leave counts
above zero, a new plan could start life pinned and never be freed -- or
a count could reach zero while an outer reentrant frame still holds the
plan, which is the use-after-free this patch exists to prevent. Storing
the refcount on the RI_QueryHashEntry that already owns the plan would
sidestep the question entirely.

4. entry->refcount-- is unguarded.

It is a uint32, so a stray extra Finished() call (or a stale entry per
point 3) wraps it to 4294967295 rather than tripping anything. An
Assert(entry->refcount > 0) before the decrement would catch that in
cassert builds.

5. The ri_InitHashTables() call in ri_PreparedPlanExecutionStarted().

if (!ri_query_plan_cache_executing_refcount)
ri_InitHashTables();

ri_InitHashTables() unconditionally recreates all four hash tables and
re-runs both CacheRegisterSyscacheCallback() calls. If this branch were
ever taken with the other caches already populated it would orphan
ri_constraint_cache, ri_query_cache and ri_compare_cache, and register
duplicate syscache callbacks against a limited pool. In practice
ri_PerformCheck() is only reached after the caches exist, so the branch
looks unreachable -- which argues for an Assert instead, or for
splitting the refcount table's initialisation out.

6. Style points

- "// Remove the entry" needs to be a /* */ comment.
- "RI_QueryPlanCacheExecutingRefCountEntry* entry" should be
"... *entry" (three occurrences).
- "bool found" is declared in ri_PreparedPlanExecutionFinished() and
ri_PreparedPlanCanRelease() but never read; both test !entry.
- Several lines run to 103-131 columns.
- The three new functions have no comment headers, unlike their
neighbours in this file.
- "this call can free the plan..." should start with a capital.

All pgindent/typedefs.list territory rather than anything substantive.

== Test patch ==

1. The identifiers are in Spanish -- crash_reentrancia_tabla_
autoreferencial, nombre, padre_id, crash_reentrancia_segunda_tabla,
valor. The rest of the tree is English, so these will need renaming.

2. The test leans on overflowing the shared invalidation queue with 1000
temp table create/drops. It does reproduce reliably here (7.2s), but
nothing makes it fail loudly if that stops being enough -- it would just
start passing on an unfixed backend. Is there a way to make the
invalidation deterministic?

3. A crash test in the isolation schedule takes down the whole cluster
when it fails, aborting the rest of the schedule. I do not know the
project's preference here, but it may be worth asking whether this
belongs in src/test/isolation or as a TAP test.

4. The expected output only shows that s1_delete completed; it does not
check the resulting table contents. Asserting the surviving rows would
turn "did not crash" into "cascaded correctly".

5. Four of the whitespace errors above are in this file.

== Regression testing ==

Full "meson test" with the patch: 359 ok, 49 skipped, 1 failed.

The failure was recovery/027_stream_regress, with

TRAP: failed Assert("plan->magic == _SPI_PLAN_MAGIC"),
File: "../src/backend/executor/spi.c", Line: 1951
client backend was terminated by signal 6: Aborted
DETAIL: Failed process was running: UPDATE temporal_mltrng
SET valid_at = datemultirange(daterange('2016-02-01','2016-03-01'))
WHERE id = '[5,6)' AND ...

I initially assumed the patch had caused this, since it is the same
use-after-free shape the patch is about. It has not. Clean master with
no patch applied reproduces the identical assertion. Counts over 15
runs each:

master : 5/15 runs hit the assert (33%)
patched : 9/15 runs hit the assert (60%)

Fisher exact two-tailed p = 0.27, so the difference is not significant
at these sample sizes and I am not claiming the patch makes it worse --
only that it does not fix it and that the failure is pre-existing. I
will report that one separately rather than tangle it up with this
thread.

Aside from 027_stream_regress, nothing regressed.

== Summary ==

The crash is real, easy to trigger, and the patch fixes it. I would
call the direction sound but the bookkeeping not ready: point 1 is a
straightforward leak on a common error path, and point 3 makes me uneasy
about the choice of hash key.

Marking this Waiting on Author. Happy to retest a v4.

On the wider question about whether a BEFORE DELETE trigger should be
deleting rows at all -- I do not have the standing to argue that either
way. But a backend that segfaults seems worth closing regardless of
whether the usage is advisable, and if the consensus is that it should
not be allowed, an explicit error would still need this same reentrancy
information to detect the situation.

Regards,
Trakshan Mishra