Skip to content

Commit dc9a506

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 059f30c commit dc9a506

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
@@ -383,6 +383,37 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') A
383383
11 | 10 | 4 | 2~5~9~10~11
384384
(8 rows)
385385

386+
-- should fail as first two columns must have the same type
387+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
388+
ERROR: invalid return type
389+
DETAIL: First two columns must be the same type.
390+
-- should fail as key field datatype should match return datatype
391+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid float8, parent_keyid float8, level int, branch text);
392+
ERROR: infinite recursion detected
393+
-- tests for values using custom queries
394+
-- query with one column - failed
395+
SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
396+
ERROR: invalid return type
397+
DETAIL: Query must return at least two columns.
398+
-- query with two columns first value as NULL
399+
SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
400+
keyid | parent_keyid | level
401+
-------+--------------+-------
402+
2 | | 0
403+
| 1 | 1
404+
(2 rows)
405+
406+
-- query with two columns second value as NULL
407+
SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
408+
ERROR: infinite recursion detected
409+
-- query with two columns, both values as NULL
410+
SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
411+
keyid | parent_keyid | level
412+
-------+--------------+-------
413+
2 | | 0
414+
| | 1
415+
(2 rows)
416+
386417
-- test for falsely detected recursion
387418
DROP TABLE connectby_int;
388419
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
@@ -187,6 +187,22 @@ SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') A
187187
-- infinite recursion failure avoided by depth limit
188188
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 4, '~') AS t(keyid int, parent_keyid int, level int, branch text);
189189

