Обсуждение: Error check always bypassed in tablefunc.c
Hi all,
As mentioned in $subject, commit 08c33c4 of 2003 has made the
following block of code dead in tablefunc.c:1320 because level is
incremented to at least 1:
/* First time through, do a little more setup */
if (level == 0)
{
/*
* Check that return tupdesc is compatible
with the one we got
* from the query, but only at level 0 -- no
need to check more
* than once
*/
if (!compatConnectbyTupleDescs(tupdesc, spi_tupdesc))
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid return type"),
errdetail("Return and
SQL tuple descriptions are " \
"incompatible.")));
}
A simple fix is simply to change "level == 0" to "level == 1" to check
for this error, like in the patch attached. This issue has been
spotted by Coverity.
Regards,
--
Michael
Вложения
Michael Paquier wrote:
> As mentioned in $subject, commit 08c33c4 of 2003 has made the
> following block of code dead in tablefunc.c:1320 because level is
> incremented to at least 1:
> /* First time through, do a little more setup */
> if (level == 0)
> {
Uh. This means the error case has no test coverage ...
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera wrote:
> Michael Paquier wrote:
>
>> As mentioned in $subject, commit 08c33c4 of 2003 has made the
>> following block of code dead in tablefunc.c:1320 because level is
>> incremented to at least 1:
>> /* First time through, do a little more setup */
>> if (level == 0)
>> {
>
> Uh. This means the error case has no test coverage ...
And looking at that more closely things are a bit more tricky than it
seems. The problem is here related especially to connectby in the
checks done to determine if the input and return key fields are
compatible or not by comparing directly some type OIDs in
compatConnectbyTupleDescs.
Note that if we simply enable this check as-is, connectby would fail
for example with test cases like this one simply because text !=
varchar:
CREATE TABLE connectby_tree(keyid text, parent_keyid text, pos int);
INSERT INTO connectby_tree VALUES('row1',NULL, 0);
INSERT INTO connectby_tree VALUES('row2','row1', 0);
INSERT INTO connectby_tree VALUES('row3','row1', 0);
INSERT INTO connectby_tree VALUES('row4','row2', 1);
INSERT INTO connectby_tree VALUES('row5','row2', 0);
SELECT * FROM connectby('connectby_tree', 'keyid', 'parent_keyid',
'pos', 'row2', 0) AS
t(keyid varchar(256), parent_keyid varchar(256), level int, pos int);
Hence, what I am proposing as a fix is to replace the old check by
something checking if there is a possible cast between the input and
output types, and to error out if types are not compatible. This way,
compatibility is preserved in HEAD and back-branches, and IMO I think
that it would be good to check this compatibility in connectby. New
regression tests are added as well.
Patch is attached. Comments welcome.
--
Michael
Вложения
Haven't looked at this patch, but I wonder if it would be better to replace the innards of connectby with a rewrite of the query to use standard WITH queries. Maybe we can remove a couple hundred lines from tablefunc.c? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 01/19/2015 08:16 AM, Alvaro Herrera wrote: > Haven't looked at this patch, but I wonder if it would be better > to replace the innards of connectby with a rewrite of the query to > use standard WITH queries. Maybe we can remove a couple hundred > lines from tablefunc.c? Seems like a good idea -- connectby is really obsolete for quite a while now other than as an SRF example. I guess we only keep it around for backwards compatibility? Joe - -- Joe Conway credativ LLC: http://www.credativ.us Linux, PostgreSQL, and general Open Source Training, Service, Consulting, & 24x7 Support -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUvQ9RAAoJEDfy90M199hlRBwP/0KvbDh8x7PpRtqpjSpH7riL 5MMF12XBOJ1UaUcEDKnEFiFj/DBQs/CRva+GB1ahwo3dqNmD733+w9RsSpdEHM7e 7s8K9zUTrlQYnqMs2GXgoi/DK7qzBgPTkAF+9gp7WPbjCqs/G9f7wlCyxM+2Sg0+ UUEGvI0rSvPObsyKjHMOQMTaMaOiQWvvcQ1aKiTBNl2lg5vb8yNUBbqq5/tlx2Cd OMlJi+PFRkCo7aKjT6HRojYVJCbK+QzXZp1UvXOAWzt1ecR695XR3HDAP/d8IInc gAZJsZbYMw8VuWQa8W6brZd2c+3sU21sMSV50dgBpO5nGBnqryKlLs9bP91+BNu6 doUB2sVDaimcYoN8pML/4lrxhQrr2tm9SBmRJMAJEhKUHnjPB3AGwwB2InDKdusK voIFKS12yduqAI7ZQ8ZcpmxCoOesV6a1himdIH/WikPan1ITkCD+toGtmniEuUNv QwDFPCueswlRJEBEq3pbh07ZN7FBeNYxZMVIcdDmomj2BffoDDwovK9mqm2SgpJq WNCT8388lak0eNyZkt8O/6n514Ensn6KAWD/FunMQPVBwgFn8J6fDChZ9z6aw85X 9RgIb3OyjK7tICpTZ4GXkY5xUma3I3LMogzsnkqFc3FaVVCVJ9eCKZ8l2TEil2Uz 5X498pFhQWaut8ptlS9c =OQdT -----END PGP SIGNATURE-----
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway <mail@joeconway.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 01/19/2015 08:16 AM, Alvaro Herrera wrote: >> Haven't looked at this patch, but I wonder if it would be better >> to replace the innards of connectby with a rewrite of the query to >> use standard WITH queries. Maybe we can remove a couple hundred >> lines from tablefunc.c? > > Seems like a good idea -- connectby is really obsolete for quite a > while now other than as an SRF example. I guess we only keep it around > for backwards compatibility? For master, yes we could brush up things a bit. Now do we really do the same for back-branches? I would think that the answer there is something close to the patch I sent. -- Michael
On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway <mail@joeconway.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 01/19/2015 08:16 AM, Alvaro Herrera wrote:
>>> Haven't looked at this patch, but I wonder if it would be better
>>> to replace the innards of connectby with a rewrite of the query to
>>> use standard WITH queries. Maybe we can remove a couple hundred
>>> lines from tablefunc.c?
>>
>> Seems like a good idea -- connectby is really obsolete for quite a
>> while now other than as an SRF example. I guess we only keep it around
>> for backwards compatibility?
> For master, yes we could brush up things a bit. Now do we really do
> the same for back-branches? I would think that the answer there is
> something close to the patch I sent.
So, using a WITH RECURSIVE, here is a query equivalent to what connectby does:
=# SELECT * FROM connectby_text;keyid | parent_keyid | pos
-------+--------------+-----row2 | row1 | 0row3 | row1 | 0row4 | row2 | 1row5 | row2
| 0row6 | row4 | 0row7 | row3 | 0row8 | row6 | 0row9 | row5 | 0row1
| null | 0
(9 rows)
=# SELECT * FROM connectby('connectby_text', 'keyid', 'parent_keyid', 'row1', 3, '~') AS t(keyid text, parent_keyid
text,level int, branch text);keyid | parent_keyid | level | branch
-------+--------------+-------+---------------------row1 | null | 0 | row1row2 | row1 | 1 |
row1~row2row4 | row2 | 2 | row1~row2~row4row6 | row4 | 3 | row1~row2~row4~row6row5 | row2
| 2 | row1~row2~row5row9 | row5 | 3 | row1~row2~row5~row9row3 | row1 | 1 |
row1~row3row7 | row3 | 2 | row1~row3~row7
(8 rows)
=# WITH RECURSIVE connectby_tree AS
( SELECT keyid, 0::int AS level, parent_keyid, keyid as
ct_full_list -- root portion FROM connectby_text WHERE keyid = 'row1' -- start point UNION ALL SELECT
ctext.keyid, (ctree.level + 1)::int AS level, ctext.parent_keyid, CAST(ctree.ct_full_list || '~' ||
ctext.keyidAS text) AS ct_full_list FROM connectby_text AS ctext INNER JOIN connectby_tree AS ctree ON
(ctext.parent_keyid= ctree.keyid) -- connect by WHERE ctree.level <= 2 -- limit of level)SELECT keyid, parent_keyid,
level,ct_full_listFROM connectby_tree ORDER BY ct_full_list;keyid | parent_keyid | level | ct_full_list
-------+--------------+-------+---------------------row1 | null | 0 | row1row2 | row1 | 1 |
row1~row2row4 | row2 | 2 | row1~row2~row4row6 | row4 | 3 | row1~row2~row4~row6row5 | row2
| 2 | row1~row2~row5row9 | row5 | 3 | row1~row2~row5~row9row3 | row1 | 1 |
row1~row3row7 | row3 | 2 | row1~row3~row7
(8 rows)
Using that we got a couple of options:
- Parametrize this query in some set of plpgsql functions and dump
tablefunc to 1.1
- Integrate directly this query in the existing C code and use SPI,
without dumping tablefunc.
Thoughts?
--
Michael
On Tue, Jan 20, 2015 at 4:05 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway <mail@joeconway.com> wrote: >>> -----BEGIN PGP SIGNED MESSAGE----- >>> Hash: SHA1 >>> >>> On 01/19/2015 08:16 AM, Alvaro Herrera wrote: >>>> Haven't looked at this patch, but I wonder if it would be better >>>> to replace the innards of connectby with a rewrite of the query to >>>> use standard WITH queries. Maybe we can remove a couple hundred >>>> lines from tablefunc.c? >>> >>> Seems like a good idea -- connectby is really obsolete for quite a >>> while now other than as an SRF example. I guess we only keep it around >>> for backwards compatibility? >> For master, yes we could brush up things a bit. Now do we really do >> the same for back-branches? I would think that the answer there is >> something close to the patch I sent. > > So, using a WITH RECURSIVE, here is a query equivalent to what connectby does: > [...] > Thoughts? Looking at this stuff, actually I do not think that it is possible for now to support orderby_fld at the same level with WITH RECURSIVE in connectby because we need to reorder the items of the base table within the 2nd query of the UNION ALL to give something like that and WITH RECURSIVE does not support ORDER BY (and mutual recursion between WITH items). Another thing to note is that WITH RECURSIVE weakens the infinite recursion detection. I don't think it would be good to weaken that... Attached is a result of this random hacking, resulting in some cleanup btw: 1 file changed, 110 insertions(+), 264 deletions(-) Regards, -- Michael
Вложения
On Sat, Jan 17, 2015 at 11:16 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > Patch is attached. Comments welcome. So, I have been poking at this code a bit more and as the values of the parameters are passed as-is to the SQL queries that connectby generates internally (this is as well mentioned in the documentation here: http://www.postgresql.org/docs/devel/static/tablefunc.html), you can do quite fancy things by passing for example values of the type "foo FROM table; --" or similar. Particularly, by enforcing a query returning only one column, or NULL values I am even able to crash the server. The interesting part is that even if compatConnectbyTupleDescs is enabled for each level, it is still possible to crash the server by passing for example NULL values casted to the same type, like that 'NULL::text, NULL::text; --'. The attached patch fixes all those things, I have also enabled compatConnectbyTupleDescs to run at each level. I'll add it to the next CF as well to not lose track of it. This behavior has been like that forever... -- Michael
Вложения
Michael Paquier <michael.paquier@gmail.com> writes: > So, I have been poking at this code a bit more and as the values of > the parameters are passed as-is to the SQL queries that connectby > generates internally (this is as well mentioned in the documentation > here: http://www.postgresql.org/docs/devel/static/tablefunc.html), you > can do quite fancy things by passing for example values of the type > "foo FROM table; --" or similar. Particularly, by enforcing a query > returning only one column, or NULL values I am even able to crash the > server. The interesting part is that even if compatConnectbyTupleDescs > is enabled for each level, it is still possible to crash the server by > passing for example NULL values casted to the same type, like that > 'NULL::text, NULL::text; --'. > The attached patch fixes all those things, I have also enabled > compatConnectbyTupleDescs to run at each level. I'll add it to the > next CF as well to not lose track of it. This behavior has been like > that forever... Since this is a potential-core-dump fix, I went ahead and dealt with it rather than waiting for the next CF. I made a few adjustments: * I thought that the way you changed the type compatibility tests was overcomplicated and unnecessary. As the code stands, there's no great need to insist on type compatibility at all: if the printed form of the constructed query's results is acceptable to the outer query's types, it'll work, and otherwise will throw a reasonably intelligible error. Now, we might well want to improve this code to skip the conversion to text and back at some point, in which case we would need to insist on a type match. But in neither of those cases does it seem helpful to ask whether there is a SQL type cast, as your patch was doing. The existence of a cast does not imply I/O representation compatibility, so it's not in line with the current behavior, and if we want to skip text conversion we'd prefer it's exactly the same type, which is the check as it originally existed before it accidentally got broken. What I did about this was to leave the behavior alone in back branches, but insist on a type match in HEAD. I think we can reasonably tighten the type requirements in a new major release, but doing it in minor releases is probably a bad idea. * I thought it was odd to throw an error for NULL input, especially an "infinite recursion" error. However, even with your patch the code would've dumped core on a null current_key value (since it would've passed a null start_with down to the next recursion level). What I changed it to was to omit the recursion test (and hence print the row) and then not recurse at a null. This seems consistent and reasonable. * I made a few other minor cleanups as well, in particular getting rid of some unnecessary pstrdup() steps. regards, tom lane
On Fri, Jan 30, 2015 at 10:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > [blah] > What I did about this was to leave the behavior alone in back branches, > but insist on a type match in HEAD. I think we can reasonably tighten > the type requirements in a new major release, but doing it in minor > releases is probably a bad idea. Hm. OK. I am fine with that for the back branches. Thanks for tightening things on master as well. > * I thought it was odd to throw an error for NULL input, especially > an "infinite recursion" error. However, even with your patch the code > would've dumped core on a null current_key value (since it would've > passed a null start_with down to the next recursion level). What I > changed it to was to omit the recursion test (and hence print the row) > and then not recurse at a null. This seems consistent and reasonable. This sounds reasonable to me as well. > * I made a few other minor cleanups as well, in particular getting > rid of some unnecessary pstrdup() steps. Thanks! -- Michael