Re: A bug with ExecCheckPermissions
On 2023-May-04, Tom Lane wrote: > It looks like this patch caused a change in the order of output from > the sepgsql tests [1]. If you expected it to re-order permissions > checking then this is probably fine, and we should just update the > expected output. Yeah, looks correct. Fix pushed. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Every machine is a smoke machine if you operate it wrong enough." https://twitter.com/libseybieda/status/1541673325781196801
Re: A bug with ExecCheckPermissions
Alvaro Herrera writes: > Thanks for looking! I have pushed it now. And many thanks to Oleg for > noticing and reporting it. It looks like this patch caused a change in the order of output from the sepgsql tests [1]. If you expected it to re-order permissions checking then this is probably fine, and we should just update the expected output. regards, tom lane [1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2023-05-04%2018%3A52%3A12
Re: A bug with ExecCheckPermissions
On 2023-Mar-09, Amit Langote wrote: > On Thu, Mar 9, 2023 at 7:39 PM Alvaro Herrera wrote: > > I didn't like very much this business of setting the perminfoindex > > directly to '2' and '1'. It looks ugly with no explanation. What do > > you think of creating the as we go along and set each index > > correspondingly, as in the attached? > > Agree it looks cleaner and self-explanatory that way. Thanks. Thanks for looking! I have pushed it now. And many thanks to Oleg for noticing and reporting it. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Re: A bug with ExecCheckPermissions
Hi Alvaro, On Thu, Mar 9, 2023 at 7:39 PM Alvaro Herrera wrote: > I didn't like very much this business of setting the perminfoindex > directly to '2' and '1'. It looks ugly with no explanation. What do > you think of creating the as we go along and set each index > correspondingly, as in the attached? Agree it looks cleaner and self-explanatory that way. Thanks. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
I didn't like very much this business of setting the perminfoindex
directly to '2' and '1'. It looks ugly with no explanation. What do
you think of creating the as we go along and set each index
correspondingly, as in the attached?
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Ellos andaban todos desnudos como su madre los parió, y también las mujeres,
aunque no vi más que una, harto moza, y todos los que yo vi eran todos
mancebos, que ninguno vi de edad de más de XXX años" (Cristóbal Colón)
>From f0041bf1ac2f0c727d6b8495a63a548240c3b9c5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Thu, 9 Mar 2023 11:33:18 +0100
Subject: [PATCH] fix ExecCheckPermissions call in RI_Initial_Check
---
src/backend/executor/execMain.c | 22
src/backend/utils/adt/ri_triggers.c | 40 -
2 files changed, 44 insertions(+), 18 deletions(-)
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index b32f419176..6230f5e3d4 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -577,6 +577,28 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
ListCell *l;
bool result = true;
+#ifdef USE_ASSERT_CHECKING
+ Bitmapset *indexset = NULL;
+
+ /* Check that rteperminfos is consistent with rangeTable */
+ foreach(l, rangeTable)
+ {
+ RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);
+
+ if (rte->perminfoindex != 0)
+ {
+ /* Sanity checks */
+ (void) getRTEPermissionInfo(rteperminfos, rte);
+ /* Many-to-one mapping not allowed */
+ Assert(!bms_is_member(rte->perminfoindex, indexset));
+ indexset = bms_add_member(indexset, rte->perminfoindex);
+ }
+ }
+
+ /* All rteperminfos are referenced */
+ Assert(bms_num_members(indexset) == list_length(rteperminfos));
+#endif
+
foreach(l, rteperminfos)
{
RTEPermissionInfo *perminfo = lfirst_node(RTEPermissionInfo, l);
diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 375b17b9f3..b4e789899e 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -1373,10 +1373,11 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
char fkrelname[MAX_QUOTED_REL_NAME_LEN];
char pkattname[MAX_QUOTED_NAME_LEN + 3];
char fkattname[MAX_QUOTED_NAME_LEN + 3];
- RangeTblEntry *pkrte;
- RangeTblEntry *fkrte;
+ RangeTblEntry *rte;
RTEPermissionInfo *pk_perminfo;
RTEPermissionInfo *fk_perminfo;
+ List *rtes = NIL;
+ List *perminfos = NIL;
const char *sep;
const char *fk_only;
const char *pk_only;
@@ -1394,25 +1395,29 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
*
* XXX are there any other show-stopper conditions to check?
*/
- pkrte = makeNode(RangeTblEntry);
- pkrte->rtekind = RTE_RELATION;
- pkrte->relid = RelationGetRelid(pk_rel);
- pkrte->relkind = pk_rel->rd_rel->relkind;
- pkrte->rellockmode = AccessShareLock;
-
pk_perminfo = makeNode(RTEPermissionInfo);
pk_perminfo->relid = RelationGetRelid(pk_rel);
pk_perminfo->requiredPerms = ACL_SELECT;
-
- fkrte = makeNode(RangeTblEntry);
- fkrte->rtekind = RTE_RELATION;
- fkrte->relid = RelationGetRelid(fk_rel);
- fkrte->relkind = fk_rel->rd_rel->relkind;
- fkrte->rellockmode = AccessShareLock;
+ perminfos = lappend(perminfos, pk_perminfo);
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = RelationGetRelid(pk_rel);
+ rte->relkind = pk_rel->rd_rel->relkind;
+ rte->rellockmode = AccessShareLock;
+ rte->perminfoindex = list_length(perminfos);
+ rtes = lappend(rtes, rte);
fk_perminfo = makeNode(RTEPermissionInfo);
fk_perminfo->relid = RelationGetRelid(fk_rel);
fk_perminfo->requiredPerms = ACL_SELECT;
+ perminfos = lappend(perminfos, fk_perminfo);
+ rte = makeNode(RangeTblEntry);
+ rte->rtekind = RTE_RELATION;
+ rte->relid = RelationGetRelid(fk_rel);
+ rte->relkind = fk_rel->rd_rel->relkind;
+ rte->rellockmode = AccessShareLock;
+ rte->perminfoindex = list_length(perminfos);
+ rtes = lappend(rtes, rte);
for (int i = 0; i < riinfo->nkeys; i++)
{
@@ -1425,8 +1430,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
fk_perminfo->selectedCols = bms_add_member(fk_perminfo->selectedCols, attno);
}
- if (!ExecCheckPermissions(list_make2(fkrte, pkrte),
- list_make2(fk_perminfo, pk_perminfo), false))
+ if (!ExecCheckPermissions(rtes, perminfos, false))
return false;
/*
@@ -1436,9 +1440,9 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
*/
if (!has_bypassrls_privilege(GetUserId()) &&
((pk_rel->rd_rel->relrowsecurity &&
- !object_ownercheck(RelationRelationId, pkrte->relid, GetUserId())) ||
+ !object_ownercheck(RelationRelationId, RelationGetRelid(pk_rel), GetUserId())) ||
(fk_rel->rd_rel->relrowsecurity &&
- !object_ownercheck(RelationRelationId, fkrte->relid, GetUserId()
+
Re: A bug with ExecCheckPermissions
Hi, On Thu, Feb 9, 2023 at 14:44 Sergey Shinderuk wrote: > On 08.02.2023 21:23, Alvaro Herrera wrote: > > On 2023-Feb-08, Amit Langote wrote: > > > >> On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera > wrote: > > > >>> I think we should also patch ExecCheckPermissions to use forboth(), > >>> scanning the RTEs as it goes over the perminfos, and make sure that the > >>> entries are consistent. > >> > >> Hmm, we can’t use forboth here, because not all RTEs have the > corresponding > >> RTEPermissionInfo, inheritance children RTEs, for example. > > > > Doh, of course. > > > >> Also, it doesn’t make much sense to reinstate the original loop over > >> range table and fetch the RTEPermissionInfo for the RTEs with non-0 > >> perminfoindex, because the main goal of the patch was to make > >> ExecCheckPermissions() independent of range table length. > > > > Yeah, I'm thinking in a mechanism that would allow us to detect bugs in > > development builds — no need to have it run in production builds. > > However, I can't see any useful way to implement it. > > > > > Maybe something like the attached would do? Thanks for the patch. Something like this makes sense to me. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
On 08.02.2023 21:23, Alvaro Herrera wrote:
On 2023-Feb-08, Amit Langote wrote:
On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera wrote:
I think we should also patch ExecCheckPermissions to use forboth(),
scanning the RTEs as it goes over the perminfos, and make sure that the
entries are consistent.
Hmm, we can’t use forboth here, because not all RTEs have the corresponding
RTEPermissionInfo, inheritance children RTEs, for example.
Doh, of course.
Also, it doesn’t make much sense to reinstate the original loop over
range table and fetch the RTEPermissionInfo for the RTEs with non-0
perminfoindex, because the main goal of the patch was to make
ExecCheckPermissions() independent of range table length.
Yeah, I'm thinking in a mechanism that would allow us to detect bugs in
development builds — no need to have it run in production builds.
However, I can't see any useful way to implement it.
Maybe something like the attached would do?
--
Sergey Shinderukhttps://postgrespro.com/
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 39bfb48dc22..94866743f48 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -577,6 +577,28 @@ ExecCheckPermissions(List *rangeTable, List *rteperminfos,
ListCell *l;
boolresult = true;
+#ifdef USE_ASSERT_CHECKING
+ Bitmapset *indexset = NULL;
+
+ /* Check that rteperminfos is consistent with rangeTable */
+ foreach(l, rangeTable)
+ {
+ RangeTblEntry *rte = lfirst_node(RangeTblEntry, l);
+
+ if (rte->perminfoindex != 0)
+ {
+ /* Sanity checks */
+ (void) getRTEPermissionInfo(rteperminfos, rte);
+ /* Many-to-one mapping not allowed */
+ Assert(!bms_is_member(rte->perminfoindex, indexset));
+ indexset = bms_add_member(indexset, rte->perminfoindex);
+ }
+ }
+
+ /* All rteperminfos are referenced */
+ Assert(bms_num_members(indexset) == list_length(rteperminfos));
+#endif
+
foreach(l, rteperminfos)
{
RTEPermissionInfo *perminfo = lfirst_node(RTEPermissionInfo, l);
diff --git a/src/backend/utils/adt/ri_triggers.c
b/src/backend/utils/adt/ri_triggers.c
index 375b17b9f3b..f6d19226a0a 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -1399,6 +1399,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel,
Relation pk_rel)
pkrte->relid = RelationGetRelid(pk_rel);
pkrte->relkind = pk_rel->rd_rel->relkind;
pkrte->rellockmode = AccessShareLock;
+ pkrte->perminfoindex = 2;
pk_perminfo = makeNode(RTEPermissionInfo);
pk_perminfo->relid = RelationGetRelid(pk_rel);
@@ -1409,6 +1410,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel,
Relation pk_rel)
fkrte->relid = RelationGetRelid(fk_rel);
fkrte->relkind = fk_rel->rd_rel->relkind;
fkrte->rellockmode = AccessShareLock;
+ fkrte->perminfoindex = 1;
fk_perminfo = makeNode(RTEPermissionInfo);
fk_perminfo->relid = RelationGetRelid(fk_rel);
Re: A bug with ExecCheckPermissions
On 2023-Feb-08, Amit Langote wrote: > On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera wrote: > > I think we should also patch ExecCheckPermissions to use forboth(), > > scanning the RTEs as it goes over the perminfos, and make sure that the > > entries are consistent. > > Hmm, we can’t use forboth here, because not all RTEs have the corresponding > RTEPermissionInfo, inheritance children RTEs, for example. Doh, of course. > Also, it doesn’t make much sense to reinstate the original loop over > range table and fetch the RTEPermissionInfo for the RTEs with non-0 > perminfoindex, because the main goal of the patch was to make > ExecCheckPermissions() independent of range table length. Yeah, I'm thinking in a mechanism that would allow us to detect bugs in development builds — no need to have it run in production builds. However, I can't see any useful way to implement it. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "Estoy de acuerdo contigo en que la verdad absoluta no existe... El problema es que la mentira sí existe y tu estás mintiendo" (G. Lama)
Re: A bug with ExecCheckPermissions
On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera wrote: > On 2023-Feb-08, o.tselebrovs...@postgrespro.ru wrote: > > > But if you debug function ExecCheckPermissions and look into what is > passed > > to function (contents of rangeTable and rteperminfos to be exact), > > you'll see some strange behaviour: > > > Both of RangeTableEntries have a perminfoindex of 0 and simultaneously > have > > a RTEPERMISSIONINFO entry for them! > > Ouch. Yeah, that's not great. As you say, it doesn't really affect > anything, and we know full well that these RTEs are ad-hoc > manufactured. But as we claim that we still pass the RTEs for the > benefit of hooks, then we should at least make them match. +1. We don’t have anything in this (core) code path that would try to use perminfoindex for these RTEs, but there might well be in the future. I think we should also patch ExecCheckPermissions to use forboth(), > scanning the RTEs as it goes over the perminfos, and make sure that the > entries are consistent. Hmm, we can’t use forboth here, because not all RTEs have the corresponding RTEPermissionInfo, inheritance children RTEs, for example. Also, it doesn’t make much sense to reinstate the original loop over range table and fetch the RTEPermissionInfo for the RTEs with non-0 perminfoindex, because the main goal of the patch was to make ExecCheckPermissions() independent of range table length. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Re: A bug with ExecCheckPermissions
On 2023-Feb-08, o.tselebrovs...@postgrespro.ru wrote: > But if you debug function ExecCheckPermissions and look into what is passed > to function (contents of rangeTable and rteperminfos to be exact), > you'll see some strange behaviour: > Both of RangeTableEntries have a perminfoindex of 0 and simultaneously have > a RTEPERMISSIONINFO entry for them! Ouch. Yeah, that's not great. As you say, it doesn't really affect anything, and we know full well that these RTEs are ad-hoc manufactured. But as we claim that we still pass the RTEs for the benefit of hooks, then we should at least make them match. I think we should also patch ExecCheckPermissions to use forboth(), scanning the RTEs as it goes over the perminfos, and make sure that the entries are consistent. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
A bug with ExecCheckPermissions
Greetings, everyone!
While working on an extension my colleague and I have found an
interesting case;
When you try to execute next SQL statements on master branch of
PostgreSQL:
CREATE TABLE parted_fk_naming (
id bigint NOT NULL default 1,
id_abc bigint,
CONSTRAINT dummy_constr FOREIGN KEY (id_abc)
REFERENCES parted_fk_naming (id),
PRIMARY KEY (id)
)
PARTITION BY LIST (id);
CREATE TABLE parted_fk_naming_1 (
id bigint NOT NULL default 1,
id_abc bigint,
PRIMARY KEY (id),
CONSTRAINT dummy_constr CHECK (true)
);
ALTER TABLE parted_fk_naming ATTACH PARTITION parted_fk_naming_1 FOR
VALUES IN ('1');
seemingly nothing suspicious happens.
But if you debug function ExecCheckPermissions and look into what is
passed to function (contents of rangeTable and rteperminfos to be
exact),
you'll see some strange behaviour:
(
{RANGETBLENTRY
:alias <>
:eref <>
:rtekind 0
:relid 16395
:relkind r
:rellockmode 1
:tablesample <>
:perminfoindex 0
:lateral false
:inh false
:inFromCl false
:securityQuals <>
}
{RANGETBLENTRY
:alias <>
:eref <>
:rtekind 0
:relid 16384
:relkind p
:rellockmode 1
:tablesample <>
:perminfoindex 0
:lateral false
:inh false
:inFromCl false
:securityQuals <>
}
)
(
{RTEPERMISSIONINFO
:relid 16395
:inh false
:requiredPerms 2
:checkAsUser 0
:selectedCols (b 9)
:insertedCols (b)
:updatedCols (b)
}
{RTEPERMISSIONINFO
:relid 16384
:inh false
:requiredPerms 2
:checkAsUser 0
:selectedCols (b 8)
:insertedCols (b)
:updatedCols (b)
}
)
Both of RangeTableEntries have a perminfoindex of 0 and simultaneously
have a RTEPERMISSIONINFO entry for them!
Right now this behaviour isn't affecting anything, but in future should
someone want to use ExecutorCheckPerms_hook from
/src/backend/executor/execMain.c, its input parameters
won't correspond to each other since members of rangeTable will have
incorrect perminfoindex.
To fix this, we're setting fk's index to 1 and pk's index to 2 in
/src/backend/utils/adt/ri_triggers.c so that list being passed to
ExecCheckPermissions and its hook
has indexes for corresponding rteperminfos entries. 1 and 2 are chosen
because perminfoindex is 1-based and fk is passed to list_make2 first;
We are eager to hear some thoughts from the community!
Regards,
Oleg Tselebrovskiidiff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c
index 29f29d664b6..7666886742a 100644
--- a/src/backend/utils/adt/ri_triggers.c
+++ b/src/backend/utils/adt/ri_triggers.c
@@ -1399,6 +1399,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
pkrte->relid = RelationGetRelid(pk_rel);
pkrte->relkind = pk_rel->rd_rel->relkind;
pkrte->rellockmode = AccessShareLock;
+ pkrte->perminfoindex = 2;
pk_perminfo = makeNode(RTEPermissionInfo);
pk_perminfo->relid = RelationGetRelid(pk_rel);
@@ -1409,6 +1410,7 @@ RI_Initial_Check(Trigger *trigger, Relation fk_rel, Relation pk_rel)
fkrte->relid = RelationGetRelid(fk_rel);
fkrte->relkind = fk_rel->rd_rel->relkind;
fkrte->rellockmode = AccessShareLock;
+ fkrte->perminfoindex = 1;
fk_perminfo = makeNode(RTEPermissionInfo);
fk_perminfo->relid = RelationGetRelid(fk_rel);
