pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedA bug with ExecCheckPermissions
11+ messages / 5 participants
[nested] [flat]
Commits from this thread:
2023-05-05 95f5a7a42c Fix sepgsql expected test output
2023-05-04 f75cec4fff Fix ExecCheckPermissions call in RI_Initial_Check
* A bug with ExecCheckPermissions
@ 2023-02-08 08:21 o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: o.tselebrovskiy@postgrespro.ru @ 2023-02-08 08:21 UTC (permalink / raw)
To: pgsql-hackers; +Cc: s.shinderuk@postgrespro.ru
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 Tselebrovskii
Attachments:
missing-perminfoindex.diff (text/x-diff, 893B) download
diff --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);
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
@ 2023-02-08 10:49 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2023-02-08 10:49 UTC (permalink / raw)
To: o.tselebrovskiy@postgrespro.ru; +Cc: pgsql-hackers@postgresql.org, s.shinderuk@postgrespro.ru, Amit Langote <amitlangote09@gmail.com>
On 2023-Feb-08, o.tselebrovskiy@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/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-02-08 11:09 ` Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2023-02-08 11:09 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: o.tselebrovskiy@postgrespro.ru; pgsql-hackers; s.shinderuk@postgrespro.ru
On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2023-Feb-08, o.tselebrovskiy@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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
@ 2023-02-08 18:23 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2023-02-08 18:23 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: o.tselebrovskiy@postgrespro.ru; pgsql-hackers; s.shinderuk@postgrespro.ru
On 2023-Feb-08, Amit Langote wrote:
> On Wed, Feb 8, 2023 at 16:19 Alvaro Herrera <alvherre@alvh.no-ip.org> 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 Herrera Breisgau, 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)
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-02-09 09:14 ` Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Sergey Shinderuk @ 2023-02-09 09:14 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; Amit Langote <amitlangote09@gmail.com>; +Cc: o.tselebrovskiy@postgrespro.ru; pgsql-hackers
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 <alvherre@alvh.no-ip.org> 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 Shinderuk https://postgrespro.com/
Attachments:
check-rteperminfos.diff (text/plain, 1.9K) download
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;
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 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
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
@ 2023-02-09 09:30 ` Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2023-02-09 09:30 UTC (permalink / raw)
To: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; +Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
Hi,
On Thu, Feb 9, 2023 at 14:44 Sergey Shinderuk <s.shinderuk@postgrespro.ru>
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 <alvherre@alvh.no-ip.org>
> 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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
@ 2023-03-09 10:39 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-09 10:56 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2023-03-09 10:39 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
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)
Attachments:
0001-fix-ExecCheckPermissions-call-in-RI_Initial_Check.patch (text/x-diff, 4.5K) download
From f0041bf1ac2f0c727d6b8495a63a548240c3b9c5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
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()))))
+ !object_ownercheck(RelationRelationId, RelationGetRelid(fk_rel), GetUserId()))))
return false;
/*----------
--
2.30.2
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-03-09 10:56 ` Amit Langote <amitlangote09@gmail.com>
2023-05-04 17:59 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Amit Langote @ 2023-03-09 10:56 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
Hi Alvaro,
On Thu, Mar 9, 2023 at 7:39 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-09 10:56 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
@ 2023-05-04 17:59 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-05-04 22:11 ` Re: A bug with ExecCheckPermissions Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Alvaro Herrera @ 2023-05-04 17:59 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Sergey Shinderuk <s.shinderuk@postgrespro.ru>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
On 2023-Mar-09, Amit Langote wrote:
> On Thu, Mar 9, 2023 at 7:39 PM Alvaro Herrera <alvherre@alvh.no-ip.org> 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/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-09 10:56 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-05-04 17:59 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2023-05-04 22:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-05-05 09:15 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2023-05-04 22:11 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Amit Langote <amitlangote09@gmail.com>; Sergey Shinderuk <s.shinderuk@postgrespro.ru>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
Alvaro Herrera <alvherre@alvh.no-ip.org> 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&dt=2023-05-04%2018%3A52%3A12
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: A bug with ExecCheckPermissions
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Re: A bug with ExecCheckPermissions Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-09 10:56 ` Re: A bug with ExecCheckPermissions Amit Langote <amitlangote09@gmail.com>
2023-05-04 17:59 ` Re: A bug with ExecCheckPermissions Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-05-04 22:11 ` Re: A bug with ExecCheckPermissions Tom Lane <tgl@sss.pgh.pa.us>
@ 2023-05-05 09:15 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2023-05-05 09:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Amit Langote <amitlangote09@gmail.com>; Sergey Shinderuk <s.shinderuk@postgrespro.ru>; o.tselebrovskiy@postgrespro.ru; pgsql-hackers
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 Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-05-05 09:15 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-02-08 08:21 A bug with ExecCheckPermissions o.tselebrovskiy@postgrespro.ru
2023-02-08 10:49 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-08 11:09 ` Amit Langote <amitlangote09@gmail.com>
2023-02-08 18:23 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-02-09 09:14 ` Sergey Shinderuk <s.shinderuk@postgrespro.ru>
2023-02-09 09:30 ` Amit Langote <amitlangote09@gmail.com>
2023-03-09 10:39 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-03-09 10:56 ` Amit Langote <amitlangote09@gmail.com>
2023-05-04 17:59 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2023-05-04 22:11 ` Tom Lane <tgl@sss.pgh.pa.us>
2023-05-05 09:15 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox