From 37507962c3d2123b0f21c50d6172fd0b1e059fe7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 29 Jan 2015 20:18:33 -0500 Subject: [PATCH] Handle unexpected query results, especially NULLs, safely in connectby(). connectby() didn't adequately check that the constructed SQL query returns what it's expected to; in fact, since commit 08c33c426bfebb32 it wasn't checking that at all. This could result in a null-pointer-dereference crash if the constructed query returns only one column instead of the expected two. Less excitingly, it could also result in surprising data conversion failures if the constructed query returned values that were not I/O-conversion-compatible with the types specified by the query calling connectby(). In all branches, insist that the query return at least two columns; this seems like a minimal sanity check that can't break any reasonable use-cases. In HEAD, insist that the constructed query return the types specified by the outer query, including checking for typmod incompatibility, which the code never did even before it got broken. This is to hide the fact that the implementation does a conversion to text and back; someday we might want to improve that. In back branches, leave that alone, since adding a type check in a minor release is more likely to break things than make people happy. Type inconsistencies will continue to work so long as the actual type and declared type are I/O representation compatible, and otherwise will fail the same way they used to. Also, in all branches, be on guard for NULL results from the constructed query, which formerly would cause null-pointer dereference crashes. We now print the row with the NULL but don't recurse down from it. In passing, get rid of the rather pointless idea that build_tuplestore_recursively() should return the same tuplestore that's passed to it. Michael Paquier, adjusted somewhat by me --- contrib/tablefunc/expected/tablefunc.out | 32 +++++ contrib/tablefunc/sql/tablefunc.sql | 16 +++ contrib/tablefunc/tablefunc.c | 163 +++++++++++++---------- 3 files changed, 138 insertions(+), 73 deletions(-) diff --git a/contrib/tablefunc/expected/tablefunc.out b/contrib/tablefunc/expected/tablefunc.out index 0437ecf90a..fffadc6e1b 100644 --- a/contrib/tablefunc/expected/tablefunc.out +++ b/contrib/tablefunc/expected/tablefunc.out @@ -376,6 +376,38 @@ 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 float8, parent_keyid float8, level int, branch text); +ERROR: invalid return type +DETAIL: SQL key field type double precision does not match return key field type integer. +-- 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: Query must return at least two columns. +-- query with two columns first value as NULL +SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int); + keyid | parent_keyid | level +-------+--------------+------- + 2 | | 0 + | 1 | 1 +(2 rows) + +-- query with two columns second value as NULL +SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', '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 +SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int); + keyid | parent_keyid | level +-------+--------------+------- + 2 | | 0 + | | 1 +(2 rows) + -- 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 bf874f26ad..ec375b05c6 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 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 float8, parent_keyid float8, level int, branch text); + +-- 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); +-- query with two columns first value as NULL +SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int); +-- query with two columns second value as NULL +SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int); +-- query with two columns, both values as NULL +SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int); + -- 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 3388fab9a4..8a95d4710b 100644 --- a/contrib/tablefunc/tablefunc.c +++ b/contrib/tablefunc/tablefunc.c @@ -54,7 +54,7 @@ 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 compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2); static void get_normal_pair(float8 *x1, float8 *x2); static Tuplestorestate *connectby(char *relname, char *key_fld, @@ -68,7 +68,7 @@ static Tuplestorestate *connectby(char *relname, MemoryContext per_query_ctx, bool randomAccess, AttInMetadata *attinmeta); -static Tuplestorestate *build_tuplestore_recursively(char *key_fld, +static void build_tuplestore_recursively(char *key_fld, char *parent_key_fld, char *relname, char *orderby_fld, @@ -1178,28 +1178,28 @@ 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); + 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 * +static void build_tuplestore_recursively(char *key_fld, char *parent_key_fld, char *relname, @@ -1230,7 +1230,7 @@ build_tuplestore_recursively(char *key_fld, HeapTuple tuple; if (max_depth > 0 && level > max_depth) - return tupstore; + return; initStringInfo(&sql); @@ -1316,22 +1316,11 @@ build_tuplestore_recursively(char *key_fld, StringInfoData chk_branchstr; StringInfoData chk_current_key; - /* 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."))); - } + /* + * Check that return tupdesc is compatible with the one we got from + * the query. + */ + compatConnectbyTupleDescs(tupdesc, spi_tupdesc); initStringInfo(&branchstr); initStringInfo(&chk_branchstr); @@ -1346,24 +1335,31 @@ build_tuplestore_recursively(char *key_fld, /* get the next sql result tuple */ spi_tuple = tuptable->vals[i]; - /* get the current key and parent */ + /* get the current key (might be NULL) */ current_key = SPI_getvalue(spi_tuple, spi_tupdesc, 1); - appendStringInfo(&chk_current_key, "%s%s%s", branch_delim, current_key, branch_delim); - current_key_parent = pstrdup(SPI_getvalue(spi_tuple, spi_tupdesc, 2)); + + /* get the parent key (might be NULL) */ + current_key_parent = SPI_getvalue(spi_tuple, spi_tupdesc, 2); /* get the current level */ sprintf(current_level, "%d", level); /* check to see if this key is also an ancestor */ - if (strstr(chk_branchstr.data, chk_current_key.data)) - elog(ERROR, "infinite recursion detected"); + if (current_key) + { + appendStringInfo(&chk_current_key, "%s%s%s", + branch_delim, current_key, branch_delim); + if (strstr(chk_branchstr.data, chk_current_key.data)) + elog(ERROR, "infinite recursion detected"); + } /* OK, extend the branch */ - appendStringInfo(&branchstr, "%s%s", branch_delim, current_key); + if (current_key) + appendStringInfo(&branchstr, "%s%s", branch_delim, current_key); current_branch = branchstr.data; /* build a tuple */ - values[0] = pstrdup(current_key); + values[0] = current_key; values[1] = current_key_parent; values[2] = current_level; if (show_branch) @@ -1379,30 +1375,31 @@ build_tuplestore_recursively(char *key_fld, tuple = BuildTupleFromCStrings(attinmeta, values); - xpfree(current_key); - xpfree(current_key_parent); - /* store the tuple for later use */ tuplestore_puttuple(tupstore, tuple); heap_freetuple(tuple); - /* recurse using current_key_parent as the new start_with */ - tupstore = build_tuplestore_recursively(key_fld, - parent_key_fld, - relname, - orderby_fld, - branch_delim, - values[0], - current_branch, - level + 1, - serial, - max_depth, - show_branch, - show_serial, - per_query_ctx, - attinmeta, - tupstore); + /* recurse using current_key as the new start_with */ + if (current_key) + build_tuplestore_recursively(key_fld, + parent_key_fld, + relname, + orderby_fld, + branch_delim, + current_key, + current_branch, + level + 1, + serial, + max_depth, + show_branch, + show_serial, + per_query_ctx, + attinmeta, + tupstore); + + xpfree(current_key); + xpfree(current_key_parent); /* reset branch for next pass */ resetStringInfo(&branchstr); @@ -1414,8 +1411,6 @@ build_tuplestore_recursively(char *key_fld, xpfree(chk_branchstr.data); xpfree(chk_current_key.data); } - - return tupstore; } /* @@ -1488,34 +1483,56 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial /* * Check if spi sql tupdesc and return tupdesc are compatible */ -static bool +static void compatConnectbyTupleDescs(TupleDesc ret_tupdesc, TupleDesc sql_tupdesc) { Oid ret_atttypid; Oid sql_atttypid; + int32 ret_atttypmod; + int32 sql_atttypmod; + + /* + * Result must have at least 2 columns. + */ + if (sql_tupdesc->natts < 2) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("invalid return type"), + errdetail("Query must return at least two columns."))); - /* check the key_fld types match */ + /* + * These columns must match the result type indicated by the calling + * query. + */ ret_atttypid = ret_tupdesc->attrs[0]->atttypid; sql_atttypid = sql_tupdesc->attrs[0]->atttypid; - if (ret_atttypid != sql_atttypid) + ret_atttypmod = ret_tupdesc->attrs[0]->atttypmod; + sql_atttypmod = sql_tupdesc->attrs[0]->atttypmod; + if (ret_atttypid != sql_atttypid || + (ret_atttypmod >= 0 && ret_atttypmod != sql_atttypmod)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("invalid return type"), - errdetail("SQL key field datatype does " \ - "not match return key field datatype."))); + errdetail("SQL key field type %s does " \ + "not match return key field type %s.", + format_type_with_typemod(ret_atttypid, ret_atttypmod), + format_type_with_typemod(sql_atttypid, sql_atttypmod)))); - /* check the parent_key_fld types match */ ret_atttypid = ret_tupdesc->attrs[1]->atttypid; sql_atttypid = sql_tupdesc->attrs[1]->atttypid; - if (ret_atttypid != sql_atttypid) + ret_atttypmod = ret_tupdesc->attrs[1]->atttypmod; + sql_atttypmod = sql_tupdesc->attrs[1]->atttypmod; + if (ret_atttypid != sql_atttypid || + (ret_atttypmod >= 0 && ret_atttypmod != sql_atttypmod)) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("invalid return type"), - errdetail("SQL parent key field datatype does " \ - "not match return parent key field datatype."))); + errdetail("SQL parent key field type %s does " \ + "not match return parent key field type %s.", + format_type_with_typemod(ret_atttypid, ret_atttypmod), + format_type_with_typemod(sql_atttypid, sql_atttypmod)))); /* OK, the two tupdescs are compatible for our purposes */ - return true; } /* -- 2.40.0