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

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