190+
-- should fail as first two columns must have the same type
191+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid text, parent_keyid int, level int, branch text);
192+
193+
-- should fail as key field datatype should match return datatype
194+
SELECT * FROM connectby('connectby_int', 'keyid', 'parent_keyid', '2', 0, '~') AS t(keyid float8, parent_keyid float8, level int, branch text);
195+
196+
-- tests for values using custom queries
197+
-- query with one column - failed
198+
SELECT * FROM connectby('connectby_int', '1; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
199+
-- query with two columns first value as NULL
200+
SELECT * FROM connectby('connectby_int', 'NULL::int, 1::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
201+
-- query with two columns second value as NULL
202+
SELECT * FROM connectby('connectby_int', '1::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
203+
-- query with two columns, both values as NULL
204+
SELECT * FROM connectby('connectby_int', 'NULL::int, NULL::int; --', 'parent_keyid', '2', 0) AS t(keyid int, parent_keyid int, level int);
205+
190206
-- test for falsely detected recursion
191207
DROP TABLE connectby_int;
192208
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
@@ -56,7 +56,7 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
5656
bool randomAccess);
5757
static void validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial);
5858
static bool compatCrosstabTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
59-
static bool compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
59+
static void compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
6060
static void get_normal_pair(float8 *x1, float8 *x2);
6161
static Tuplestorestate *connectby(char *relname,
6262
char *key_fld,
@@ -70,7 +70,7 @@ static Tuplestorestate *connectby(char *relname,
7070
MemoryContext per_query_ctx,
7171
bool randomAccess,
7272
AttInMetadata *attinmeta);
73-
static Tuplestorestate *build_tuplestore_recursively(char *key_fld,
73+
static void build_tuplestore_recursively(char *key_fld,
7474
char *parent_key_fld,
7575
char *relname,
7676
char *orderby_fld,
@@ -1181,28 +1181,28 @@ connectby(char *relname,
11811181
MemoryContextSwitchTo(oldcontext);
11821182

11831183
/* now go get the whole tree */
1184-
tupstore = build_tuplestore_recursively(key_fld,
1185-
parent_key_fld,
1186-
relname,
1187-
orderby_fld,
1188-
branch_delim,
1189-
start_with,
1190-
start_with, /* current_branch */
1191-
0, /* initial level is 0 */
1192-
&serial, /* initial serial is 1 */
1193-
max_depth,
1194-
show_branch,
1195-
show_serial,
1196-
per_query_ctx,
1197-
attinmeta,
1198-
tupstore);
1184+
build_tuplestore_recursively(key_fld,
1185+
parent_key_fld,
1186+
relname,
1187+
orderby_fld,
1188+
branch_delim,
1189+
start_with,
1190+
start_with, /* current_branch */
1191+
0, /* initial level is 0 */
1192+
&serial, /* initial serial is 1 */
1193+
max_depth,
1194+
show_branch,
1195+
show_serial,
1196+
per_query_ctx,
1197+
attinmeta,
1198+
tupstore);
11991199

12001200
SPI_finish();
12011201

12021202
return tupstore;
12031203
}
12041204

1205-
static Tuplestorestate *
1205+
static void
12061206
build_tuplestore_recursively(char *key_fld,
12071207
char *parent_key_fld,
12081208
char *relname,
@@ -1233,7 +1233,7 @@ build_tuplestore_recursively(char *key_fld,
12331233
HeapTuple tuple;
12341234

12351235
if (max_depth > 0 && level > max_depth)
1236-
return tupstore;
1236+
return;
12371237

12381238
initStringInfo(&sql);
12391239

@@ -1319,22 +1319,11 @@ build_tuplestore_recursively(char *key_fld,
13191319
StringInfoData chk_branchstr;
13201320
StringInfoData chk_current_key;
13211321

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

13391328
initStringInfo(&branchstr);
13401329
initStringInfo(&chk_branchstr);
@@ -1349,24 +1338,31 @@ build_tuplestore_recursively(char *key_fld,
13491338
/* get the next sql result tuple */
13501339
spi_tuple = tuptable->vals[i];
13511340

1352-
/* get the current key and parent */
1341+
/* get the current key (might be NULL) */
13531342
current_key = SPI_getvalue(spi_tuple, spi_tupdesc, 1);
1354-
appendStringInfo(&chk_current_key, "%s%s%s", branch_delim, current_key, branch_delim);
1355-
current_key_parent = pstrdup(SPI_getvalue(spi_tuple, spi_tupdesc, 2));
1343+
1344+
/* get the parent key (might be NULL) */
1345+
current_key_parent = SPI_getvalue(spi_tuple, spi_tupdesc, 2);
13561346

13571347
/* get the current level */
13581348
sprintf(current_level, "%d", level);
13591349

13601350
/* check to see if this key is also an ancestor */
1361-
if (strstr(chk_branchstr.data, chk_current_key.data))
1362-
elog(ERROR, "infinite recursion detected");
1351+
if (current_key)
1352+
{
1353+
appendStringInfo(&chk_current_key, "%s%s%s",
1354+
branch_delim, current_key, branch_delim);
1355+
if (strstr(chk_branchstr.data, chk_current_key.data))
1356+
elog(ERROR, "infinite recursion detected");
1357+
}
13631358

13641359
/* OK, extend the branch */
1365-
appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
1360+
if (current_key)
1361+
appendStringInfo(&branchstr, "%s%s", branch_delim, current_key);
13661362
current_branch = branchstr.data;
13671363

13681364
/* build a tuple */
1369-
values[0] = pstrdup(current_key);
1365+
values[0] = current_key;
13701366
values[1] = current_key_parent;
13711367
values[2] = current_level;
13721368
if (show_branch)
@@ -1382,30 +1378,31 @@ build_tuplestore_recursively(char *key_fld,
13821378

13831379
tuple = BuildTupleFromCStrings(attinmeta, values);
13841380

1385-
xpfree(current_key);
1386-
xpfree(current_key_parent);
1387-
13881381
/* store the tuple for later use */
13891382
tuplestore_puttuple(tupstore, tuple);
13901383

13911384
heap_freetuple(tuple);
13921385

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

14101407
/* reset branch for next pass */
14111408
resetStringInfo(&branchstr);
@@ -1417,8 +1414,6 @@ build_tuplestore_recursively(char *key_fld,
14171414
xpfree(chk_branchstr.data);
14181415
xpfree(chk_current_key.data);
14191416
}
1420-
1421-
return tupstore;
14221417
}
14231418

14241419
/*
@@ -1491,34 +1486,25 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial
14911486
/*
14921487
* Check if spi sql tupdesc and return tupdesc are compatible
14931488
*/
1494-
static bool
1489+
static void
14951490
compatConnectbyTupleDescs(TupleDesc ret_tupdesc, TupleDesc sql_tupdesc)
14961491
{
1497-
Oid ret_atttypid;
1498-
Oid sql_atttypid;
1499-
1500-
/* check the key_fld types match */
1501-
ret_atttypid = ret_tupdesc->attrs[0]->atttypid;
1502-
sql_atttypid = sql_tupdesc->attrs[0]->atttypid;
1503-
if (ret_atttypid != sql_atttypid)
1492+
/*
1493+
* Result must have at least 2 columns.
1494+
*/
1495+
if (sql_tupdesc->natts < 2)
15041496
ereport(ERROR,
15051497
(errcode(ERRCODE_SYNTAX_ERROR),
15061498
errmsg("invalid return type"),
1507-
errdetail("SQL key field datatype does " \
1508-
"not match return key field datatype.")));
1499+
errdetail("Query must return at least two columns.")));
15091500

1510-
/* check the parent_key_fld types match */
1511-
ret_atttypid = ret_tupdesc->attrs[1]->atttypid;
1512-
sql_atttypid = sql_tupdesc->attrs[1]->atttypid;
1513-
if (ret_atttypid != sql_atttypid)
1514-
ereport(ERROR,
1515-
(errcode(ERRCODE_SYNTAX_ERROR),
1516-
errmsg("invalid return type"),
1517-
errdetail("SQL parent key field datatype does " \
1518-
"not match return parent key field datatype.")));
1501+
/*
1502+
* We have failed to check datatype match since 2003, so we don't do that
1503+
* here. The call will work as long as the datatypes are I/O
1504+
* representation compatible.
1505+
*/
15191506

15201507
/* OK, the two tupdescs are compatible for our purposes */
1521-
return true;
15221508
}
15231509

15241510
/*

0 commit comments

Comments
 (0)