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

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