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

On Mon, Oct 29, 2018 at 2:53 PM Amit Langote
<Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> 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.

Make sense to me.
>
>
> 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

>
> [1]
> https://www.postgresql.org/message-id/f06a398a-40a9-efb4-fab9-784400fecf13%40lab.ntt.co.jp

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com