Horton

Thread

Commits

  1. Allow access to child table statistics if user can read parent table.

  2. Mark built-in btree comparison functions as leakproof where it's safe.

  1. CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-10T22:31:15Z

    I poked into the odd behavior reported in bug #15251:

https://www.postgresql.org/message-id/152967245839.1266.6939666809369185595@wrigleys.postgresql.org

The reason for the poor plan chosen when the caller hasn't got select
privilege on the child table is that statistic_proc_security_check()
decides not to allow use of the stats for the child table.  There are
two functions for which it decides that, because they aren't leakproof:
textregexeq() and btint4cmp().  Now, it's probably necessary that we
consider textregexeq() leaky, since it might spit up errors about its
pattern argument.  But btint4cmp?  It seems pretty silly that the
integer comparison operators are marked leakproof while their underlying
comparison function isn't.

(The reason we're hitting this is that calc_arraycontsel() finds the
datatype's default btree comparison function and tries to use that
to estimate selectivity.  While marking btint4cmp leakproof doesn't
completely fix the misestimation, it goes a long way in this example.)

I propose to run through the system operator classes, find any for which
the comparison function isn't marked leakproof but the operators are,
and fix them.  This is clearly appropriate for HEAD and maybe it's not
too late to force an initdb for v11 --- thoughts?

Another question that could be raised is why we are refusing to use
stats for a child table when the caller has select on the parent.
It's completely trivial to extract data from a child table if you
have select on the parent, so it seems like we are checking the
wrong table's privileges.

			regards, tom lane

  2. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-10T22:44:21Z

    On 2018-Jul-10, Tom Lane wrote:

> I propose to run through the system operator classes, find any for which
> the comparison function isn't marked leakproof but the operators are,
> and fix them.  This is clearly appropriate for HEAD and maybe it's not
> too late to force an initdb for v11 --- thoughts?

on initdb in v11, see
https://postgr.es/m/CAKJS1f9cqoSKS9JVcBKGa2mdn-24YPWc9XLzFDNsmjJMUpth1w@mail.gmail.com

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  3. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-10T22:58:05Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> on initdb in v11, see
> https://postgr.es/m/CAKJS1f9cqoSKS9JVcBKGa2mdn-24YPWc9XLzFDNsmjJMUpth1w@mail.gmail.com

[confused...]  I recall a thread mentioning that we might need initdb
in v11, but that one doesn't seem to be it?

			regards, tom lane

  4. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-11T22:52:00Z

    I wrote:
> I propose to run through the system operator classes, find any for which
> the comparison function isn't marked leakproof but the operators are,
> and fix them.  This is clearly appropriate for HEAD and maybe it's not
> too late to force an initdb for v11 --- thoughts?

I did that for the built-in btree opclasses.  I decided that it's probably
not worth forcing an initdb in v11 for, though.  In principle, losing
selectivity estimates because of non-leakproof functions should only
happen in queries that are going to fail at runtime anyway.  The real
problem that ought to be addressed and perhaps back-patched is this:

> Another question that could be raised is why we are refusing to use
> stats for a child table when the caller has select on the parent.
> It's completely trivial to extract data from a child table if you
> have select on the parent, so it seems like we are checking the
> wrong table's privileges.

			regards, tom lane

  5. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-07-14T11:14:21Z

    On 12.07.18 00:52, Tom Lane wrote:
>> Another question that could be raised is why we are refusing to use
>> stats for a child table when the caller has select on the parent.
>> It's completely trivial to extract data from a child table if you
>> have select on the parent, so it seems like we are checking the
>> wrong table's privileges.

That seems like an oversight.

The underlying principle is that we want to allow access to statistics
if the user could read the table, or more accurately the column, anyway.
 This could also happen through inheritance, so we should check that as
well, but we need to make sure that the particular column is inherited
and not added locally.  Also, for the expression index case, we don't
track the individual columns, so we don't have that information.  For
partitioning, we can rely on all the columns being inherited, but not
for plain inheritance.  So there are some details to work through, it seems.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  6. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-14T14:29:23Z

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> ... For
> partitioning, we can rely on all the columns being inherited, but not
> for plain inheritance.

Uh, what?

			regards, tom lane

  7. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2018-10-22T14:03:03Z

    On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> > ... For
> > partitioning, we can rely on all the columns being inherited, but not
> > for plain inheritance.
>
> Uh, what?

Maybe he meant that partitioning doesn't allow locally defined columns
in children, but plain inheritance does.  Btw, Peter also said this
earlier in the paragraph:

"This could also happen through inheritance, so we should check that as
well, but we need to make sure that the particular column is inherited
and not added locally."

But maybe for the case under question, that's irrelevant, because
we're only interested in access to inherited columns as those are the
only ones that can be accessed in queries via parent.

Thanks,
Amit

  8. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-10-22T14:17:31Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>>> ... For
>>> partitioning, we can rely on all the columns being inherited, but not
>>> for plain inheritance.

>> Uh, what?

> But maybe for the case under question, that's irrelevant, because
> we're only interested in access to inherited columns as those are the
> only ones that can be accessed in queries via parent.

Yeah, that's what I thought.  It seems like it should be possible to base
all stats access decisions off the table actually named in the query,
because only columns appearing in that table could be referenced, and only
that table's permissions actually get checked at runtime.

I guess it's possible that a child table could have, say, an index on
column X (inherited) and column Y (local) and that some aspect of costing
might then be interested in the behavior of column Y, even though the
query could only mention X not Y.  But then we could fall back to the
existing behavior.

			regards, tom lane

  9. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2018-10-25T10:54:04Z

    On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Sat, Jul 14, 2018 at 11:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> >>> ... For
