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

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(at)gmail(dot)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