public inbox for [email protected]
help / color / mirror / Atom feedFrom: Lucas Jeffrey <[email protected]>
To: [email protected]
Cc: Rodolfo Campero <[email protected]>
Cc: Marcos Castedo <[email protected]>
Cc: Andrés Krüger <[email protected]>
Subject: [PATCH] Add reentrancy guards in ri_triggers.c
Date: Wed, 20 May 2026 10:14:36 -0300
Message-ID: <CAGHzy7RCXRo6iz1kL-p6g1r9x=EL-Yb7jPgLj4n5YEtYsugzJg@mail.gmail.com> (raw)
Hi hackers,
We found a bug where executing a DELETE on a self-referential table that
fires triggers can cause a segmentation fault. This is due to a
*use-after-free* of a Postgres plan generated by the referential integrity
module (ri_triggers.c, RI_FKey_cascade_del). The crash occurs if the
Postgres plancache is invalidated (ResetPlanCache) during the execution of
a reentrant RI trigger.
A reentrant RI_FKey_cascade_del can occur if a table is self-referential
(i.e., it has a foreign key referencing its own primary key) and has BEFORE
DELETE triggers that delete rows from that same table.
-
*The first patch* adds a test case that reproduces the segmentation
fault. The crash itself happens in _SPI_execute_plan, but the root cause
is that the plan being executed was prematurely freed by the RI module.
-
*The second patch* fixes ri_triggers.c by introducing reentrancy guards,
which maintain a reference count of plans in execution to prevent them from
being freed while active.
Feedback and reviews are welcome.
Best regards,
Lucas Jeffrey
Attachments:
[text/x-patch] 0002-Fix-crash-in-RI-triggers-by-refcounting-active-plans.patch (4.8K, 3-0002-Fix-crash-in-RI-triggers-by-refcounting-active-plans.patch)
download | inline diff:
From b93e8f45f6b67083f610c53624e19320cb80f6e9 Mon Sep 17 00:00:00 2001
From: luquijeffrey <[email protected]>
Date: Tue, 19 May 2026 17:42:11 -0300
Subject: [PATCH 2/2] Fix crash in RI triggers by refcounting active plans
---
src/backend/utils/adt/ri_triggers.c | 86 ++++++++++++++++++++++++++++-
1 file changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index dc89c686394..10019dc8ec5 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -251,12 +251,24 @@ typedef struct RI_FastPathEntry
int batch_count;
} RI_FastPathEntry;
+/*
+ * RI_QueryPlanCacheExecutingRefCountEntry
+ *
+ * Entry to track the number of times a prepared plan is being executed.
+ */
+typedef struct RI_QueryPlanCacheExecutingRefCountEntry
+{
+ SPIPlanPtr plan;
+ uint32 refcount; /* number of times this plan is being executed (can be more than 1 if reentrant) */
+} RI_QueryPlanCacheExecutingRefCountEntry;
+
/*
* Local data
*/
static HTAB *ri_constraint_cache = NULL;
static HTAB *ri_query_cache = NULL;
static HTAB *ri_compare_cache = NULL;
+static HTAB *ri_query_plan_cache_executing_refcount = NULL;
static dclist_head ri_constraint_cache_valid_list;
static HTAB *ri_fastpath_cache = NULL;
@@ -295,6 +307,11 @@ static SPIPlanPtr ri_FetchPreparedPlan(RI_QueryKey *key);
static void ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan);
static RI_CompareHashEntry *ri_HashCompareOp(Oid eq_opr, Oid typeid);
+/* Reentrancy protection: prevent segfault on deleting a plan in execution if invalidated during reentrant RI check. */
+static void ri_PreparedPlanExecutionStarted(SPIPlanPtr plan);
+static void ri_PreparedPlanExecutionFinished(SPIPlanPtr plan);
+static bool ri_PreparedPlanCanRelease(SPIPlanPtr plan);
+
static void ri_CheckTrigger(FunctionCallInfo fcinfo, const char *funcname,
int tgkind);
static RI_ConstraintInfo *ri_FetchConstraintInfo(Trigger *trigger,
@@ -2724,6 +2741,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
save_sec_context | SECURITY_LOCAL_USERID_CHANGE |
SECURITY_NOFORCE_RLS);
+ /* Increase plan use count for reentrancy protection. */
+ ri_PreparedPlanExecutionStarted(qplan);
+
/*
* Finally we can run the query.
*
@@ -2735,6 +2755,9 @@ ri_PerformCheck(const RI_ConstraintInfo *riinfo,
vals, nulls,
test_snapshot, crosscheck_snapshot,
false, false, limit);
+
+ /* Decrease plan use count. this call can free the plan if it was invalidated and no longer in use. */
+ ri_PreparedPlanExecutionFinished(qplan);
/* Restore UID and security context */
SetUserIdAndSecContext(save_userid, save_sec_context);
@@ -3762,6 +3785,12 @@ ri_InitHashTables(void)
ri_compare_cache = hash_create("RI compare cache",
RI_INIT_QUERYHASHSIZE,
&ctl, HASH_ELEM | HASH_BLOBS);
+
+ ctl.keysize = sizeof(SPIPlanPtr);
+ ctl.entrysize = sizeof(RI_QueryPlanCacheExecutingRefCountEntry);
+ ri_query_plan_cache_executing_refcount = hash_create("RI plan cache execution refcount",
+ RI_INIT_QUERYHASHSIZE,
+ &ctl, HASH_ELEM | HASH_BLOBS);
}
@@ -3811,7 +3840,7 @@ ri_FetchPreparedPlan(RI_QueryKey *key)
* memory space before we make a new one.
*/
entry->plan = NULL;
- if (plan)
+ if (plan && ri_PreparedPlanCanRelease(plan))
SPI_freeplan(plan);
return NULL;
@@ -3847,6 +3876,61 @@ ri_HashPreparedPlan(RI_QueryKey *key, SPIPlanPtr plan)
}
+static void
+ri_PreparedPlanExecutionStarted(SPIPlanPtr plan)
+{
+ RI_QueryPlanCacheExecutingRefCountEntry* entry;
+ bool found;
+
+ if (!ri_query_plan_cache_executing_refcount)
+ ri_InitHashTables();
+
+ entry = (RI_QueryPlanCacheExecutingRefCountEntry*) hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_ENTER, &found);
+ if (found)
+ entry->refcount++;
+ else
+ entry->refcount = 1;
+}
+
+static void
+ri_PreparedPlanExecutionFinished(SPIPlanPtr plan)
+{
+ RI_QueryPlanCacheExecutingRefCountEntry* entry;
+ bool found;
+
+ if (!ri_query_plan_cache_executing_refcount)
+ return;
+
+ entry = (RI_QueryPlanCacheExecutingRefCountEntry*) hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_FIND, &found);
+ if (!entry)
+ return;
+
+ entry->refcount--;
+ if (entry->refcount == 0 && !SPI_plan_is_valid(plan))
+ {
+ // Remove the entry
+ hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_REMOVE, NULL);
+ SPI_freeplan(plan);
+ }
+}
+
+static bool
+ri_PreparedPlanCanRelease(SPIPlanPtr plan)
+{
+ RI_QueryPlanCacheExecutingRefCountEntry* entry;
+ bool found;
+
+ if (!ri_query_plan_cache_executing_refcount)
+ return true;
+
+ entry = (RI_QueryPlanCacheExecutingRefCountEntry*) hash_search(ri_query_plan_cache_executing_refcount, &plan, HASH_FIND, &found);
+ if (!entry)
+ return true;
+
+ return entry->refcount == 0;
+}
+
+
/*
* ri_KeysEqual -
*
--
2.34.1
[text/x-patch] 0001-Add-isolation-test-case-for-RI-plan-invalidation-cra.patch (5.1K, 4-0001-Add-isolation-test-case-for-RI-plan-invalidation-cra.patch)
download | inline diff:
From ae9ed37ccfa88c6874ba9114953f5032cc85d086 Mon Sep 17 00:00:00 2001
From: luquijeffrey <[email protected]>
Date: Tue, 19 May 2026 17:42:11 -0300
Subject: [PATCH 1/2] Add isolation test case for RI plan invalidation crash
---
.../isolation/expected/ri-cascade-del.out | 28 ++++++
src/test/isolation/isolation_schedule | 1 +
src/test/isolation/specs/ri-cascade-del.spec | 85 +++++++++++++++++++
3 files changed, 114 insertions(+)
create mode 100644 src/test/isolation/expected/ri-cascade-del.out
create mode 100644 src/test/isolation/specs/ri-cascade-del.spec
diff --git a/src/test/isolation/expected/ri-cascade-del.out b/src/test/isolation/expected/ri-cascade-del.out
new file mode 100644
index 00000000000..051083b17f8
--- /dev/null
+++ b/src/test/isolation/expected/ri-cascade-del.out
@@ -0,0 +1,28 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s2_lock s1_delete s2_inval s2_unlock s1_commit
+step s2_lock: SELECT pg_advisory_lock(1);
+pg_advisory_lock
+----------------
+
+(1 row)
+
+step s1_delete: DELETE FROM crash_reentrancia_tabla_autorederencial WHERE id = 1; <waiting ...>
+step s2_inval:
+ DO $$
+ BEGIN
+ FOR i IN 1..1000 LOOP
+ EXECUTE 'CREATE TEMPORARY TABLE t_temp_inval_(id INTEGER PRIMARY KEY)';
+ EXECUTE 'DROP TABLE t_temp_inval_';
+ END LOOP;
+ END;
+ $$;
+
+step s2_unlock: SELECT pg_advisory_unlock(1);
+pg_advisory_unlock
+------------------
+t
+(1 row)
+
+step s1_delete: <... completed>
+step s1_commit: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 1578ba191c8..39a0a1ee792 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -12,6 +12,7 @@ test: project-manager
test: classroom-scheduling
test: total-cash
test: referential-integrity
+test: ri-cascade-del
test: ri-trigger
test: partial-index
test: two-ids
diff --git a/src/test/isolation/specs/ri-cascade-del.spec b/src/test/isolation/specs/ri-cascade-del.spec
new file mode 100644
index 00000000000..aa8e090c7ad
--- /dev/null
+++ b/src/test/isolation/specs/ri-cascade-del.spec
@@ -0,0 +1,85 @@
+# Setup for referential integrity crash test
+setup
+{
+ CREATE TABLE crash_reentrancia_tabla_autorederencial (
+ id int PRIMARY KEY,
+ nombre text,
+ padre_id int REFERENCES crash_reentrancia_tabla_autorederencial(id) ON DELETE CASCADE
+ );
+
+ CREATE TABLE crash_reentrancia_segunda_tabla (
+ id int PRIMARY KEY,
+ valor text
+ );
+
+ CREATE OR REPLACE FUNCTION crash_reentrancia_before_delete()
+ RETURNS trigger AS $$
+ DECLARE
+ v_valor text;
+ BEGIN
+ IF OLD.id % 2 = 1 THEN
+ RETURN OLD;
+ END IF;
+
+ -- Wait for S2 to finish flooding the invalidation message queue
+ IF OLD.id = 2 THEN
+ PERFORM pg_advisory_lock(1);
+ PERFORM pg_advisory_unlock(1);
+ END IF;
+
+ IF OLD.id > 4 THEN
+ -- This opens the table and forces processing of pending inval messages
+ SELECT valor INTO v_valor FROM crash_reentrancia_segunda_tabla WHERE id = OLD.id;
+ END IF;
+
+ DELETE FROM crash_reentrancia_tabla_autorederencial WHERE padre_id = OLD.id;
+ RETURN OLD;
+ END;
+ $$ LANGUAGE plpgsql;
+
+ CREATE TRIGGER trg_crash_reentrancia_before_delete
+ BEFORE DELETE ON crash_reentrancia_tabla_autorederencial
+ FOR EACH ROW EXECUTE FUNCTION crash_reentrancia_before_delete();
+
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (1, 'A', NULL);
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (2, 'B', 1);
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (3, 'C', 2);
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (4, 'D', 3);
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (5, 'E', 4);
+ INSERT INTO crash_reentrancia_tabla_autorederencial VALUES (6, 'F', 5);
+
+ INSERT INTO crash_reentrancia_segunda_tabla VALUES
+ (1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e'), (6, 'f');
+}
+
+teardown
+{
+ DROP TRIGGER trg_crash_reentrancia_before_delete ON crash_reentrancia_tabla_autorederencial;
+ DROP FUNCTION crash_reentrancia_before_delete CASCADE;
+ DROP TABLE crash_reentrancia_tabla_autorederencial CASCADE;
+ DROP TABLE crash_reentrancia_segunda_tabla CASCADE;
+}
+
+session s1
+setup { BEGIN; }
+step s1_delete { DELETE FROM crash_reentrancia_tabla_autorederencial WHERE id = 1; }
+step s1_commit { COMMIT; }
+
+session s2
+step s2_lock { SELECT pg_advisory_lock(1); }
+step s2_inval {
+ DO $$
+ BEGIN
+ FOR i IN 1..1000 LOOP
+ EXECUTE 'CREATE TEMPORARY TABLE t_temp_inval_(id INTEGER PRIMARY KEY)';
+ EXECUTE 'DROP TABLE t_temp_inval_';
+ END LOOP;
+ END;
+ $$;
+}
+step s2_unlock { SELECT pg_advisory_unlock(1); }
+
+# Execution permutation
+# S2 locks -> S1 blocks on S2 -> S2 forces inval queue overflow -> S2 unlocks
+# S1 awakens -> S1 forces table_open -> invalidation processed -> segfault!
+permutation s2_lock s1_delete s2_inval s2_unlock s1_commit
--
2.34.1
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [PATCH] Add reentrancy guards in ri_triggers.c
In-Reply-To: <CAGHzy7RCXRo6iz1kL-p6g1r9x=EL-Yb7jPgLj4n5YEtYsugzJg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox