Skip to content

Commit 66cc746

Browse files
tglsfdctglsfdc
committed
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 08c33c4 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
1 parent 8251acf commit 66cc746

File tree

3 files changed

+116
-83
lines changed

3 files changed

+116
-83
lines changed

‎contrib/tablefunc/expected/tablefunc.out

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,37 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') A
377377
11 | 10 | 4 | 2~5~9~10~11
378378
(8 rows)
379379

380+
-- should fail as first two columns must have the same type
381+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
382+
ERROR: invalid return type
383+
DETAIL: First two columns must be the same type.
384+
-- should fail as key field datatype should match return datatype
385+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid float8, parent_keyid float8, level int, branch text);
386+
ERROR: infinite recursion detected
387+
-- tests for values using custom queries
388+
-- query with one column - failed
389+
SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
390+
ERROR: invalid return type
391+
DETAIL: Query must return at least two columns.
392+
-- query with two columns first value as NULL
393+
SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
394+
keyid | parent_keyid | level
395+
-------+--------------+-------
396+
2 | | 0
397+
| 1 | 1
398+
(2 rows)
399+
400+
-- query with two columns second value as NULL
401+
SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
402+
ERROR: infinite recursion detected
403+
-- query with two columns, both values as NULL
404+
SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
405+
keyid | parent_keyid | level
406+
-------+--------------+-------
407+
2 | | 0
408+
| | 1
409+
(2 rows)
410+
380411
-- test for falsely detected recursion
381412
DROP TABLE connectby_int;
382413
CREATE TABLE connectby_int(keyid int, parent_keyid int);

‎contrib/tablefunc/sql/tablefunc.sql

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,22 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') A
179179
-- infinite recursion failure avoided by depth limit
180180
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int, level int, branch text);
181181

182+
-- should fail as first two columns must have the same type
183+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
184+
185+
-- should fail as key field datatype should match return datatype
186+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid float8, parent_keyid float8, level int, branch text);
187+
188+
-- tests for values using custom queries
189+
-- query with one column - failed
190+
SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
191+
-- query with two columns first value as NULL
192+
SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
193+
-- query with two columns second value as NULL
194+
SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
195+
-- query with two columns, both values as NULL
196+
SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
197+
182198
-- test for falsely detected recursion
183199
DROP TABLE connectby_int;
184200
CREATE TABLE connectby_int(keyid int, parent_keyid int);

‎contrib/tablefunc/tablefunc.c

