Re: [HACKERS] Error check always bypassed in tablefunc.c
On Fri, Jan 30, 2015 at 10:32 AM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
On Sat, Jan 17, 2015 at 11:16 PM, Michael Paquier
michael.paqu...@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
From 40ef6b9fa378af588712c0fc98d4e44047f9756c Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Mon, 26 Jan 2015 15:17:23 +0900
Subject: [PATCH] Fix crashes and tuple compatibility checks in connectby()
Coverity has pointed out that compatConnectbyTupleDescs has been dead code
since commit 08c33c4 of 2003. However, after further analysis, it happens
that it is even possible to crash the server as values passed by connectby
are used as-is by the process generating SELECT queries, with for example
values like that '1; --' or that 'column::text, NULL FROM foo; --'. It is
even possible to bypass compatConnectbyTupleDescs by enforcing the types
of the attributes returned as with NULL values as key and parent key
processing did not have checks to see if a value could be NULL. It is already
specified in the documentation that any kind of values can be used by callers
of connectby(), so this commit tightens check around tuple compatibility to
ensure that no callers can crash the server by either enforcing NULL values,
in which case value used is really NULL or when having a query returning only
one column, in which case process simply errors out because a parent key cannot
be defined.
---
contrib/tablefunc/expected/tablefunc.out | 22 +++
contrib/tablefunc/sql/tablefunc.sql | 16 +
contrib/tablefunc/tablefunc.c| 101 +++
3 files changed, 101 insertions(+), 38 deletions(-)
diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out
index 0437ecf..7bf7d6d 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -376,6 +376,28 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') A
11 | 10 | 4 | 2~5~9~10~11
(8 rows)
+-- should fail as first two columns must have the same type
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
+ERROR: invalid return type
+DETAIL: First two columns must be the same type.
+-- should fail as key field datatype should match return datatype
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid json, parent_keyid json, level int, branch text);
+ERROR: invalid return type
+DETAIL: Failed to cast integer to json
+-- tests for values using custom queries
+-- query with one column - failed
+SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: invalid return type
+DETAIL: Return tuple needs at least two columns.
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, 1::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
+-- query with two columns first value as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', '1::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
+-- query with two columns, both values as NULL - infinite recursion
+SELECT * FROM connectby('connectby_int', 'NULL::text, NULL::text; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
+ERROR: infinite recursion detected
-- test for falsely detected recursion
DROP TABLE connectby_int;
CREATE TABLE connectby_int(keyid int, parent_keyid int);
diff --git a/contrib/tablefunc/sql/tablefunc.sql b/contrib/tablefunc/sql/tablefunc.sql
index bf874f2..835eb28 100644
--- a/contrib/tablefunc/sql/tablefunc.sql
+++ b/contrib/tablefunc/sql/tablefunc.sql
@@ -179,6 +179,22 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') A
-- infinite recursion failure avoided by depth limit
SELECT * FROM
Re: [HACKERS] Error check always bypassed in tablefunc.c
On Tue, Jan 20, 2015 at 4:05 PM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway m...@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
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 3388fab..e7c3674 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -54,7 +54,6 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
bool randomAccess);
static void validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial);
static bool compatCrosstabTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
-static bool compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
static void get_normal_pair(float8 *x1, float8 *x2);
static Tuplestorestate *connectby(char *relname,
char *key_fld,
@@ -68,21 +67,6 @@ static Tuplestorestate *connectby(char *relname,
MemoryContext per_query_ctx,
bool randomAccess,
AttInMetadata *attinmeta);
-static Tuplestorestate *build_tuplestore_recursively(char *key_fld,
- char *parent_key_fld,
- char *relname,
- char *orderby_fld,
- char *branch_delim,
- char *start_with,
- char *branch,
- int level,
- int *serial,
- int max_depth,
- bool show_branch,
- bool show_serial,
- MemoryContext per_query_ctx,
- AttInMetadata *attinmeta,
- Tuplestorestate *tupstore);
typedef struct
{
@@ -1161,14 +1145,16 @@ connectby(char *relname,
Tuplestorestate *tupstore = NULL;
int ret;
MemoryContext oldcontext;
-
- int serial = 1;
+ StringInfoData inner_sql, outer_sql, orderby_sql;
+ char **values;
+ int num_cols;
/* Connect to SPI manager */
if ((ret = SPI_connect()) 0)
/* internal error */
elog(ERROR, connectby: SPI_connect returned %d, ret);
+
/* switch to longer term context to create the tuple store */
oldcontext = MemoryContextSwitchTo(per_query_ctx);
@@ -1177,244 +1163,137 @@ connectby(char *relname,
MemoryContextSwitchTo(oldcontext);
- /* now go get the whole tree */
- tupstore = build_tuplestore_recursively(key_fld,
- parent_key_fld,
- relname,
- orderby_fld,
- branch_delim,
- start_with,
- start_with, /* current_branch */
- 0, /* initial level is 0 */
- serial, /* initial serial is 1 */
- max_depth,
- show_branch,
- show_serial,
- per_query_ctx,
- attinmeta,
- tupstore);
-
- SPI_finish();
-
- return tupstore;
-}
-
-static Tuplestorestate *
-build_tuplestore_recursively(char *key_fld,
- char *parent_key_fld,
- char *relname,
- char *orderby_fld,
- char *branch_delim,
- char *start_with,
- char *branch,
- int level,
- int *serial,
- int max_depth,
- bool show_branch,
- bool show_serial,
- MemoryContext per_query_ctx,
- AttInMetadata *attinmeta,
- Tuplestorestate *tupstore)
-{
- TupleDesc tupdesc = attinmeta-tupdesc;
- int ret;
- int proc;
- int serial_column;
- StringInfoData sql;
- char **values;
- char *current_key;
- char *current_key_parent;
- char current_level[INT32_STRLEN];
- char serial_str[INT32_STRLEN];
- char *current_branch;
- HeapTuple tuple;
-
- if (max_depth 0 level max_depth)
- return tupstore;
-
- initStringInfo(sql);
-
- /* Build initial sql statement */
- if (!show_serial)
- {
-
Re: [HACKERS] Error check always bypassed in tablefunc.c
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway m...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier
michael.paqu...@gmail.com wrote:
On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway m...@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 | 0
row3 | row1 | 0
row4 | row2 | 1
row5 | row2 | 0
row6 | row4 | 0
row7 | row3 | 0
row8 | row6 | 0
row9 | row5 | 0
row1 | 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 | row1
row2 | row1 | 1 | row1~row2
row4 | row2 | 2 | row1~row2~row4
row6 | row4 | 3 | row1~row2~row4~row6
row5 | row2 | 2 | row1~row2~row5
row9 | row5 | 3 | row1~row2~row5~row9
row3 | row1 | 1 | row1~row3
row7 | 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.keyid AS 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_list
FROM connectby_tree ORDER BY ct_full_list;
keyid | parent_keyid | level |ct_full_list
---+--+---+-
row1 | null | 0 | row1
row2 | row1 | 1 | row1~row2
row4 | row2 | 2 | row1~row2~row4
row6 | row4 | 3 | row1~row2~row4~row6
row5 | row2 | 2 | row1~row2~row5
row9 | row5 | 3 | row1~row2~row5~row9
row3 | row1 | 1 | row1~row3
row7 | 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
-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- -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
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
From 5d63ecc1642818f4de006faf0c39c23124d1c0e5 Mon Sep 17 00:00:00 2001
From: Michael Paquier mich...@otacoo.com
Date: Sat, 17 Jan 2015 22:52:16 +0900
Subject: [PATCH] Fix compatibility checks of connectby in tablefunc
Coverity has pointed out a block of dead code that was used to check the
compatibility of the input and return types of connectby. The check done
simply compared the input and output OIDs, returning an error if the type
OIDs do not match, however it seems more solid to check for a cast between
those two types, to avoid at the same time errors related to data input.
---
contrib/tablefunc/expected/tablefunc.out | 8
contrib/tablefunc/sql/tablefunc.sql | 6 +++
contrib/tablefunc/tablefunc.c| 64 ++--
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out
index 0437ecf..75eff3e 100644
--- a/contrib/tablefunc/expected/tablefunc.out
+++ b/contrib/tablefunc/expected/tablefunc.out
@@ -376,6 +376,14 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') A
11 | 10 | 4 | 2~5~9~10~11
(8 rows)
+-- should fail as first two columns must have the same type
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
+ERROR: invalid return type
+DETAIL: First two columns must be the same type.
+-- should fail as key field datatype should match return datatype
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid json, parent_keyid json, level int, branch text);
+ERROR: invalid return type
+DETAIL: Failed to cast integer to json
-- test for falsely detected recursion
DROP TABLE connectby_int;
CREATE TABLE connectby_int(keyid int, parent_keyid int);
diff --git a/contrib/tablefunc/sql/tablefunc.sql b/contrib/tablefunc/sql/tablefunc.sql
index bf874f2..aff0699 100644
--- a/contrib/tablefunc/sql/tablefunc.sql
+++ b/contrib/tablefunc/sql/tablefunc.sql
@@ -179,6 +179,12 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') A
-- infinite recursion failure avoided by depth limit
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int, level int, branch text);
+-- should fail as first two columns must have the same type
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
+
+-- should fail as key field datatype should match return datatype
+SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid json, parent_keyid json, level int, branch text);
+
-- test for falsely detected recursion
DROP TABLE connectby_int;
CREATE TABLE connectby_int(keyid int, parent_keyid int);
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 3388fab..3677d96 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -40,7 +40,10 @@
#include funcapi.h
#include lib/stringinfo.h
#include miscadmin.h
+#include nodes/makefuncs.h
[HACKERS] 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
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 3388fab..878255e 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -1317,12 +1317,12 @@ build_tuplestore_recursively(char *key_fld,
StringInfoData chk_current_key;
/* First time through, do a little more setup */
- if (level == 0)
+ if (level == 1)
{
/*
* 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
+ * from the query, but only at the first level -- no need to check
+ * more than once
*/
if (!compatConnectbyTupleDescs(tupdesc, spi_tupdesc))
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Error check always bypassed in tablefunc.c
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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
