Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof? - Mailing list pgsql-hackers
From | Amit Langote |
---|---|
Subject | Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof? |
Date | |
Msg-id | CA+HiwqH9j0N3kKXA6mHu7ZQUr5DhD76pHW8g=warj+8KDc00Rw@mail.gmail.com Whole thread Raw |
In response to | Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof? (Tom Lane <tgl@sss.pgh.pa.us>) |
Responses |
Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?
|
List | pgsql-hackers |
Thanks for the review. On Thu, Nov 21, 2019 at 6:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > 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? Hmm, the test case *used to* fail without the code fix back in September. That is no longer the case, in short because text_ge() got marked leakproof since then. Anyway, I have modified the test case such that it now fails without the code fix, but I don't have enough faith that it's robust enough. :( > 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. Added a test case involving an expression index, which helped spot a problem with the code added in examine_variable, which fixed too. > 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". In examine_simple_variable however, we need to not just map the child relid to root table relid, but also convert the Var, so we have to traverse the ancestry chain via AppendRelInfos. So, we don't really need inh_root_relid there, because we can calculate that as we're traversing the AppendRelInfo chain. I have expanded the comment for inh_root_relid a bit. > 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. I've added an Assert to cross-check that the AppendRelInfo traversal loop stops once it has computed a Var matching inh_root_relid. Attached updated patch. Thanks, Amit
Attachment
pgsql-hackers by date: