From 18575dfccfd0e966237f58c5de6589b9698c4aff Mon Sep 17 00:00:00 2001 From: Ben <52712273+BenSpex@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:52:06 +0200 Subject: [PATCH] Fix segfault on DETACH DELETE under RLS with a function-based edge policy A non-superuser subject to row-level security crashed the backend with SIGSEGV when running DETACH DELETE on a vertex that has a connected edge, if the edge label carried an RLS policy whose USING/WITH CHECK qual invokes a function (e.g. a STABLE tenant/owner accessor). Reported in issue #2474; reproduces on 1.7.0 and master. Mechanism: AGE deletes a vertex's connected edges in check_for_connected_edges(), which is called from end_cypher_delete() -- i.e. during executor shutdown (ExecEndPlan, reached via PortalCleanup -> ExecutorEnd when the portal is dropped). By that point the portal's active snapshot has already been popped, so the active-snapshot stack is empty. RLS for Cypher DELETE is enforced at the executor level (added in #2309): check_for_connected_edges() compiles the edge label's security quals and evaluates them per candidate edge with check_security_quals() -> ExecQual(). When the qual calls a SQL-language function, ExecQual() dispatches into fmgr_sql() -> postquel_start(), which runs the function's query and reads the current snapshot with GetActiveSnapshot(). With no snapshot on the stack that is a NULL-pointer dereference -> SIGSEGV (an assert build trips the Assert(ActiveSnapshotSet()) in postquel_start first). This is why the narrowing in the report holds: the vertex path (process_delete_list) runs during normal execution while a snapshot is active; edge-only DELETE and node-only DETACH DELETE never evaluate an edge qual at teardown; a superuser bypasses RLS; and a constant-only edge policy never enters fmgr_sql, so only DETACH DELETE of an edge-connected vertex under a function-bearing edge policy trips it. Fix: Ensure an active snapshot for the duration of the connected-edge scan in check_for_connected_edges(). es_snapshot is still valid there (the EState is not torn down until this returns) and is the correct snapshot for reading the edges, so push it if none is active and pop it before return. The scans themselves already pass es_snapshot explicitly, so this only affects RLS qual evaluation; non-RLS paths are unchanged. On the error path (e.g. an RLS denial raised mid-scan) transaction abort resets the active-snapshot stack, so the unpaired push is cleaned up. Test: Adds a regression to the security suite (PART 11b): an edge label policy whose qual calls a STABLE SQL function, then DETACH DELETE of an edge-connected vertex as the RLS-bound role. It must delete the vertex and its edge (leaving the other endpoint) instead of crashing. The existing PART 11 uses a constant-only edge policy, which does not exercise the snapshot-dependent function path. --- regress/expected/security.out | 94 ++++++++++++++++++++++++++++ regress/sql/security.sql | 75 ++++++++++++++++++++++ src/backend/executor/cypher_delete.c | 29 +++++++++ 3 files changed, 198 insertions(+) diff --git a/regress/expected/security.out b/regress/expected/security.out index 59e58cb05..521b76b62 100644 --- a/regress/expected/security.out +++ b/regress/expected/security.out @@ -1357,6 +1357,100 @@ $$) AS (a agtype); --- (0 rows) +-- ============================================================================ +-- PART 11b: DETACH DELETE with a function-based edge policy (issue #2474) +-- ============================================================================ +-- Regression for a SIGSEGV: an edge-label RLS policy whose USING / WITH CHECK +-- qual invokes a SQL-language function (e.g. a tenant/owner accessor) crashed +-- the backend on DETACH DELETE of an edge-connected vertex. The connected-edge +-- RLS check runs from end_cypher_delete() during executor shutdown, after the +-- portal's active snapshot has been popped; evaluating the qual then called the +-- function via fmgr_sql()/postquel_start(), which dereferences +-- GetActiveSnapshot() unconditionally -> NULL deref. A constant-only edge +-- policy (PART 11 above) never enters that path, so it did not crash. DETACH +-- DELETE must now complete and delete the vertex and its edge under the policy. +-- A STABLE SQL-language accessor (LANGUAGE sql is what exercises the +-- snapshot-dependent postquel executor path; a C/builtin function would not). +CREATE FUNCTION rls_detach_owner() RETURNS text + LANGUAGE sql STABLE AS $$ SELECT current_user::text $$; +ALTER TABLE rls_graph."Person" ENABLE ROW LEVEL SECURITY; +CREATE POLICY detach_fn_person_all ON rls_graph."Person" + FOR ALL USING (true) WITH CHECK (true); +ALTER TABLE rls_graph."KNOWS" ENABLE ROW LEVEL SECURITY; +CREATE POLICY detach_fn_knows_owner ON rls_graph."KNOWS" + FOR ALL + USING (properties->>'"owner"' = rls_detach_owner()) + WITH CHECK (properties->>'"owner"' = rls_detach_owner()); +-- Seed (as superuser, bypassing the WITH CHECK) an edge owned by rls_user1. +SELECT * FROM cypher('rls_graph', $$ + CREATE (:Person {name: 'DetachFn1', owner: 'rls_user1', department: 'DetachFn'}) +$$) AS (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('rls_graph', $$ + CREATE (:Person {name: 'DetachFn2', owner: 'rls_user1', department: 'DetachFn'}) +$$) AS (a agtype); + a +--- +(0 rows) + +SELECT * FROM cypher('rls_graph', $$ + MATCH (a:Person {name: 'DetachFn1'}), (b:Person {name: 'DetachFn2'}) + CREATE (a)-[:KNOWS {since: 2021, owner: 'rls_user1'}]->(b) +$$) AS (a agtype); + a +--- +(0 rows) + +SET ROLE rls_user1; +-- Must NOT crash: deletes DetachFn1 and its connected edge under a +-- function-based edge policy the role satisfies. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn1'}) DETACH DELETE p +$$) AS (a agtype); + a +--- +(0 rows) + +RESET ROLE; +-- DetachFn1 is gone. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn1'}) RETURN p.name +$$) AS (name agtype); + name +------ +(0 rows) + +-- DetachFn2 (the other endpoint) survives. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn2'}) RETURN p.name +$$) AS (name agtype); + name +------------- + "DetachFn2" +(1 row) + +-- The connecting edge is gone (no edge now points into DetachFn2). +SELECT * FROM cypher('rls_graph', $$ + MATCH ()-[k:KNOWS]->(b:Person {name: 'DetachFn2'}) RETURN k.since +$$) AS (since agtype); + since +------- +(0 rows) + +-- cleanup +DROP POLICY detach_fn_knows_owner ON rls_graph."KNOWS"; +DROP POLICY detach_fn_person_all ON rls_graph."Person"; +DROP FUNCTION rls_detach_owner(); +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person) WHERE p.department = 'DetachFn' DETACH DELETE p +$$) AS (a agtype); + a +--- +(0 rows) + -- ============================================================================ -- PART 12: Multiple Labels in Single Query -- ============================================================================ diff --git a/regress/sql/security.sql b/regress/sql/security.sql index 344dd23d4..65910cb5f 100644 --- a/regress/sql/security.sql +++ b/regress/sql/security.sql @@ -1179,6 +1179,81 @@ SELECT * FROM cypher('rls_graph', $$ MATCH (p:Person) WHERE p.department = 'Detach' DETACH DELETE p $$) AS (a agtype); +-- ============================================================================ +-- PART 11b: DETACH DELETE with a function-based edge policy (issue #2474) +-- ============================================================================ +-- Regression for a SIGSEGV: an edge-label RLS policy whose USING / WITH CHECK +-- qual invokes a SQL-language function (e.g. a tenant/owner accessor) crashed +-- the backend on DETACH DELETE of an edge-connected vertex. The connected-edge +-- RLS check runs from end_cypher_delete() during executor shutdown, after the +-- portal's active snapshot has been popped; evaluating the qual then called the +-- function via fmgr_sql()/postquel_start(), which dereferences +-- GetActiveSnapshot() unconditionally -> NULL deref. A constant-only edge +-- policy (PART 11 above) never enters that path, so it did not crash. DETACH +-- DELETE must now complete and delete the vertex and its edge under the policy. + +-- A STABLE SQL-language accessor (LANGUAGE sql is what exercises the +-- snapshot-dependent postquel executor path; a C/builtin function would not). +CREATE FUNCTION rls_detach_owner() RETURNS text + LANGUAGE sql STABLE AS $$ SELECT current_user::text $$; + +ALTER TABLE rls_graph."Person" ENABLE ROW LEVEL SECURITY; +CREATE POLICY detach_fn_person_all ON rls_graph."Person" + FOR ALL USING (true) WITH CHECK (true); + +ALTER TABLE rls_graph."KNOWS" ENABLE ROW LEVEL SECURITY; +CREATE POLICY detach_fn_knows_owner ON rls_graph."KNOWS" + FOR ALL + USING (properties->>'"owner"' = rls_detach_owner()) + WITH CHECK (properties->>'"owner"' = rls_detach_owner()); + +-- Seed (as superuser, bypassing the WITH CHECK) an edge owned by rls_user1. +SELECT * FROM cypher('rls_graph', $$ + CREATE (:Person {name: 'DetachFn1', owner: 'rls_user1', department: 'DetachFn'}) +$$) AS (a agtype); + +SELECT * FROM cypher('rls_graph', $$ + CREATE (:Person {name: 'DetachFn2', owner: 'rls_user1', department: 'DetachFn'}) +$$) AS (a agtype); + +SELECT * FROM cypher('rls_graph', $$ + MATCH (a:Person {name: 'DetachFn1'}), (b:Person {name: 'DetachFn2'}) + CREATE (a)-[:KNOWS {since: 2021, owner: 'rls_user1'}]->(b) +$$) AS (a agtype); + +SET ROLE rls_user1; + +-- Must NOT crash: deletes DetachFn1 and its connected edge under a +-- function-based edge policy the role satisfies. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn1'}) DETACH DELETE p +$$) AS (a agtype); + +RESET ROLE; + +-- DetachFn1 is gone. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn1'}) RETURN p.name +$$) AS (name agtype); + +-- DetachFn2 (the other endpoint) survives. +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person {name: 'DetachFn2'}) RETURN p.name +$$) AS (name agtype); + +-- The connecting edge is gone (no edge now points into DetachFn2). +SELECT * FROM cypher('rls_graph', $$ + MATCH ()-[k:KNOWS]->(b:Person {name: 'DetachFn2'}) RETURN k.since +$$) AS (since agtype); + +-- cleanup +DROP POLICY detach_fn_knows_owner ON rls_graph."KNOWS"; +DROP POLICY detach_fn_person_all ON rls_graph."Person"; +DROP FUNCTION rls_detach_owner(); +SELECT * FROM cypher('rls_graph', $$ + MATCH (p:Person) WHERE p.department = 'DetachFn' DETACH DELETE p +$$) AS (a agtype); + -- ============================================================================ -- PART 12: Multiple Labels in Single Query -- ============================================================================ diff --git a/src/backend/executor/cypher_delete.c b/src/backend/executor/cypher_delete.c index 19eca3ad6..e2161e66e 100644 --- a/src/backend/executor/cypher_delete.c +++ b/src/backend/executor/cypher_delete.c @@ -25,6 +25,7 @@ #include "miscadmin.h" #include "utils/acl.h" #include "utils/rls.h" +#include "utils/snapmgr.h" #include "catalog/ag_label.h" #include "executor/cypher_executor.h" @@ -689,6 +690,29 @@ static void check_for_connected_edges(CustomScanState *node) (cypher_delete_custom_scan_state *)node; EState *estate = css->css.ss.ps.state; char *graph_name = css->delete_data->graph_name; + bool pushed_snapshot = false; + + /* + * check_for_connected_edges() runs from end_cypher_delete(), i.e. during + * executor shutdown (ExecEndPlan), by which point the portal's active + * snapshot has already been popped. Evaluating an edge-label RLS policy + * whose USING/WITH CHECK qual invokes a function (e.g. a STABLE tenant + * accessor) requires an active snapshot: check_security_quals() -> + * ExecQual() -> fmgr_sql() -> postquel_start() runs the function's query + * with GetActiveSnapshot() and dereferences it unconditionally. With no + * snapshot on the stack that is a NULL dereference -> SIGSEGV (#2474). + * + * Ensure a snapshot is active for the duration of the scan. es_snapshot is + * still valid here (the EState is not torn down until after this returns), + * and is the correct snapshot for reading connected edges. If an error is + * raised mid-scan (e.g. an RLS denial), transaction abort resets the active + * snapshot stack, so the unpaired push on that path is cleaned up. + */ + if (!ActiveSnapshotSet()) + { + PushActiveSnapshot(estate->es_snapshot); + pushed_snapshot = true; + } /* scans each label from css->edge_labels */ foreach (lc, css->edge_labels) @@ -834,4 +858,9 @@ static void check_for_connected_edges(CustomScanState *node) destroy_entity_result_rel_info(resultRelInfo); } + + if (pushed_snapshot) + { + PopActiveSnapshot(); + } }