Hi hackers,
While reading at fmgr_security_definer() function implementation I noticed
that setup and teardown are not mirror images of each other:
GetUserIdAndSecContext(&save_userid, &save_sec_context);
save_nestlevel = NewGUCNestLevel(); /* if proconfig */
SetUserIdAndSecContext(fcache->userid, ...); /* if prosecdef */
apply proconfig;
fmgr_hook(FHET_START, ...);
... call the function ...
AtEOXact_GUC(true, save_nestlevel); /* if proconfig */
SetUserIdAndSecContext(save_userid, save_sec_context);
fmgr_hook(FHET_END, ...);
There are two asymmetries:
1. FHET_START is called with the function's user and proconfig in effect,
but FHET_END is called after both have been undone. A hook therefore sees
different state at END than it saw at START. Whatever a hook sets up at
START can't be undone at the same level at END, because the surrounding
state has already been unwound.
2. The GUC nest level is opened before the user switch but closed before
the user is restored. So the two scopes overlap instead of one nesting
inside the other.
The other places that switch the user and isolate GUC changes, such as
vacuum_rel(), do_analyze_rel(), index_build() and cluster_rel(), are
strictly nested:
SetUserIdAndSecContext(owner, ...);
save_nestlevel = NewGUCNestLevel();
...
AtEOXact_GUC(false, save_nestlevel);
SetUserIdAndSecContext(save_userid, save_sec_context);
The attached patch makes fmgr_security_definer() follow the same shape:
save -> switch user -> GUC level -> proconfig -> START
... call ...
END -> close GUC level -> restore user
- The user switch now happens before NewGUCNestLevel(). Proconfig is still
applied after the switch, so its behaviour doesn't change.
- FHET_END is now called before anything is undone, so it runs in the same
state as FHET_START.
- The error path is unchanged: FHET_ABORT is still called from PG_CATCH,
and (sub)transaction abort still cleans up.
Most commonly used fmgr_hook implementations (sepgsql, pg_hint_plan) only
maintain their own state and don't look at the user or GUCs at END, so
they're unaffected.
Thoughts?
--
Alexander Kukushkin
From befae8714d4472133f30bf15f9314188d2c52a46 Mon Sep 17 00:00:00 2001
From: Alexander Kukushkin <[email protected]>
Date: Wed, 4 Nov 2026 10:40:25 +0100
Subject: [PATCH] Make fmgr_security_definer() setup and teardown symmetric
fmgr_hook saw FHET_START with the function's user and proconfig in
effect but FHET_END after both had been reverted, and the GUC nest level
was opened outside the user switch but closed inside it. Switch the
user before opening the GUC nest level and call FHET_END before undoing
anything, so setup and teardown are strictly nested as in vacuum_rel()
and similar code.
---
src/backend/utils/fmgr/fmgr.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/src/backend/utils/fmgr/fmgr.c b/src/backend/utils/fmgr/fmgr.c
index b2fd6dbb73d..6abc47e9c52 100644
--- a/src/backend/utils/fmgr/fmgr.c
+++ b/src/backend/utils/fmgr/fmgr.c
@@ -703,15 +703,15 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
/* GetUserIdAndSecContext is cheap enough that no harm in a wasted call */
GetUserIdAndSecContext(&save_userid, &save_sec_context);
+ if (OidIsValid(fcache->userid))
+ SetUserIdAndSecContext(fcache->userid,
+ save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
+
if (fcache->configNames != NIL) /* Need a new GUC nesting level */
save_nestlevel = NewGUCNestLevel();
else
save_nestlevel = 0; /* keep compiler quiet */
- if (OidIsValid(fcache->userid))
- SetUserIdAndSecContext(fcache->userid,
- save_sec_context | SECURITY_LOCAL_USERID_CHANGE);
-
forthree(lc1, fcache->configNames,
lc2, fcache->configHandles,
lc3, fcache->configValues)
@@ -768,12 +768,12 @@ fmgr_security_definer(PG_FUNCTION_ARGS)
fcinfo->flinfo = save_flinfo;
+ if (fmgr_hook)
+ (*fmgr_hook) (FHET_END, &fcache->flinfo, &fcache->arg);
if (fcache->configNames != NIL)
AtEOXact_GUC(true, save_nestlevel);
if (OidIsValid(fcache->userid))
SetUserIdAndSecContext(save_userid, save_sec_context);
- if (fmgr_hook)
- (*fmgr_hook) (FHET_END, &fcache->flinfo, &fcache->arg);
return result;
}
--
2.34.1
