RI fastpath misses checking EXECUTE on functions
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253913psql -h localhost -U postgresBuilt from patchset v8 (message #8), October 05, 2026 at 11:53 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253913_8 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253913_8 && git checkout t253913_8Patchset v8 (message #8) is on t253913_8
Hi,
It was pointed out to me offlist by Nikolay Samokhvalov that the RI
fast path (ri_FastPathCheck) doesn't check EXECUTE privilege on the
functions it invokes on the FK values. There are two such functions:
the equality operator's function, which goes into the ScanKey the
index AM evaluates, and the implicit cast function, which is used when
the FK value must be converted to the PK column's type. In the SPI
path's case, because the operator appears in the generated query's
WHERE clause and the cast as $1::pktype, ExecutorStart checked EXECUTE
on both as part of initializing the query, as the referenced table's
owner. So while I added ri_CheckPermissions() to parallel the
executor's ExecCheckPermissions(), I missed the non-relation
permission checks that occur as part of expression initialization.
The attached patch adds ri_CheckFunctionPermissions(), which does
object_aclcheck(ProcedureRelationId, ..., ACL_EXECUTE) plus
InvokeFunctionExecuteHook() for each of the two functions, and calls
it once the fast-path metadata is populated. That puts the checks at
the same point as the existing relation permission check.
Regression tests are included for both functions. They reach the fast
path through per-row validation of a new constraint, and repeat each
case against a partitioned referenced table, which takes the SPI path,
to establish that the two behave identically.
I'll add an open item. I would like to get this committed by EOW if
there are no comments.
--
Thanks, Amit Langote
On 24/09/26 00:42, Amit Langote wrote:
Hi,
It was pointed out to me offlist by Nikolay Samokhvalov that the RI
fast path (ri_FastPathCheck) doesn't check EXECUTE privilege on the
functions it invokes on the FK values. There are two such functions:
the equality operator's function, which goes into the ScanKey the
index AM evaluates, and the implicit cast function, which is used when
the FK value must be converted to the PK column's type. In the SPI
path's case, because the operator appears in the generated query's
WHERE clause and the cast as $1::pktype, ExecutorStart checked EXECUTE
on both as part of initializing the query, as the referenced table's
owner. So while I added ri_CheckPermissions() to parallel the
executor's ExecCheckPermissions(), I missed the non-relation
permission checks that occur as part of expression initialization.The attached patch adds ri_CheckFunctionPermissions(), which does
object_aclcheck(ProcedureRelationId, ..., ACL_EXECUTE) plus
InvokeFunctionExecuteHook() for each of the two functions, and calls
it once the fast-path metadata is populated. That puts the checks at
the same point as the existing relation permission check.Regression tests are included for both functions. They reach the fast
path through per-row validation of a new constraint, and repeat each
case against a partitioned referenced table, which takes the SPI path,
to establish that the two behave identically.I'll add an open item. I would like to get this committed by EOW if
there are no comments.
I take a look on the patch, it looks good to me. Just a minor
comment:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
Attachments:
t253913_2v2-0001-Check-EXECUTE-privilege-on-functions-invoked-by-t.patchtext/plain; charset=UTF-8; name=v2-0001-Check-EXECUTE-privilege-on-functions-invoked-by-t.patchDownload+201-1
v2-0002-Check-EXECUTE-on-functions-invoked-by-the-batched.patchtext/plain; charset=UTF-8; name=v2-0002-Check-EXECUTE-on-functions-invoked-by-the-batched.patchDownload+37-1
Hi Matheus,
On Thu, Sep 24, 2026 at 11:45 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
On 24/09/26 00:42, Amit Langote wrote:
Hi,
It was pointed out to me offlist by Nikolay Samokhvalov that the RI
fast path (ri_FastPathCheck) doesn't check EXECUTE privilege on the
functions it invokes on the FK values. There are two such functions:
the equality operator's function, which goes into the ScanKey the
index AM evaluates, and the implicit cast function, which is used when
the FK value must be converted to the PK column's type. In the SPI
path's case, because the operator appears in the generated query's
WHERE clause and the cast as $1::pktype, ExecutorStart checked EXECUTE
on both as part of initializing the query, as the referenced table's
owner. So while I added ri_CheckPermissions() to parallel the
executor's ExecCheckPermissions(), I missed the non-relation
permission checks that occur as part of expression initialization.The attached patch adds ri_CheckFunctionPermissions(), which does
object_aclcheck(ProcedureRelationId, ..., ACL_EXECUTE) plus
InvokeFunctionExecuteHook() for each of the two functions, and calls
it once the fast-path metadata is populated. That puts the checks at
the same point as the existing relation permission check.Regression tests are included for both functions. They reach the fast
path through per-row validation of a new constraint, and repeat each
case against a partitioned referenced table, which takes the SPI path,
to establish that the two behave identically.I'll add an open item. I would like to get this committed by EOW if
there are no comments.I take a look on the patch, it looks good to me. Just a minor
comment:
Thanks for looking.
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?
That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).
I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.
I have attached a new version where I polished
ri_CheckFunctionPermissions()'s comment and the commit message. I
would like to commit it tomorrow if there are no more comments.
--
Thanks, Amit Langote
On 25/09/26 05:16, Amit Langote wrote:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.
Ok, make sense.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.
Ok, thanks for letting me know.
I have attached a new version where I polished
ri_CheckFunctionPermissions()'s comment and the commit message. I
would like to commit it tomorrow if there are no more comments.
Looks good to me.
--
Matheus Alcantara
EDB: https://www.enterprisedb.com
On Fri, Sep 25, 2026 at 8:45 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:
On 25/09/26 05:16, Amit Langote wrote:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.Ok, make sense.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.Ok, thanks for letting me know.
I have attached a new version where I polished
ri_CheckFunctionPermissions()'s comment and the commit message. I
would like to commit it tomorrow if there are no more comments.Looks good to me.
Thanks, pushed.
--
Thanks, Amit Langote
On Sat, 26 Sept 2026 at 07:16, Amit Langote <amitlangote09@gmail.com> wrote:
On Fri, Sep 25, 2026 at 8:45 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:On 25/09/26 05:16, Amit Langote wrote:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.Ok, make sense.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.Ok, thanks for letting me know.
I have attached a new version where I polished
ri_CheckFunctionPermissions()'s comment and the commit message. I
would like to commit it tomorrow if there are no more comments.Looks good to me.
Thanks, pushed.
--
Thanks, Amit Langote
HI!
Isn't ri_CheckFunctionPermissions missing in ri_FastPathBatchFlush ?
For this repro
CREATE SCHEMA s;
CREATE ROLE noproc NOLOGIN;
CREATE ROLE fkuser NOLOGIN;
CREATE FUNCTION s.eq(int4,int4) RETURNS bool AS 'int4eq'
LANGUAGE internal IMMUTABLE STRICT;
CREATE OPERATOR s.=== (LEFTARG=int4, RIGHTARG=int4, PROCEDURE=s.eq);
CREATE OPERATOR CLASS s.ops FOR TYPE int4 USING btree AS
OPERATOR 1 <, OPERATOR 2 <=, OPERATOR 3 s.===,
OPERATOR 4 >=, OPERATOR 5 >, FUNCTION 1 btint4cmp(int4,int4);
REVOKE EXECUTE ON FUNCTION s.eq(int4,int4) FROM PUBLIC;
CREATE TABLE pk(a int4);
CREATE UNIQUE INDEX ON pk (a s.ops);
CREATE TABLE fk(a int4);
ALTER TABLE fk ADD FOREIGN KEY (a) REFERENCES pk(a);
ALTER TABLE pk OWNER TO noproc;
ALTER TABLE fk OWNER TO fkuser;
GRANT SELECT ON pk TO fkuser;
INSERT INTO pk VALUES (1);
SET ROLE fkuser;
INSERT INTO fk VALUES (1);
I expect `ERROR: permission denied for function eq`, but on master
its executed without error.
This diff fixes issue
```
diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 2797ac4abea..2799e8aea99 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -3141,6 +3141,7 @@ ri_FastPathBatchFlush(RI_FastPathEntry *fpentry,
Relation fk_rel,
ri_populate_fastpath_metadata(riinfo, fk_rel, idx_rel);
}
Assert(riinfo->fpmeta);
+ ri_CheckFunctionPermissions(riinfo, riinfo->fpmeta); /*
* Take our own reference to the metadata for the duration of the flush.
```
--
Best regards,
Kirill Reshke
Hi Kirill,
On Tue, Sep 29, 2026 at 9:37 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
On Sat, 26 Sept 2026 at 07:16, Amit Langote <amitlangote09@gmail.com> wrote:
On Fri, Sep 25, 2026 at 8:45 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:On 25/09/26 05:16, Amit Langote wrote:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.Ok, make sense.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.
As I wrote here, I intend to revert this piece of code in master. I
haven't gotten around to it yet due to some personal circumstances at
the moment, but I aim to get it done by EOW.
Thanks for your attention and the patch nonetheless.
--
Thanks, Amit Langote
Hi,
On Sat, Sep 26, 2026 at 11:15 AM Amit Langote <amitlangote09@gmail.com> wrote:
On Fri, Sep 25, 2026 at 8:45 PM Matheus Alcantara
<matheusssilv97@gmail.com> wrote:On 25/09/26 05:16, Amit Langote wrote:
ri_CheckFunctionPermissions(riinfo, fpmeta) passes both when fpmeta
== riinfo->fpmeta. I'm wondering if we could just pass riinfo?That's just for consistency with build_index_scankeys(); it isn't
needed, so I don't feel strongly either way.Ok, make sense.
IIUC this patch only fix the case for FastPath without batching right?
Since batching is still on master, I'm wondering if we could also fix
it. See attached patch (v2-0001 is your v1-0001).I've left the batch code alone because I intend to revert it from
master too sometime next week. Thanks for the patch, though.Ok, thanks for letting me know.
I have attached a new version where I polished
ri_CheckFunctionPermissions()'s comment and the commit message. I
would like to commit it tomorrow if there are no more comments.Looks good to me.
Thanks, pushed.
A review of the fast-path code using Opus 5.5 found a potential NULL
de-reference after this commit. ri_FastPathCheck() reads
riinfo->fpmeta again after ri_CheckFunctionPermissions(), whose
catalog lookups can process an invalidation that detaches the metadata
and sets riinfo->fpmeta to NULL.
The attached patch keeps a local pointer, as
InvalidateConstraintCacheCallBack() already assumes callers do. It
applies to both master and REL_19_STABLE. Also, there's no batching
code anymore to keep in sync as of last Saturday [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=425daf545d9146e008220ae0b21415982220cd3f.
--
Thanks, Amit Langote