From: Tom Lane <tgl@sss.pgh.pa.us>
To: Amit Langote <amitlangote09@gmail.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?
Date: Wed, 20 Nov 2019 16:34:40 -0500
Message-ID: <28143.1574285680@sss.pgh.pa.us> (raw)
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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, amitlangote09@gmail.com, alvherre@2ndquadrant.com, dilipbalaut@gmail.com, Langote_Amit_f8@lab.ntt.co.jp, peter.eisentraut@2ndquadrant.com, pgsql-hackers@lists.postgresql.org
Subject: Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?
In-Reply-To: <28143.1574285680@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox