From 6251c1c0a5ff21667318d198bdf89cd4889c3a0d Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Tue, 18 Aug 2026 21:34:24 +0900 Subject: [PATCH v2 1/2] Don't free fast-path FK metadata from the inval callback Commit e484b0eea6 made InvalidateConstraintCacheCallBack() pfree an entry's FastPathMeta to plug a leak, but that breaks the rule stated atop the callback: entries may have active references at invalidation time, so we mark them invalid rather than removing them. The metadata is subject to the same rule. ri_FastPathCheck() and ri_FastPathBatchFlush() copy riinfo->fpmeta into a local, and ri_FastPathFlushArray() additionally takes FmgrInfo pointers into it before its "walk all matches" loop. That loop runs index_getnext_slot(), ri_LockPKTuple(), and user-supplied cast and equality functions, any of which can accept invalidation messages, and a user function that performs DDL triggers one deliberately, no concurrency required. The callback then freed the object still in use, so the loop read freed memory and called through FmgrInfos in it. Fix by unlinking the metadata from the entry, so the next check rebuilds it as before, but deferring the actual free to AtEOXact_RI(), which runs from CommitTransaction() / PrepareTransaction() / AbortTransaction() with no RI check on the stack. Detached objects are chained through a new next_dead field and released there. The queue holds a few kB per detached object until the transaction ends, which only affects transactions that interleave DDL with FK-checking DML. That seems clearly preferable to a use-after-free. Unlinking alone is not enough for the multi-column path. ri_FastPathFlushLoop() calls build_index_scankeys() once per buffered row, and that function re-read riinfo->fpmeta each time. A cast invoked for one row can accept an invalidation that clears the field, so the next row found NULL there. Pass the metadata down from ri_FastPathBatchFlush() instead, as the array path already did, so one reference covers the whole batch. That is only safe because of the deferred release above; latching without it would turn the NULL dereference into a use-after-free. Reported-by: Ayush Tiwari Reviewed-by: Ayush Tiwari Discussion: https://postgr.es/m/CA+HiwqFFB6vzx8v3t2=rbNYyxMistLf5kkJfqzJ81nadFyLrxA@mail.gmail.com Backpatch-through: 19 --- src/backend/utils/adt/ri_triggers.c | 89 +++++++++++++++++++---- src/test/regress/expected/foreign_key.out | 64 ++++++++++++++++ src/test/regress/sql/foreign_key.sql | 56 ++++++++++++++ 3 files changed, 195 insertions(+), 14 deletions(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 5d843ce8a0d..5be183706e8 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -160,6 +160,9 @@ typedef struct FastPathMeta Oid subtypes[RI_MAX_NUMKEYS]; int strats[RI_MAX_NUMKEYS]; AttrNumber index_attnos[RI_MAX_NUMKEYS]; /* index column positions */ + + /* Link in ri_fpmeta_dead_list while awaiting deferred release */ + struct FastPathMeta *next_dead; } FastPathMeta; /* @@ -272,6 +275,14 @@ static HTAB *ri_fastpath_cache = NULL; static bool ri_fastpath_callback_registered = false; static bool ri_fastpath_flushing = false; +/* + * FastPathMeta objects detached from their cache entry by invalidation, but + * possibly still referenced by an RI check further up the stack. Released + * by AtEOXact_RI(), where no such reference can exist. See + * InvalidateConstraintCacheCallBack(). + */ +static FastPathMeta *ri_fpmeta_dead_list = NULL; + /* * Local function prototypes */ @@ -326,10 +337,12 @@ static void ri_FastPathBatchAdd(RI_ConstraintInfo *riinfo, static void ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel, RI_ConstraintInfo *riinfo); static int ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, - const RI_ConstraintInfo *riinfo, Relation fk_rel, + const RI_ConstraintInfo *riinfo, + FastPathMeta *fpmeta, Relation fk_rel, Snapshot snapshot, IndexScanDesc scandesc); static int ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, - const RI_ConstraintInfo *riinfo, Relation fk_rel, + const RI_ConstraintInfo *riinfo, + FastPathMeta *fpmeta, Relation fk_rel, Snapshot snapshot, IndexScanDesc scandesc); static bool ri_FastPathProbeOne(Relation pk_rel, Relation idx_rel, IndexScanDesc scandesc, TupleTableSlot *slot, @@ -342,6 +355,7 @@ static void ri_CheckPermissions(Relation query_rel); static bool recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys, TupleTableSlot *new_slot); static void build_index_scankeys(const RI_ConstraintInfo *riinfo, + FastPathMeta *fpmeta, Relation idx_rel, Datum *pk_vals, char *pk_nulls, ScanKey skeys); static void ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo, @@ -2561,6 +2575,10 @@ get_ri_constraint_root(Oid constrOid) * from the cache, but only mark them invalid, which is harmless to active * uses. (Any query using an entry should hold a lock sufficient to keep that * data from changing under it --- but we may get cache flushes anyway.) + * + * The fast-path metadata hanging off an entry is subject to the same rule. + * We unlink it so that the next check rebuilds it, but the object itself is + * only queued here and is actually released by AtEOXact_RI(). */ static void InvalidateConstraintCacheCallBack(Datum arg, SysCacheIdentifier cacheid, @@ -2594,11 +2612,25 @@ InvalidateConstraintCacheCallBack(Datum arg, SysCacheIdentifier cacheid, riinfo->rootHashValue == hashvalue) { riinfo->valid = false; + + /* + * Detach any fast-path metadata so that the next check + * repopulates it, but do not free it here. ri_FastPathCheck() + * and the flush routines copy riinfo->fpmeta into a local (and + * take FmgrInfo pointers into it) and then run index scans, tuple + * locking, and user-supplied cast and equality functions, all of + * which can accept invalidation messages and reach this callback. + * Freeing now would leave those callers reading freed memory. + * Queue it instead; AtEOXact_RI() releases it once no RI check + * can be running. + */ if (riinfo->fpmeta) { - pfree(riinfo->fpmeta); + riinfo->fpmeta->next_dead = ri_fpmeta_dead_list; + ri_fpmeta_dead_list = riinfo->fpmeta; riinfo->fpmeta = NULL; } + /* Remove invalidated entries from the list, too */ dclist_delete_from(&ri_constraint_cache_valid_list, iter.cur); } @@ -2862,7 +2894,8 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, } Assert(riinfo->fpmeta); ri_ExtractValues(fk_rel, newslot, riinfo, false, pk_vals, pk_nulls); - build_index_scankeys(riinfo, idx_rel, pk_vals, pk_nulls, skey); + build_index_scankeys(riinfo, riinfo->fpmeta, idx_rel, pk_vals, pk_nulls, + skey); found = ri_FastPathProbeOne(pk_rel, idx_rel, scandesc, slot, snapshot, riinfo, skey, riinfo->nkeys); SetUserIdAndSecContext(saved_userid, saved_sec_context); @@ -2954,6 +2987,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel, Oid saved_userid; int saved_sec_context; MemoryContext oldcxt; + FastPathMeta *fpmeta; int violation_index; if (fpentry->batch_count == 0) @@ -3011,6 +3045,15 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel, } Assert(riinfo->fpmeta); + /* + * Take our own reference to the metadata for the duration of the flush. + * The probe below runs user-defined cast and equality functions, which can + * accept invalidation messages; InvalidateConstraintCacheCallBack() then + * clears riinfo->fpmeta, so re-reading it partway through the batch would + * find NULL. The object itself stays valid until AtEOXact_RI(). + */ + fpmeta = riinfo->fpmeta; + /* * The probe runs user-defined cast and equality functions. Set the * flushing flag around it so a re-entrant ri_FastPathBatchAdd on this @@ -3024,10 +3067,12 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel, /* Skip array overhead for single-row batches. */ if (riinfo->nkeys == 1 && fpentry->batch_count > 1) violation_index = ri_FastPathFlushArray(fpentry, fk_slot, riinfo, - fk_rel, snapshot, scandesc); + fpmeta, fk_rel, snapshot, + scandesc); else violation_index = ri_FastPathFlushLoop(fpentry, fk_slot, riinfo, - fk_rel, snapshot, scandesc); + fpmeta, fk_rel, snapshot, + scandesc); } PG_FINALLY(); { @@ -3065,8 +3110,9 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry, Relation fk_rel, */ static int ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, - const RI_ConstraintInfo *riinfo, Relation fk_rel, - Snapshot snapshot, IndexScanDesc scandesc) + const RI_ConstraintInfo *riinfo, FastPathMeta *fpmeta, + Relation fk_rel, Snapshot snapshot, + IndexScanDesc scandesc) { Relation pk_rel = fpentry->pk_rel; Relation idx_rel = fpentry->idx_rel; @@ -3080,7 +3126,7 @@ ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, { ExecStoreHeapTuple(fpentry->batch[i], fk_slot, false); ri_ExtractValues(fk_rel, fk_slot, riinfo, false, pk_vals, pk_nulls); - build_index_scankeys(riinfo, idx_rel, pk_vals, pk_nulls, skey); + build_index_scankeys(riinfo, fpmeta, idx_rel, pk_vals, pk_nulls, skey); found = ri_FastPathProbeOne(pk_rel, idx_rel, scandesc, pk_slot, snapshot, riinfo, skey, riinfo->nkeys); @@ -3109,10 +3155,10 @@ ri_FastPathFlushLoop(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, */ static int ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, - const RI_ConstraintInfo *riinfo, Relation fk_rel, - Snapshot snapshot, IndexScanDesc scandesc) + const RI_ConstraintInfo *riinfo, FastPathMeta *fpmeta, + Relation fk_rel, Snapshot snapshot, + IndexScanDesc scandesc) { - FastPathMeta *fpmeta = riinfo->fpmeta; Relation pk_rel = fpentry->pk_rel; Relation idx_rel = fpentry->idx_rel; TupleTableSlot *pk_slot = fpentry->pk_slot; @@ -3496,11 +3542,10 @@ recheck_matched_pk_tuple(Relation idxrel, ScanKeyData *skeys, int nkeys, */ static void build_index_scankeys(const RI_ConstraintInfo *riinfo, + FastPathMeta *fpmeta, Relation idx_rel, Datum *pk_vals, char *pk_nulls, ScanKey skeys) { - FastPathMeta *fpmeta = riinfo->fpmeta; - Assert(fpmeta); /* @@ -3553,8 +3598,10 @@ ri_populate_fastpath_metadata(RI_ConstraintInfo *riinfo, MemoryContext oldcxt = MemoryContextSwitchTo(TopMemoryContext); Assert(riinfo != NULL && riinfo->valid); + Assert(riinfo->fpmeta == NULL); fpmeta = palloc_object(FastPathMeta); + fpmeta->next_dead = NULL; for (int i = 0; i < riinfo->nkeys; i++) { Oid eq_opr = riinfo->pf_eq_oprs[i]; @@ -4368,6 +4415,20 @@ AtEOXact_RI(bool isCommit) * set. */ ri_fastpath_flushing = false; + + /* + * Release fast-path metadata detached during this transaction by + * InvalidateConstraintCacheCallBack(). We are past every RI check that + * could still hold a pointer into one of these, so freeing here is safe + * on both the commit and the abort path. + */ + while (ri_fpmeta_dead_list != NULL) + { + FastPathMeta *dead = ri_fpmeta_dead_list; + + ri_fpmeta_dead_list = dead->next_dead; + pfree(dead); + } } /* diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out index 120d3319451..b699164260a 100644 --- a/src/test/regress/expected/foreign_key.out +++ b/src/test/regress/expected/foreign_key.out @@ -3846,3 +3846,67 @@ DETAIL: Key (a)=(999) is not present in table "fp_subxact_pk". DROP TRIGGER fp_subxact_trg ON fp_subxact_fk; DROP FUNCTION fp_abort_subxact(); DROP TABLE fp_subxact_fk, fp_subxact_pk; +-- +-- Cache invalidation arriving in the middle of a fast-path batch flush. +-- +-- A cross-type foreign key runs the user's cast function once per key per +-- buffered row, inside ri_FastPathFlushLoop(). A cast that performs DDL +-- raises an invalidation there, which detaches the constraint's fast-path +-- metadata while the flush is still using it. +-- +CREATE TYPE fkint; +CREATE FUNCTION fkint_in(cstring) RETURNS fkint + AS 'int4in' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: return type fkint is only a shell +CREATE FUNCTION fkint_out(fkint) RETURNS cstring + AS 'int4out' LANGUAGE internal IMMUTABLE STRICT; +NOTICE: argument type fkint is only a shell +LINE 1: CREATE FUNCTION fkint_out(fkint) RETURNS cstring + ^ +CREATE TYPE fkint (INPUT = fkint_in, OUTPUT = fkint_out, LIKE = int4); +-- Renames the constraint the first time it is called, and so raises an +-- invalidation partway through the flush. Guarded on the catalog so the +-- second and later calls are no-ops. +CREATE FUNCTION fkint_to_int4(fkint) RETURNS int4 AS $$ +BEGIN + IF EXISTS (SELECT 1 FROM pg_constraint + WHERE conrelid = 'fktable_inval'::regclass + AND conname = 'fktable_inval_fk') THEN + EXECUTE 'ALTER TABLE fktable_inval' + ' RENAME CONSTRAINT fktable_inval_fk TO fktable_inval_fk2'; + END IF; + RETURN format('%s', $1)::int4; +END $$ LANGUAGE plpgsql; +CREATE CAST (fkint AS int4) WITH FUNCTION fkint_to_int4(fkint) AS IMPLICIT; +CREATE TABLE pktable_inval (a int4, b int4, PRIMARY KEY (a, b)); +INSERT INTO pktable_inval VALUES (1, 1), (2, 2); +-- Multi-column FK, so the flush takes the per-row loop rather than the +-- array path; cross-type on column a, so the cast above is invoked. +CREATE TABLE fktable_inval (a fkint, b int4, + CONSTRAINT fktable_inval_fk FOREIGN KEY (a, b) + REFERENCES pktable_inval (a, b)); +-- More than one row, so the flush is still running after the invalidation. +INSERT INTO fktable_inval VALUES ('1', 1), ('2', 2); +-- Confirms the cast actually ran and raised the invalidation. Without this +-- the insert above could pass merely by not exercising the path at all. +SELECT conname FROM pg_constraint + WHERE conrelid = 'fktable_inval'::regclass AND contype = 'f'; + conname +------------------- + fktable_inval_fk2 +(1 row) + +SELECT count(*) FROM fktable_inval; + count +------- + 2 +(1 row) + +DROP TABLE fktable_inval; +DROP TABLE pktable_inval; +DROP CAST (fkint AS int4); +DROP FUNCTION fkint_to_int4(fkint); +DROP TYPE fkint CASCADE; +NOTICE: drop cascades to 2 other objects +DETAIL: drop cascades to function fkint_in(cstring) +drop cascades to function fkint_out(fkint) diff --git a/src/test/regress/sql/foreign_key.sql b/src/test/regress/sql/foreign_key.sql index b9b88064ea5..31736251d78 100644 --- a/src/test/regress/sql/foreign_key.sql +++ b/src/test/regress/sql/foreign_key.sql @@ -2801,3 +2801,59 @@ INSERT INTO fp_subxact_fk VALUES (999, 'bad'), (0, 'boom'), (1, 'ok'); DROP TRIGGER fp_subxact_trg ON fp_subxact_fk; DROP FUNCTION fp_abort_subxact(); DROP TABLE fp_subxact_fk, fp_subxact_pk; + +-- +-- Cache invalidation arriving in the middle of a fast-path batch flush. +-- +-- A cross-type foreign key runs the user's cast function once per key per +-- buffered row, inside ri_FastPathFlushLoop(). A cast that performs DDL +-- raises an invalidation there, which detaches the constraint's fast-path +-- metadata while the flush is still using it. +-- +CREATE TYPE fkint; +CREATE FUNCTION fkint_in(cstring) RETURNS fkint + AS 'int4in' LANGUAGE internal IMMUTABLE STRICT; +CREATE FUNCTION fkint_out(fkint) RETURNS cstring + AS 'int4out' LANGUAGE internal IMMUTABLE STRICT; +CREATE TYPE fkint (INPUT = fkint_in, OUTPUT = fkint_out, LIKE = int4); + +-- Renames the constraint the first time it is called, and so raises an +-- invalidation partway through the flush. Guarded on the catalog so the +-- second and later calls are no-ops. +CREATE FUNCTION fkint_to_int4(fkint) RETURNS int4 AS $$ +BEGIN + IF EXISTS (SELECT 1 FROM pg_constraint + WHERE conrelid = 'fktable_inval'::regclass + AND conname = 'fktable_inval_fk') THEN + EXECUTE 'ALTER TABLE fktable_inval' + ' RENAME CONSTRAINT fktable_inval_fk TO fktable_inval_fk2'; + END IF; + RETURN format('%s', $1)::int4; +END $$ LANGUAGE plpgsql; + +CREATE CAST (fkint AS int4) WITH FUNCTION fkint_to_int4(fkint) AS IMPLICIT; + +CREATE TABLE pktable_inval (a int4, b int4, PRIMARY KEY (a, b)); +INSERT INTO pktable_inval VALUES (1, 1), (2, 2); + +-- Multi-column FK, so the flush takes the per-row loop rather than the +-- array path; cross-type on column a, so the cast above is invoked. +CREATE TABLE fktable_inval (a fkint, b int4, + CONSTRAINT fktable_inval_fk FOREIGN KEY (a, b) + REFERENCES pktable_inval (a, b)); + +-- More than one row, so the flush is still running after the invalidation. +INSERT INTO fktable_inval VALUES ('1', 1), ('2', 2); + +-- Confirms the cast actually ran and raised the invalidation. Without this +-- the insert above could pass merely by not exercising the path at all. +SELECT conname FROM pg_constraint + WHERE conrelid = 'fktable_inval'::regclass AND contype = 'f'; + +SELECT count(*) FROM fktable_inval; + +DROP TABLE fktable_inval; +DROP TABLE pktable_inval; +DROP CAST (fkint AS int4); +DROP FUNCTION fkint_to_int4(fkint); +DROP TYPE fkint CASCADE; -- 2.47.3