> >>> partitioning, we can rely on all the columns being inherited, but not
> >>> for plain inheritance.
>
> >> Uh, what?
>
> > But maybe for the case under question, that's irrelevant, because
> > we're only interested in access to inherited columns as those are the
> > only ones that can be accessed in queries via parent.
>
> Yeah, that's what I thought.  It seems like it should be possible to base
> all stats access decisions off the table actually named in the query,
> because only columns appearing in that table could be referenced, and only
> that table's permissions actually get checked at runtime.
>
> I guess it's possible that a child table could have, say, an index on
> column X (inherited) and column Y (local) and that some aspect of costing
> might then be interested in the behavior of column Y, even though the
> query could only mention X not Y.  But then we could fall back to the
> existing behavior.

Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
recursively fetch its parent until we reach to the base relation
(which is actually named in the query). And, once we have the base
relation we can check ACL on that and set vardata->acl_ok accordingly.
Additionally, for getting the parent RTI we need to traverse
"root->append_rel_list". Another alternative could be that we can add
parent_rti member in RelOptInfo structure.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  10. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-26T07:42:30Z

    On 2018/10/25 19:54, Dilip Kumar wrote:
> On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Amit Langote <amitlangote09@gmail.com> writes:
>>> But maybe for the case under question, that's irrelevant, because
>>> we're only interested in access to inherited columns as those are the
>>> only ones that can be accessed in queries via parent.
>>
>> Yeah, that's what I thought.  It seems like it should be possible to base
>> all stats access decisions off the table actually named in the query,
>> because only columns appearing in that table could be referenced, and only
>> that table's permissions actually get checked at runtime.
>>
>> I guess it's possible that a child table could have, say, an index on
>> column X (inherited) and column Y (local) and that some aspect of costing
>> might then be interested in the behavior of column Y, even though the
>> query could only mention X not Y.  But then we could fall back to the
>> existing behavior.
> 
> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
> recursively fetch its parent until we reach to the base relation
> (which is actually named in the query). And, once we have the base
> relation we can check ACL on that and set vardata->acl_ok accordingly.
> Additionally, for getting the parent RTI we need to traverse
> "root->append_rel_list". Another alternative could be that we can add
> parent_rti member in RelOptInfo structure.

Adding parent_rti would be a better idea [1].  I think that traversing
append_rel_list every time would be inefficient.

Thanks,
Amit