Lines changed: 69 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
5252
bool randomAccess);
5353
static void validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial);
5454
static bool compatCrosstabTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
55-
static bool compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
55+
static void compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
5656
static void get_normal_pair(float8 *x1, float8 *x2);
5757
static Tuplestorestate *connectby(char *relname,
5858
char *key_fld,
@@ -66,7 +66,7 @@ static Tuplestorestate *connectby(char *relname,
6666
MemoryContext per_query_ctx,
6767
bool randomAccess,
6868
AttInMetadata *attinmeta);
69-
static Tuplestorestate *build_tuplestore_recursively(char *key_fld,
69+
static void build_tuplestore_recursively(char *key_fld,
7070
char *parent_key_fld,
7171
char *relname,
7272
char *orderby_fld,
@@ -1176,28 +1176,28 @@ connectby(char *relname,
11761176
MemoryContextSwitchTo(oldcontext);
11771177

11781178
/* now go get the whole tree */
1179-
tupstore = build_tuplestore_recursively(key_fld,
1180-
parent_key_fld,
1181-
relname,
1182-
orderby_fld,
1183-
branch_delim,
1184-
start_with,
1185-
start_with, /* current_branch */
1186-
0, /* initial level is 0 */
1187-
&serial, /* initial serial is 1 */
1188-
max_depth,
1189-
show_branch,
1190-
show_serial,
1191-
per_query_ctx,
1192-
attinmeta,
1193-
tupstore);
1179+
build_tuplestore_recursively(key_fld,
1180+
parent_key_fld,
1181+
relname,
1182+
orderby_fld,
1183+
branch_delim,
1184+
start_with,
1185+
start_with, /* current_branch */
1186+
0, /* initial level is 0 */
1187+
&serial, /* initial serial is 1 */
1188+
max_depth,
1189+
show_branch,
1190+
show_serial,
1191+
per_query_ctx,
1192+
attinmeta,
1193+
tupstore);
11941194

11951195
SPI_finish();
11961196

11971197
return tupstore;
11981198
}
11991199

1200-
static Tuplestorestate *
1200+
static void
12011201
build_tuplestore_recursively(char *key_fld,
12021202
char *parent_key_fld,
12031203
char *relname,
@@ -1228,7 +1228,7 @@ build_tuplestore_recursively(char *key_fld,
12281228
HeapTuple tuple;
12291229

12301230
if (max_depth > 0 && level > max_depth)
1231-
return tupstore;
1231+
return;
12321232

12331233
initStringInfo(&sql);
12341234

@@ -1314,22 +1314,11 @@ build_tuplestore_recursively(char *key_fld,
13141314
StringInfoData chk_branchstr;
13151315
StringInfoData chk_current_key;
13161316

1317-
/* First time through, do a little more setup */
1318-
if (level == 0)
1319-
{
1320-
/*
1321-
* Check that return tupdesc is compatible with the one we got
1322-
* from the query, but only at level 0 -- no need to check more
1323-
* than once
1324-
*/
1325-
1326-
if (!compatConnectbyTupleDescs(tupdesc, spi_tupdesc))
1327-
ereport(ERROR,
1328-
(errcode(ERRCODE_SYNTAX_ERROR),
1329-
errmsg("invalid return type"),
1330-
errdetail("Return and SQL tuple descriptions are " \
1331-
"incompatible.")));
1332-
}
1317+
/*
1318+
* Check that return tupdesc is compatible with the one we got from
1319+
* the query.
1320+
*/
1321+
compatConnectbyTupleDescs(tupdesc, spi_tupdesc);
13331322

13341323
initStringInfo(&branchstr);
13351324
initStringInfo(&chk_branchstr);
@@ -1344,24 +1333,31 @@ build_tuplestore_recursively(char *key_fld,
13441333
/* get the next sql result tuple */
13451334
spi_tuple = tuptable->vals[i];
13461335

1347-
/* get the current key and parent */
1336+
/* get the current key (might be NULL) */
13481337
current_key = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
1349-
appendStringInfo(&chk_current_key, "%s%s%s", branch_delim, current_key, branch_delim);
1350-
current_key_parent = pstrdup(SPI_getvalue(spi_tuple, spi_tupdesc, 2));
1338+
1339+
/* get the parent key (might be NULL) */
1340+
current_key_parent = SPI_getvalue(spi_tuple, spi_tupdesc, 2);
13511341

13521342
/* get the current level */
13531343
sprintf(current_level, "%d", level);
13541344

13551345
/* check to see if this key is also an ancestor */
1356-
if (strstr(chk_branchstr.data, chk_current_key.data))
1357-
elog(ERROR, "infinite recursion detected");
1346+
if (current_key)
1347+
{
1348+
appendStringInfo(&chk_current_key, "%s%s%s",
1349+
branch_delim, current_key, branch_delim);
1350+
if (strstr(chk_branchstr.data, chk_current_key.data))
1351+
elog(ERROR, "infinite recursion detected");
1352+
}
13581353

13591354
/* OK, extend the branch */
1360-
appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
1355+
if (current_key)
1356+
appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
13611357
current_branch = branchstr.data;
13621358

13631359
/* build a tuple */
1364-
values[0] = pstrdup(current_key);
1360+
values[0] = current_key;
13651361
values[1] = current_key_parent;
13661362
values[2] = current_level;
13671363
if (show_branch)
@@ -1377,30 +1373,31 @@ build_tuplestore_recursively(char *key_fld,
13771373

13781374
tuple = BuildTupleFromCStrings(attinmeta, values);
13791375

1380-
xpfree(current_key);
1381-
xpfree(current_key_parent);
1382-
13831376
/* store the tuple for later use */
13841377
tuplestore_puttuple(tupstore, tuple);
13851378

13861379
heap_freetuple(tuple);
13871380

1388-
/* recurse using current_key_parent as the new start_with */
1389-
tupstore = build_tuplestore_recursively(key_fld,
1390-
parent_key_fld,
1391-
relname,
1392-
orderby_fld,
1393-
branch_delim,
1394-
values[0],
1395-
current_branch,
1396-
level + 1,
1397-
serial,
1398-
max_depth,
1399-
show_branch,
1400-
show_serial,
1401-
per_query_ctx,
1402-
attinmeta,
1403-
tupstore);
1381+
/* recurse using current_key as the new start_with */
1382+
if (current_key)
1383+
build_tuplestore_recursively(key_fld,
1384+
parent_key_fld,
1385+
relname,
1386+
orderby_fld,
1387+
branch_delim,
1388+
current_key,
1389+
current_branch,
1390+
level + 1,
1391+
serial,
1392+
max_depth,
1393+
show_branch,
1394+
show_serial,
1395+
per_query_ctx,
1396+
attinmeta,
1397+
tupstore);
1398+
1399+
xpfree(current_key);
1400+
xpfree(current_key_parent);
14041401

14051402
/* reset branch for next pass */
14061403
resetStringInfo(&branchstr);
@@ -1412,8 +1409,6 @@ build_tuplestore_recursively(char *key_fld,
14121409
xpfree(chk_branchstr.data);
14131410
xpfree(chk_current_key.data);
14141411
}
1415-
1416-
return tupstore;
14171412
}
14181413

14191414
/*
@@ -1486,34 +1481,25 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial
14861481
/*
14871482
* Check if spi sql tupdesc and return tupdesc are compatible
14881483
*/
1489-
static bool
1484+
static void
14901485
compatConnectbyTupleDescs(TupleDesc ret_tupdesc, TupleDesc sql_tupdesc)
14911486
{
1492-
Oid ret_atttypid;
1493-
Oid sql_atttypid;
1494-
1495-
/* check the key_fld types match */
1496-
ret_atttypid = ret_tupdesc->attrs[0]->atttypid;
1497-
sql_atttypid = sql_tupdesc->attrs[0]->atttypid;
1498-
if (ret_atttypid != sql_atttypid)
1487+
/*
1488+
* Result must have at least 2 columns.
1489+
*/
1490+
if (sql_tupdesc->natts < 2)
14991491
ereport(ERROR,
15001492
(errcode(ERRCODE_SYNTAX_ERROR),
15011493
errmsg("invalid return type"),
1502-
errdetail("SQL key field datatype does " \
1503-
"not match return key field datatype.")));
1494+
errdetail("Query must return at least two columns.")));
15041495

1505-
/* check the parent_key_fld types match */
1506-
ret_atttypid = ret_tupdesc->attrs[1]->atttypid;
1507-
sql_atttypid = sql_tupdesc->attrs[1]->atttypid;
1508-
if (ret_atttypid != sql_atttypid)
1509-
ereport(ERROR,
1510-
(errcode(ERRCODE_SYNTAX_ERROR),
1511-
errmsg("invalid return type"),
1512-
errdetail("SQL parent key field datatype does " \
1513-
"not match return parent key field datatype.")));
1496+
/*
1497+
* We have failed to check datatype match since 2003, so we don't do that
1498+
* here. The call will work as long as the datatypes are I/O
1499+
* representation compatible.
1500+
*/
15141501

15151502
/* OK, the two tupdescs are compatible for our purposes */
1516-
return true;
15171503
}
15181504

15191505
/*

0 commit comments

Comments
 (0)