From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
        Dilip Kumar <dilipbalaut@gmail.com>,
        Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>,
        Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: CVE-2017-7484-induced bugs, or,
 btree cmp functions are not leakproof?
In-reply-to: 
 <CA+HiwqHAdRSTmrTKwg7bxxC=XW+ZzUF3+7wMZK284Ajy0GK4Lw@mail.gmail.com>
References: 
 <CA+HiwqGhPRsmza4fY4KVQ3G4kgbJy+G8FDo5OT5DfSNRciAJvg@mail.gmail.com>
 <20190925201550.GA23406@alvherre.pgsql>
 <CA+HiwqHAdRSTmrTKwg7bxxC=XW+ZzUF3+7wMZK284Ajy0GK4Lw@mail.gmail.com>
Comments: In-reply-to Amit Langote <amitlangote09@gmail.com>
	message dated "Thu, 26 Sep 2019 10:18:29 +0900"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <28142.1574285680.1@sss.pgh.pa.us>
Date: Wed, 20 Nov 2019 16:34:40 -0500
Message-ID: <28143.1574285680@sss.pgh.pa.us>
Precedence: bulk

Amit Langote <amitlangote09@gmail.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?
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.

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.

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.

			regards, tom lane