[1] I've named it inh_root_parent in one of the patches I'm working on
where I needed such a field (https://commitfest.postgresql.org/20/1778/)

  11. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2018-10-27T04:37:00Z

    On Fri, Oct 26, 2018 at 1:12 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2018/10/25 19:54, Dilip Kumar wrote:
> > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Amit Langote <amitlangote09@gmail.com> writes:
> >>> But maybe for the case under question, that's irrelevant, because
> >>> we're only interested in access to inherited columns as those are the
> >>> only ones that can be accessed in queries via parent.
> >>
> >> Yeah, that's what I thought.  It seems like it should be possible to base
> >> all stats access decisions off the table actually named in the query,
> >> because only columns appearing in that table could be referenced, and only
> >> that table's permissions actually get checked at runtime.
> >>
> >> I guess it's possible that a child table could have, say, an index on
> >> column X (inherited) and column Y (local) and that some aspect of costing
> >> might then be interested in the behavior of column Y, even though the
> >> query could only mention X not Y.  But then we could fall back to the
> >> existing behavior.
> >
> > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
> > recursively fetch its parent until we reach to the base relation
> > (which is actually named in the query). And, once we have the base
> > relation we can check ACL on that and set vardata->acl_ok accordingly.
> > Additionally, for getting the parent RTI we need to traverse
> > "root->append_rel_list". Another alternative could be that we can add
> > parent_rti member in RelOptInfo structure.
>
> Adding parent_rti would be a better idea [1].  I think that traversing
> append_rel_list every time would be inefficient.
> [1] I've named it inh_root_parent in one of the patches I'm working on
> where I needed such a field (https://commitfest.postgresql.org/20/1778/)
Ok, Make sense. I have written a patch by adding this variable.
There is still one FIXME in the patch, basically, after getting the
baserel rte I need to convert child varattno to parent varattno
because in case of inheritance that can be different.  Do we already
have any mapping from child attno to parent attno or we have to look
up the cache.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  12. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2018-10-28T11:35:36Z

    On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >
> > On 2018/10/25 19:54, Dilip Kumar wrote:
> > > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> Amit Langote <amitlangote09@gmail.com> writes:
> > >>> But maybe for the case under question, that's irrelevant, because
> > >>> we're only interested in access to inherited columns as those are the
> > >>> only ones that can be accessed in queries via parent.
> > >>
> > >> Yeah, that's what I thought.  It seems like it should be possible to base
> > >> all stats access decisions off the table actually named in the query,
> > >> because only columns appearing in that table could be referenced, and only
> > >> that table's permissions actually get checked at runtime.
> > >>
> > >> I guess it's possible that a child table could have, say, an index on
> > >> column X (inherited) and column Y (local) and that some aspect of costing
> > >> might then be interested in the behavior of column Y, even though the
> > >> query could only mention X not Y.  But then we could fall back to the
> > >> existing behavior.
> > >
> > > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
> > > recursively fetch its parent until we reach to the base relation
> > > (which is actually named in the query). And, once we have the base
> > > relation we can check ACL on that and set vardata->acl_ok accordingly.
> > > Additionally, for getting the parent RTI we need to traverse
> > > "root->append_rel_list". Another alternative could be that we can add
> > > parent_rti member in RelOptInfo structure.
> >
> > Adding parent_rti would be a better idea [1].  I think that traversing
> > append_rel_list every time would be inefficient.
> > [1] I've named it inh_root_parent in one of the patches I'm working on
> > where I needed such a field (https://commitfest.postgresql.org/20/1778/)
> Ok, Make sense. I have written a patch by adding this variable.
> There is still one FIXME in the patch, basically, after getting the
> baserel rte I need to convert child varattno to parent varattno
> because in case of inheritance that can be different.  Do we already
> have any mapping from child attno to parent attno or we have to look
> up the cache.

Attached patch handles this issue.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  13. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-10-29T09:23:00Z

    Thank you for creating the patch.

On 2018/10/28 20:35, Dilip Kumar wrote:
> On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote wrote:
>>> On 2018/10/25 19:54, Dilip Kumar wrote:
>>>> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
>>>> recursively fetch its parent until we reach to the base relation
>>>> (which is actually named in the query). And, once we have the base
>>>> relation we can check ACL on that and set vardata->acl_ok accordingly.
>>>> Additionally, for getting the parent RTI we need to traverse
>>>> "root->append_rel_list". Another alternative could be that we can add
>>>> parent_rti member in RelOptInfo structure.
>>>
>>> Adding parent_rti would be a better idea [1].  I think that traversing
>>> append_rel_list every time would be inefficient.
>>>
>>> [1] I've named it inh_root_parent in one of the patches I'm working on
>>> where I needed such a field (https://commitfest.postgresql.org/20/1778/)
>>>
>> Ok, Make sense. I have written a patch by adding this variable.
>> There is still one FIXME in the patch, basically, after getting the
>> baserel rte I need to convert child varattno to parent varattno
>> because in case of inheritance that can be different.  Do we already
>> have any mapping from child attno to parent attno or we have to look
>> up the cache.

Sorry I forgot to cc you, but I'd posted a patch on the "speeding up
planning with partitions" thread, that's extracted from the bigger patch,
which adds inh_root_parent member to RelOptInfo [1].  Find it attached
with this email.

One of the differences from your patch is that it makes inh_root_parent
work not just for partitioning, but to inheritance in general.  Also, it
codes the changes to build_simple_rel to set inh_root_parent's value a bit
differently than your patch.

> Attached patch handles this issue.

I noticed a typo in your patch:

transalate_varattno -> translate_varattno

+static int
+transalate_varattno(Oid oldrelid, Oid newrelid, int old_attno)
+{
+	Relation	oldrelation = heap_open(oldrelid, NoLock);

It doesn't seem nice that it performs a heap_open on the parent relation.

+	att = TupleDescAttr(old_tupdesc, old_attno - 1);
+	attname = NameStr(att->attname);
+
+	newtup = SearchSysCacheAttName(newrelid, attname);
+	if (!newtup)
+	{
+		heap_close(oldrelation, NoLock);
+		return InvalidAttrNumber;
+	}

and

+	varattno = transalate_varattno(relid, rte->relid, var->varattno);
+	if (AttributeNumberIsValid(varattno))
+		relid = rte->relid;
+	else
+		varattno = var->varattno;

It's not possible for varattno to be invalid here, because the query on
inheritance parent only allows to select parent's columns, so we'd error
out before getting here if a column not present in the parent were selected.


Anyway, why don't we just use the child table's AppendRelInfo to get the
parent's version of varattno instead of creating a new function?  It can
be done as shown in the attached revised version of the portion of the
patch changing selfuncs.c.  Please take a look.

[1]
https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp

  14. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2018-11-01T11:34:14Z

    On Mon, Oct 29, 2018 at 2:53 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> Thank you for creating the patch.
>
> On 2018/10/28 20:35, Dilip Kumar wrote:
> > On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote wrote:
> >>> On 2018/10/25 19:54, Dilip Kumar wrote:
> >>>> Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
> >>>> recursively fetch its parent until we reach to the base relation
> >>>> (which is actually named in the query). And, once we have the base
> >>>> relation we can check ACL on that and set vardata->acl_ok accordingly.
> >>>> Additionally, for getting the parent RTI we need to traverse
> >>>> "root->append_rel_list". Another alternative could be that we can add
> >>>> parent_rti member in RelOptInfo structure.
> >>>
> >>> Adding parent_rti would be a better idea [1].  I think that traversing
> >>> append_rel_list every time would be inefficient.
> >>>
> >>> [1] I've named it inh_root_parent in one of the patches I'm working on
> >>> where I needed such a field (https://commitfest.postgresql.org/20/1778/)
> >>>
> >> Ok, Make sense. I have written a patch by adding this variable.
> >> There is still one FIXME in the patch, basically, after getting the
> >> baserel rte I need to convert child varattno to parent varattno
> >> because in case of inheritance that can be different.  Do we already
> >> have any mapping from child attno to parent attno or we have to look
> >> up the cache.
>
> Sorry I forgot to cc you, but I'd posted a patch on the "speeding up
> planning with partitions" thread, that's extracted from the bigger patch,
> which adds inh_root_parent member to RelOptInfo [1].  Find it attached
> with this email.
>
> One of the differences from your patch is that it makes inh_root_parent
> work not just for partitioning, but to inheritance in general.  Also, it
> codes the changes to build_simple_rel to set inh_root_parent's value a bit
> differently than your patch.
>
> > Attached patch handles this issue.
>
> I noticed a typo in your patch:
>
> transalate_varattno -> translate_varattno
>
> +static int
> +transalate_varattno(Oid oldrelid, Oid newrelid, int old_attno)
> +{
> +       Relation        oldrelation = heap_open(oldrelid, NoLock);
>
> It doesn't seem nice that it performs a heap_open on the parent relation.
>
> +       att = TupleDescAttr(old_tupdesc, old_attno - 1);
> +       attname = NameStr(att->attname);
> +
> +       newtup = SearchSysCacheAttName(newrelid, attname);
> +       if (!newtup)
> +       {
> +               heap_close(oldrelation, NoLock);
> +               return InvalidAttrNumber;
> +       }
>
> and
>
> +       varattno = transalate_varattno(relid, rte->relid, var->varattno);
> +       if (AttributeNumberIsValid(varattno))
> +               relid = rte->relid;
> +       else
> +               varattno = var->varattno;
>
> It's not possible for varattno to be invalid here, because the query on
> inheritance parent only allows to select parent's columns, so we'd error
> out before getting here if a column not present in the parent were selected.

Make sense to me.
>
>
> Anyway, why don't we just use the child table's AppendRelInfo to get the
> parent's version of varattno instead of creating a new function?  It can
> be done as shown in the attached revised version of the portion of the
> patch changing selfuncs.c.  Please take a look.

+1

>
> [1]
> https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp



-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  15. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-11-02T08:04:27Z

    On 2018/11/01 20:34, Dilip Kumar wrote:
> On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote:
>> Anyway, why don't we just use the child table's AppendRelInfo to get the
>> parent's version of varattno instead of creating a new function?  It can
>> be done as shown in the attached revised version of the portion of the
>> patch changing selfuncs.c.  Please take a look.
> 
> +1

Okay, here are two patches:

0001 adds a new RelOptInfo member inh_root_parent that's set for
inheritance child otherrels and contains the RT index of the inheritance
parent table mentioned in the query from which they originated.

0002 is your patch that modifies examine_variable, etc. to use the
permissions granted on parent before reading stats on otherrel inheritance
child tables. I've added your name as the author in the 2nd patch.

Thanks,
Amit

  16. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-07-10T04:14:37Z

    On Fri, Nov 2, 2018 at 1:34 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2018/11/01 20:34, Dilip Kumar wrote:
> > On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote:
> >> Anyway, why don't we just use the child table's AppendRelInfo to get the
> >> parent's version of varattno instead of creating a new function?  It can
> >> be done as shown in the attached revised version of the portion of the
> >> patch changing selfuncs.c.  Please take a look.
> >
> > +1
>
> Okay, here are two patches:
>
> 0001 adds a new RelOptInfo member inh_root_parent that's set for
> inheritance child otherrels and contains the RT index of the inheritance
> parent table mentioned in the query from which they originated.
>
> 0002 is your patch that modifies examine_variable, etc. to use the
> permissions granted on parent before reading stats on otherrel inheritance
> child tables. I've added your name as the author in the 2nd patch.
>

I have looked into the patches and these look fine to me.  I have also
added it to the next commitfest.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  17. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-07-10T04:29:19Z

    On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Fri, Nov 2, 2018 at 1:34 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >
> > On 2018/11/01 20:34, Dilip Kumar wrote:
> > > On Mon, Oct 29, 2018 at 2:53 PM Amit Langote wrote:
> > >> Anyway, why don't we just use the child table's AppendRelInfo to get the
> > >> parent's version of varattno instead of creating a new function?  It can
> > >> be done as shown in the attached revised version of the portion of the
> > >> patch changing selfuncs.c.  Please take a look.
> > >
> > > +1
> >
> > Okay, here are two patches:
> >
> > 0001 adds a new RelOptInfo member inh_root_parent that's set for
> > inheritance child otherrels and contains the RT index of the inheritance
> > parent table mentioned in the query from which they originated.
> >
> > 0002 is your patch that modifies examine_variable, etc. to use the
> > permissions granted on parent before reading stats on otherrel inheritance
> > child tables. I've added your name as the author in the 2nd patch.
> >
>
> I have looked into the patches and these look fine to me.  I have also
> added it to the next commitfest.
>
Hi Amit,

I have reviewed your 0001 patch and I think you have already taken a
look on 0002.  So should I move it to "Ready for Committer" or you
want to review it further?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  18. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-07-10T04:45:29Z

    Hi Dilip,

On Wed, Jul 10, 2019 at 1:29 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > On Fri, Nov 2, 2018 at 1:34 PM Amit Langote wrote:
> > > Okay, here are two patches:
> > >
> > > 0001 adds a new RelOptInfo member inh_root_parent that's set for
> > > inheritance child otherrels and contains the RT index of the inheritance
> > > parent table mentioned in the query from which they originated.
> > >
> > > 0002 is your patch that modifies examine_variable, etc. to use the
> > > permissions granted on parent before reading stats on otherrel inheritance
> > > child tables. I've added your name as the author in the 2nd patch.
> > >
> >
> > I have looked into the patches and these look fine to me.  I have also
> > added it to the next commitfest.
> >
> Hi Amit,
>
> I have reviewed your 0001 patch and I think you have already taken a
> look on 0002.  So should I move it to "Ready for Committer" or you
> want to review it further?

Thanks for checking.  There has been a lot of churn in the inheritance
planning code since my last email on this thread, so I'd like to
reconsider.  I'm busy this week with some things, so I'll try posting
something on next Tuesday.

Thanks,
Amit

  19. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-07-10T05:43:26Z

    On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Hi Dilip,
>
> On Wed, Jul 10, 2019 at 1:29 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > On Wed, Jul 10, 2019 at 9:44 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > On Fri, Nov 2, 2018 at 1:34 PM Amit Langote wrote:
> > > > Okay, here are two patches:
> > > >
> > > > 0001 adds a new RelOptInfo member inh_root_parent that's set for
> > > > inheritance child otherrels and contains the RT index of the inheritance
> > > > parent table mentioned in the query from which they originated.
> > > >
> > > > 0002 is your patch that modifies examine_variable, etc. to use the
> > > > permissions granted on parent before reading stats on otherrel inheritance
> > > > child tables. I've added your name as the author in the 2nd patch.
> > > >
> > >
> > > I have looked into the patches and these look fine to me.  I have also
> > > added it to the next commitfest.
> > >
> > Hi Amit,
> >
> > I have reviewed your 0001 patch and I think you have already taken a
> > look on 0002.  So should I move it to "Ready for Committer" or you
> > want to review it further?
>
> Thanks for checking.  There has been a lot of churn in the inheritance
> planning code since my last email on this thread, so I'd like to
> reconsider.  I'm busy this week with some things, so I'll try posting
> something on next Tuesday.
>
Sounds good.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  20. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-07-17T09:08:48Z

    On Wed, Jul 10, 2019 at 2:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > Thanks for checking.  There has been a lot of churn in the inheritance
> > planning code since my last email on this thread, so I'd like to
> > reconsider.  I'm busy this week with some things, so I'll try posting
> > something on next Tuesday.
> >
> Sounds good.

I looked at this today and concluded that the problem and the patches
discussed here are fairly isolated from inheritance planning changes
committed to PG 12.

I've combined the two patches into one.  I tried to think up test
cases to go with the code changes, but couldn't come up with one.

Thanks,
Amit

  21. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-07-29T09:59:20Z

    On Wed, Jul 17, 2019 at 2:39 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Wed, Jul 10, 2019 at 2:43 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > On Wed, Jul 10, 2019 at 10:15 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > > Thanks for checking.  There has been a lot of churn in the inheritance
> > > planning code since my last email on this thread, so I'd like to
> > > reconsider.  I'm busy this week with some things, so I'll try posting
> > > something on next Tuesday.
> > >
> > Sounds good.
>
> I looked at this today and concluded that the problem and the patches
> discussed here are fairly isolated from inheritance planning changes
> committed to PG 12.
>
> I've combined the two patches into one.
Looks fine to me, moved to ready for committer.

  I tried to think up test
> cases to go with the code changes, but couldn't come up with one.

I am also not sure how to test whether we have access to the
statistics of the table.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  22. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-07-30T07:34:29Z

    On Mon, Jul 29, 2019 at 6:59 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Wed, Jul 17, 2019 at 2:39 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I've combined the two patches into one.
> Looks fine to me, moved to ready for committer.

Thank you Dilip.

Regards,
Amit

  23. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-03T23:53:05Z

    Amit Langote <amitlangote09@gmail.com> writes:
> [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]

I took a quick look through this.  I have some cosmetic thoughts and
also a couple of substantive concerns:

* As a rule, patches that add fields at the end of a struct are wrong.
There is almost always some more-appropriate place to put the field
based on its semantics.  We don't want our struct layouts to be historical
annals; they should reflect a coherent design.  In this case I'd be
inclined to put the new field next to the regular relid field.  It
should have a name that's less completely unrelated to relid, too.

* It might make more sense to define the new field as "top parent's relid,
or self if no parent", rather than "... or zero if no parent".  Then you
don't need if-tests like this:

+                                if (rel->inh_root_parent > 0)
+                                    rte = planner_rt_fetch(rel->inh_root_parent,
+                                                           root);
+                                else
+                                    rte = planner_rt_fetch(rel->relid, root);

In the places where you actually want the other definition, you could
test for inh_root_parent being different from relid.  That's slightly
more complicated than testing for nonzero, but there aren't many such
places so I think getting rid of the other if's is more useful.

* The business about reverse mapping Vars seems quite inefficient, but
what's much worse is that it only accounts for one level of parent.
I'm pretty certain this will give the wrong answer for multiple levels
of partitioning, if the column numbers aren't all the same.

* To fix the above, you probably need to map back one inheritance level
at a time, which suggests that you could just use the AppendRelInfo
parent-rel info and not need any addition to RelOptInfo at all.  This
makes the efficiency issue even worse, though.  I don't immediately have a
great idea about doing better.  Making it faster might require adding more
info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.

* I'd be inclined to use an actual test-and-elog not just an Assert
for the no-mapping-found case.  For one reason, some compilers are
going to complain about a set-but-not-used variable in non-assert
builds.  More importantly, I'm not very convinced that it's impossible
to hit the no-mapping case.  The original proposal was to fall back
to current behavior (test the child-table permissions) if we couldn't
match the var to the top parent, and I think that that is still a
sane proposal.


As for how to test, it doesn't seem like it should be that hard to devise
a situation where you'll get different plan shapes depending on whether
the planner has an accurate or default selectivity estimate.

			regards, tom lane

  24. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-09-05T08:42:17Z

    On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]
>
> I took a quick look through this.  I have some cosmetic thoughts and
> also a couple of substantive concerns:

Thanks a lot for reviewing this.

> * As a rule, patches that add fields at the end of a struct are wrong.
> There is almost always some more-appropriate place to put the field
> based on its semantics.  We don't want our struct layouts to be historical
> annals; they should reflect a coherent design.  In this case I'd be
> inclined to put the new field next to the regular relid field.  It
> should have a name that's less completely unrelated to relid, too.

I've renamed the field to inh_root_relid and placed it next to relid.

> * It might make more sense to define the new field as "top parent's relid,
> or self if no parent", rather than "... or zero if no parent".  Then you
> don't need if-tests like this:
>
> +                                if (rel->inh_root_parent > 0)
> +                                    rte = planner_rt_fetch(rel->inh_root_parent,
> +                                                           root);
> +                                else
> +                                    rte = planner_rt_fetch(rel->relid, root);

Agreed, done this way.

> * The business about reverse mapping Vars seems quite inefficient, but
> what's much worse is that it only accounts for one level of parent.
> I'm pretty certain this will give the wrong answer for multiple levels
> of partitioning, if the column numbers aren't all the same.

Indeed.  We need to be checking the root parent's permissions, not the
immediate parent's which might be a child itself.

> * To fix the above, you probably need to map back one inheritance level
> at a time, which suggests that you could just use the AppendRelInfo
> parent-rel info and not need any addition to RelOptInfo at all.  This
> makes the efficiency issue even worse, though.  I don't immediately have a
> great idea about doing better.  Making it faster might require adding more
> info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.

Hmm, yes.  If AppendRelInfos had contained a reverse translation list
that maps Vars of a given child to the root parent's, this patch would
end up being much simpler and not add too much cost to the selectivity
code.  However building such a map would not be free and the number of
places where it's useful would be significantly smaller where the
existing parent-to-child translation list is used.

Anyway, I've fixed the above-mentioned oversights in the current code for now.

> * I'd be inclined to use an actual test-and-elog not just an Assert
> for the no-mapping-found case.  For one reason, some compilers are
> going to complain about a set-but-not-used variable in non-assert
> builds.  More importantly, I'm not very convinced that it's impossible
> to hit the no-mapping case.  The original proposal was to fall back
> to current behavior (test the child-table permissions) if we couldn't
> match the var to the top parent, and I think that that is still a
> sane proposal.

OK, I've removed the Assert.  For child Vars that can't be translated
to root parent's, permissions are checked with the child relation,
like before.


> As for how to test, it doesn't seem like it should be that hard to devise
> a situation where you'll get different plan shapes depending on whether
> the planner has an accurate or default selectivity estimate.

I managed to find a test case by trial-and-error, but it may be more
convoluted than it has to be.

-- On HEAD
create table permtest_parent (a int, b text, c text) partition by list (a);
create table permtest_child (b text, a int, c text) partition by list (b);
create table permtest_grandchild (c text, b text, a int);
alter table permtest_child attach partition permtest_grandchild for
values in ('a');
alter table permtest_parent attach partition permtest_child for values in (1);
insert into permtest_parent select 1, 'a', i || 'x' || i * 10 from
generate_series(1, 1000) i;
analyze permtest_parent;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
                                   QUERY PLAN
---------------------------------------------------------------------------------
 Nested Loop  (cost=0.00..47.00 rows=1000 width=28)
   Join Filter: (p1.a = p2.a)
   ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50 rows=1 width=14)
         Filter: (c ~~ '4x5%'::text)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
(5 rows)

create role regress_no_child_access;
revoke all on permtest_grandchild from regress_no_child_access;
grant all on permtest_parent to regress_no_child_access;
set session authorization regress_no_child_access;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
                                     QUERY PLAN
------------------------------------------------------------------------------------
 Hash Join  (cost=18.56..93.31 rows=5000 width=28)
   Hash Cond: (p2.a = p1.a)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
   ->  Hash  (cost=18.50..18.50 rows=5 width=14)
         ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50
rows=5 width=14)
               Filter: (c ~~ '4x5%'::text)
(6 rows)

-- Patched:

set session authorization regress_no_child_access;
explain (costs off) select * from permtest_parent p1 inner join
permtest_parent p2 on p1.a = p2.a and p1.c like '4x5%';
                                   QUERY PLAN
---------------------------------------------------------------------------------
 Nested Loop  (cost=0.00..47.00 rows=1000 width=28)
   Join Filter: (p1.a = p2.a)
   ->  Seq Scan on permtest_grandchild p1  (cost=0.00..18.50 rows=1 width=14)
         Filter: (c ~~ '4x5%'::text)
   ->  Seq Scan on permtest_grandchild p2  (cost=0.00..16.00 rows=1000 width=14)
(5 rows)

Updated patch attached.

Thanks,
Amit

  25. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-09-05T09:18:27Z

    On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote:

Thanks for the patch, I was almost about to press the send button with
my patch.  But, this looks similar to my version.
>
> On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Amit Langote <amitlangote09@gmail.com> writes:
> > > [ v2-0001-Use-root-parent-s-permissions-when-read-child-tab.patch ]
> >
> > I took a quick look through this.  I have some cosmetic thoughts and
> > also a couple of substantive concerns:
>
> Thanks a lot for reviewing this.
>
> > * As a rule, patches that add fields at the end of a struct are wrong.
> > There is almost always some more-appropriate place to put the field
> > based on its semantics.  We don't want our struct layouts to be historical
> > annals; they should reflect a coherent design.  In this case I'd be
> > inclined to put the new field next to the regular relid field.  It
> > should have a name that's less completely unrelated to relid, too.
>
> I've renamed the field to inh_root_relid and placed it next to relid.
>
> > * It might make more sense to define the new field as "top parent's relid,
> > or self if no parent", rather than "... or zero if no parent".  Then you
> > don't need if-tests like this:
> >
> > +                                if (rel->inh_root_parent > 0)
> > +                                    rte = planner_rt_fetch(rel->inh_root_parent,
> > +                                                           root);
> > +                                else
> > +                                    rte = planner_rt_fetch(rel->relid, root);
>
> Agreed, done this way.
>
> > * The business about reverse mapping Vars seems quite inefficient, but
> > what's much worse is that it only accounts for one level of parent.
> > I'm pretty certain this will give the wrong answer for multiple levels
> > of partitioning, if the column numbers aren't all the same.
>
> Indeed.  We need to be checking the root parent's permissions, not the
> immediate parent's which might be a child itself.
>
> > * To fix the above, you probably need to map back one inheritance level
> > at a time, which suggests that you could just use the AppendRelInfo
> > parent-rel info and not need any addition to RelOptInfo at all.  This
> > makes the efficiency issue even worse, though.  I don't immediately have a
> > great idea about doing better.  Making it faster might require adding more
> > info to AppendRelInfos, and I'm not quite sure if it's worth the tradeoff.
>
> Hmm, yes.  If AppendRelInfos had contained a reverse translation list
> that maps Vars of a given child to the root parent's, this patch would
> end up being much simpler and not add too much cost to the selectivity
> code.  However building such a map would not be free and the number of
> places where it's useful would be significantly smaller where the
> existing parent-to-child translation list is used.
>
> Anyway, I've fixed the above-mentioned oversights in the current code for now.
>
> > * I'd be inclined to use an actual test-and-elog not just an Assert
> > for the no-mapping-found case.  For one reason, some compilers are
> > going to complain about a set-but-not-used variable in non-assert
> > builds.  More importantly, I'm not very convinced that it's impossible
> > to hit the no-mapping case.  The original proposal was to fall back
> > to current behavior (test the child-table permissions) if we couldn't
> > match the var to the top parent, and I think that that is still a
> > sane proposal.
>
> OK, I've removed the Assert.  For child Vars that can't be translated
> to root parent's, permissions are checked with the child relation,
> like before.

Instead of falling back to the child, isn't it make more sense to
check the permissions on the parent upto which we could translate (it
may not be the root parent)?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  26. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-09-05T09:33:22Z

    Hi Dilip,

Thanks for checking.

On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Thanks for the patch, I was almost about to press the send button with
> my patch.  But, this looks similar to my version.

Good to hear that.

> > On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > * I'd be inclined to use an actual test-and-elog not just an Assert
> > > for the no-mapping-found case.  For one reason, some compilers are
> > > going to complain about a set-but-not-used variable in non-assert
> > > builds.  More importantly, I'm not very convinced that it's impossible
> > > to hit the no-mapping case.  The original proposal was to fall back
> > > to current behavior (test the child-table permissions) if we couldn't
> > > match the var to the top parent, and I think that that is still a
> > > sane proposal.
> >
> > OK, I've removed the Assert.  For child Vars that can't be translated
> > to root parent's, permissions are checked with the child relation,
> > like before.
>
> Instead of falling back to the child, isn't it make more sense to
> check the permissions on the parent upto which we could translate (it
> may not be the root parent)?

Hmm, in that case, the parent up to which we might be able to
translate would still be a child and might have different permissions
than the table mentioned in the query (what's being called "root" in
this context).  Would it be worth further complicating this code if
that's the case?

Thanks,
Amit

  27. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-09-05T09:33:36Z

    On Thu, Sep 5, 2019 at 2:48 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Thu, Sep 5, 2019 at 2:12 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> Thanks for the patch, I was almost about to press the send button with
> my patch.  But, this looks similar to my version.
> >
> > On Wed, Sep 4, 2019 at 8:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

>
> Instead of falling back to the child, isn't it make more sense to
> check the permissions on the parent upto which we could translate (it
> may not be the root parent)?
>

  /*
+ * For inheritance child relations, we also need to remember
+ * the root parent.
+ */
+ if (parent->rtekind == RTE_RELATION)
+ rel->inh_root_relid = parent->inh_root_relid > 0 ?
+ parent->inh_root_relid :
+ parent->relid;
+ else
+ /* Child relation of flattened UNION ALL subquery. */
+ rel->inh_root_relid = relid;

With the current changes, parent->inh_root_relid will always be > 0 so
(parent->inh_root_relid > 0) condition doesn't make sence. Right?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  28. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-09-05T09:56:29Z

    On Thu, Sep 5, 2019 at 6:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>   /*
> + * For inheritance child relations, we also need to remember
> + * the root parent.
> + */
> + if (parent->rtekind == RTE_RELATION)
> + rel->inh_root_relid = parent->inh_root_relid > 0 ?
> + parent->inh_root_relid :
> + parent->relid;
> + else
> + /* Child relation of flattened UNION ALL subquery. */
> + rel->inh_root_relid = relid;
>
> With the current changes, parent->inh_root_relid will always be > 0 so
> (parent->inh_root_relid > 0) condition doesn't make sence. Right?

Oops, you're right.  It should be:

if (parent->rtekind == RTE_RELATION)
    rel->inh_root_relid = parent->inh_root_relid;
else
    rel->inh_root_relid = relid;

Updated patch attached.

Thanks,
Amit

  29. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Dilip Kumar <dilipbalaut@gmail.com> — 2019-09-05T10:17:36Z

    On Thu, Sep 5, 2019 at 3:26 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Thu, Sep 5, 2019 at 6:33 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >   /*
> > + * For inheritance child relations, we also need to remember
> > + * the root parent.
> > + */
> > + if (parent->rtekind == RTE_RELATION)
> > + rel->inh_root_relid = parent->inh_root_relid > 0 ?
> > + parent->inh_root_relid :
> > + parent->relid;
> > + else
> > + /* Child relation of flattened UNION ALL subquery. */
> > + rel->inh_root_relid = relid;
> >
> > With the current changes, parent->inh_root_relid will always be > 0 so
> > (parent->inh_root_relid > 0) condition doesn't make sence. Right?
>
> Oops, you're right.  It should be:
>
> if (parent->rtekind == RTE_RELATION)
>     rel->inh_root_relid = parent->inh_root_relid;
> else
>     rel->inh_root_relid = relid;
>
Right!

> Updated patch attached.
>

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

  30. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-09-05T15:53:31Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>> Instead of falling back to the child, isn't it make more sense to
>> check the permissions on the parent upto which we could translate (it
>> may not be the root parent)?

> Hmm, in that case, the parent up to which we might be able to
> translate would still be a child and might have different permissions
> than the table mentioned in the query (what's being called "root" in
> this context).  Would it be worth further complicating this code if
> that's the case?

I think that checking intermediate levels would be an actively bad idea
--- it would make the behavior too confusing.  We should preferably check
the table actually named in the query, or if we can't then check the
table we are using the stats of; nothing else.

Another idea that we should consider, though, is to allow the access if
*either* of those two tables allows it.  The main reason that that's
attractive is that it's certain not to break any case that works today.
But also, it would mean that in many practical cases we'd not have to
try to map Vars back up to the original parent, thus avoiding the
performance penalty.  (That is, check the target table as we do now,
and only if we find it lacks permissions do we start mapping back.)

			regards, tom lane

  31. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-09-06T05:59:25Z

    On Fri, Sep 6, 2019 at 12:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Sep 5, 2019 at 6:18 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >> Instead of falling back to the child, isn't it make more sense to
> >> check the permissions on the parent upto which we could translate (it
> >> may not be the root parent)?
>
> > Hmm, in that case, the parent up to which we might be able to
> > translate would still be a child and might have different permissions
> > than the table mentioned in the query (what's being called "root" in
> > this context).  Would it be worth further complicating this code if
> > that's the case?
>
> I think that checking intermediate levels would be an actively bad idea
> --- it would make the behavior too confusing.  We should preferably check
> the table actually named in the query, or if we can't then check the
> table we are using the stats of; nothing else.

Agreed.

> Another idea that we should consider, though, is to allow the access if
> *either* of those two tables allows it.  The main reason that that's
> attractive is that it's certain not to break any case that works today.
> But also, it would mean that in many practical cases we'd not have to
> try to map Vars back up to the original parent, thus avoiding the
> performance penalty.  (That is, check the target table as we do now,
> and only if we find it lacks permissions do we start mapping back.)

Ah, that sounds like a good idea.

Patch updated that way.

Thanks,
Amit

  32. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-09-25T20:15:50Z

    Travis complains that this patch adds a new compile warning.  Please
fix.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  33. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-09-26T01:18:29Z

    On Thu, Sep 26, 2019 at 5:15 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Travis complains that this patch adds a new compile warning.  Please
> fix.

Thanks, updated patch attached.

Regards,
Amit

  34. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-20T21:34:40Z

    Amit Langote <amitlangote09@gmail.com> writes:
> [ v6-0001-Use-root-parent-s-permissions-when-reading-child-.patch ]

I started to review this, and discovered that the new regression test
passes just fine without applying any of the rest of the patch.
Usually we try to design regression test additions so that they
demonstrate that the new code does something different, so this seems
a bit odd.  Can't we set up the test to fail with unpatched code?
Also, the test case contains no expression index, so I can't see how
it'd provide any code coverage for the code added in examine_variable.

The comment for inh_root_relid seems rather inadequate, since it
fails to mention the special case for UNION ALL subqueries.
But do we even need that special case?  It looks to me like the
walk-up-to-parent code is defending against such cases by checking
relkind, so maybe we don't need to throw away info for UNION ALL.
In general, if we're going to add inh_root_relid, I'd like its
definition to be as simple and consistent as possible, because
I'm sure there will be other uses for it.  If it could be something
like "baserel that this otherrel is a child of", full stop,
I think that'd be good.

I don't especially like the logic in examine_simple_variable,
because it walks back up the AppendRelInfo chain but then proceeds
to use
	rte = planner_rt_fetch(rel->inh_root_relid, root);
without any sort of cross-check that it's stopped at that relation
and not some other one.  It'd be better to keep track of the top
parent_relid while walking up, and use that.  Or else make the
loop stop condition be reaching the matching relid.

			regards, tom lane

  35. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-11-21T09:42:10Z

    Thanks for the review.

On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > [ v6-0001-Use-root-parent-s-permissions-when-reading-child-.patch ]
>
> I started to review this, and discovered that the new regression test
> passes just fine without applying any of the rest of the patch.
> Usually we try to design regression test additions so that they
> demonstrate that the new code does something different, so this seems
> a bit odd.  Can't we set up the test to fail with unpatched code?

Hmm, the test case *used to* fail without the code fix back in
September.  That is no longer the case, in short because text_ge() got
marked leakproof since then.

Anyway, I have modified the test case such that it now fails without
the code fix, but I don't have enough faith that it's robust enough.
:(

> Also, the test case contains no expression index, so I can't see how
> it'd provide any code coverage for the code added in examine_variable.

Added a test case involving an expression index, which helped spot a
problem with the code added in examine_variable, which fixed too.

> The comment for inh_root_relid seems rather inadequate, since it
> fails to mention the special case for UNION ALL subqueries.
> But do we even need that special case?  It looks to me like the
> walk-up-to-parent code is defending against such cases by checking
> relkind, so maybe we don't need to throw away info for UNION ALL.
> In general, if we're going to add inh_root_relid, I'd like its
> definition to be as simple and consistent as possible, because
> I'm sure there will be other uses for it.  If it could be something
> like "baserel that this otherrel is a child of", full stop,
> I think that'd be good.

If inh_root_relid meant that, it would no longer be useful to
examine_variable.  In examine_variable, we need to map a child table's
relid to the relid of its root parent table.  If the root parent
itself is under a UNION ALL subquery parent, then inh_root_relid of
all relations in that ancestry chain would point to the UNION ALL
subquery parent, which is not what examine_variable would want to use,
because it's really looking for the root "table".

In examine_simple_variable however, we need to not just map the child
relid to root table relid, but also convert the Var, so we have to
traverse the ancestry chain via AppendRelInfos.  So, we don't really
need inh_root_relid there, because we can calculate that as we're
traversing the AppendRelInfo chain.

I have expanded the comment for inh_root_relid a bit.

> I don't especially like the logic in examine_simple_variable,
> because it walks back up the AppendRelInfo chain but then proceeds
> to use
>         rte = planner_rt_fetch(rel->inh_root_relid, root);
> without any sort of cross-check that it's stopped at that relation
> and not some other one.  It'd be better to keep track of the top
> parent_relid while walking up, and use that.  Or else make the
> loop stop condition be reaching the matching relid.

I've added an Assert to cross-check that the AppendRelInfo traversal
loop stops once it has computed a Var matching inh_root_relid.

Attached updated patch.

Thanks,
Amit

  36. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-11-26T18:25:18Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The comment for inh_root_relid seems rather inadequate, since it
>> fails to mention the special case for UNION ALL subqueries.
>> But do we even need that special case?  It looks to me like the
>> walk-up-to-parent code is defending against such cases by checking
>> relkind, so maybe we don't need to throw away info for UNION ALL.
>> In general, if we're going to add inh_root_relid, I'd like its
>> definition to be as simple and consistent as possible, because
>> I'm sure there will be other uses for it.  If it could be something
>> like "baserel that this otherrel is a child of", full stop,
>> I think that'd be good.

> If inh_root_relid meant that, it would no longer be useful to
> examine_variable.  In examine_variable, we need to map a child table's
> relid to the relid of its root parent table.  If the root parent
> itself is under a UNION ALL subquery parent, then inh_root_relid of
> all relations in that ancestry chain would point to the UNION ALL
> subquery parent, which is not what examine_variable would want to use,
> because it's really looking for the root "table".

Hm, I see.  Still, the definition seems quite ad-hoc and of uncertain
usefulness to any other use-case.  Given that checking permissions for
access to an expression index's stats is a pretty uncommon thing to
be doing, I don't really want to let it drive the definition of a
new RelOptInfo field.

The other reason that I'm on the warpath against this field is that
it makes the patch un-back-patchable, and I'd like to be able to fix
this problem in the back branches.

Given the existence of the append_rel_array array, it's not really
difficult or expensive to use that to chain up to the root parent,
as in the attached simplified patch.  We could only use this back
to v11 where append_rel_array was added, but that's still a lot
better than no back-patched fix at all.

I've not studied the test case too closely yet, other than to verify
that it does fail without the code fix :-).  Other than that, though,
I think this patch is committable for v11 through HEAD.

			regards, tom lane

  37. Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

    Amit Langote <amitlangote09@gmail.com> — 2019-11-27T02:03:34Z

    On Wed, Nov 27, 2019 at 3:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > If inh_root_relid meant that, it would no longer be useful to
> > examine_variable.  In examine_variable, we need to map a child table's
> > relid to the relid of its root parent table.  If the root parent
> > itself is under a UNION ALL subquery parent, then inh_root_relid of
> > all relations in that ancestry chain would point to the UNION ALL
> > subquery parent, which is not what examine_variable would want to use,
> > because it's really looking for the root "table".
>
> Hm, I see.  Still, the definition seems quite ad-hoc and of uncertain
> usefulness to any other use-case.  Given that checking permissions for
> access to an expression index's stats is a pretty uncommon thing to
> be doing, I don't really want to let it drive the definition of a
> new RelOptInfo field.
>
> The other reason that I'm on the warpath against this field is that
> it makes the patch un-back-patchable, and I'd like to be able to fix
> this problem in the back branches.

Both arguments make sense.

> Given the existence of the append_rel_array array, it's not really
> difficult or expensive to use that to chain up to the root parent,
> as in the attached simplified patch.  We could only use this back
> to v11 where append_rel_array was added, but that's still a lot
> better than no back-patched fix at all.

I agree.

> I've not studied the test case too closely yet, other than to verify
> that it does fail without the code fix :-).  Other than that, though,
> I think this patch is committable for v11 through HEAD.

Thanks for committing.

Regards,
Amit