Thread: 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 Tselebrovskii

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/

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)

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/

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)

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

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/

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

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