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

Thanks for the review.

On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Langote <amitlangote09(at)gmail(dot)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