Horton

Thread

Commits

  1. Fix sepgsql expected test output

  2. Fix ExecCheckPermissions call in RI_Initial_Check

  1. A bug with ExecCheckPermissions

    Oleg Tselebrovskiy <o.tselebrovskiy@postgrespro.ru> — 2023-02-08T08:21:03Z

    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

  2. Re: A bug with ExecCheckPermissions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-02-08T10:49:00Z

    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/

  3. Re: A bug with ExecCheckPermissions

    Amit Langote <amitlangote09@gmail.com> — 2023-02-08T11:09:38Z

    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

  4. Re: A bug with ExecCheckPermissions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-02-08T18:23:18Z

    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)

  5. Re: A bug with ExecCheckPermissions

    Sergey Shinderuk <s.shinderuk@postgrespro.ru> — 2023-02-09T09:14:44Z

    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/

  6. Re: A bug with ExecCheckPermissions

    Amit Langote <amitlangote09@gmail.com> — 2023-02-09T09:30:15Z

    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

  7. Re: A bug with ExecCheckPermissions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-03-09T10:39:38Z

    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)

  8. Re: A bug with ExecCheckPermissions

    Amit Langote <amitlangote09@gmail.com> — 2023-03-09T10:56:58Z

    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

  9. Re: A bug with ExecCheckPermissions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-04T17:59:17Z

    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/

  10. Re: A bug with ExecCheckPermissions

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-05-04T22:11:49Z

    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

  11. Re: A bug with ExecCheckPermissions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-05-05T09:15:36Z

    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