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

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