Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit53ae246

Browse files
committed
Handle unexpected query results, especially NULLs, safely in connectby().
connectby() didn't adequately check that the constructed SQL query returnswhat it's expected to; in fact, since commit08c33c4 it wasn'tchecking that at all. This could result in a null-pointer-dereferencecrash if the constructed query returns only one column instead of theexpected two. Less excitingly, it could also result in surprising dataconversion failures if the constructed query returned values that werenot I/O-conversion-compatible with the types specified by the querycalling 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 reasonableuse-cases.In HEAD, insist that the constructed query return the types specified bythe outer query, including checking for typmod incompatibility, which thecode never did even before it got broken. This is to hide the fact thatthe implementation does a conversion to text and back; someday we mightwant to improve that.In back branches, leave that alone, since adding a type check in a minorrelease is more likely to break things than make people happy. Typeinconsistencies will continue to work so long as the actual type anddeclared type are I/O representation compatible, and otherwise will failthe same way they used to.Also, in all branches, be on guard for NULL results from the constructedquery, 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 thatbuild_tuplestore_recursively() should return the same tuplestore that'spassed to it.Michael Paquier, adjusted somewhat by me
1 parent9be6556 commit53ae246

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

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

‎contrib/tablefunc/tablefunc.c

Lines changed: 69 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
5454
boolrandomAccess);
5555
staticvoidvalidateConnectbyTupleDesc(TupleDesctupdesc,boolshow_branch,boolshow_serial);
5656
staticboolcompatCrosstabTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
57-
staticboolcompatConnectbyTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
57+
staticvoidcompatConnectbyTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
5858
staticvoidget_normal_pair(float8*x1,float8*x2);
5959
staticTuplestorestate*connectby(char*relname,
6060
char*key_fld,
@@ -68,7 +68,7 @@ static Tuplestorestate *connectby(char *relname,
6868
MemoryContextper_query_ctx,
6969
boolrandomAccess,
7070
AttInMetadata*attinmeta);
71-
staticTuplestorestate*build_tuplestore_recursively(char*key_fld,
71+
staticvoidbuild_tuplestore_recursively(char*key_fld,
7272
char*parent_key_fld,
7373
char*relname,
7474
char*orderby_fld,
@@ -1178,28 +1178,28 @@ connectby(char *relname,
11781178
MemoryContextSwitchTo(oldcontext);
11791179

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

11971197
SPI_finish();
11981198

11991199
returntupstore;
12001200
}
12011201

1202-
staticTuplestorestate*
1202+
staticvoid
12031203
build_tuplestore_recursively(char*key_fld,
12041204
char*parent_key_fld,
12051205
char*relname,
@@ -1230,7 +1230,7 @@ build_tuplestore_recursively(char *key_fld,
12301230
HeapTupletuple;
12311231

12321232
if (max_depth>0&&level>max_depth)
1233-
returntupstore;
1233+
return;
12341234

12351235
initStringInfo(&sql);
12361236

@@ -1316,22 +1316,11 @@ build_tuplestore_recursively(char *key_fld,
13161316
StringInfoDatachk_branchstr;
13171317
StringInfoDatachk_current_key;
13181318

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

13361325
initStringInfo(&branchstr);
13371326
initStringInfo(&chk_branchstr);
@@ -1346,24 +1335,31 @@ build_tuplestore_recursively(char *key_fld,
13461335
/* get the next sql result tuple */
13471336
spi_tuple=tuptable->vals[i];
13481337

1349-
/* get the current keyand parent */
1338+
/* get the current key(might be NULL) */
13501339
current_key=SPI_getvalue(spi_tuple,spi_tupdesc,1);
1351-
appendStringInfo(&chk_current_key,"%s%s%s",branch_delim,current_key,branch_delim);
1352-
current_key_parent=pstrdup(SPI_getvalue(spi_tuple,spi_tupdesc,2));
1340+
1341+
/* get the parent key (might be NULL) */
1342+
current_key_parent=SPI_getvalue(spi_tuple,spi_tupdesc,2);
13531343

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

13571347
/* check to see if this key is also an ancestor */
1358-
if (strstr(chk_branchstr.data,chk_current_key.data))
1359-
elog(ERROR,"infinite recursion detected");
1348+
if (current_key)
1349+
{
1350+
appendStringInfo(&chk_current_key,"%s%s%s",
1351+
branch_delim,current_key,branch_delim);
1352+
if (strstr(chk_branchstr.data,chk_current_key.data))
1353+
elog(ERROR,"infinite recursion detected");
1354+
}
13601355

13611356
/* OK, extend the branch */
1362-
appendStringInfo(&branchstr,"%s%s",branch_delim,current_key);
1357+
if (current_key)
1358+
appendStringInfo(&branchstr,"%s%s",branch_delim,current_key);
13631359
current_branch=branchstr.data;
13641360

13651361
/* build a tuple */
1366-
values[0]=pstrdup(current_key);
1362+
values[0]=current_key;
13671363
values[1]=current_key_parent;
13681364
values[2]=current_level;
13691365
if (show_branch)
@@ -1379,30 +1375,31 @@ build_tuplestore_recursively(char *key_fld,
13791375

13801376
tuple=BuildTupleFromCStrings(attinmeta,values);
13811377

1382-
xpfree(current_key);
1383-
xpfree(current_key_parent);
1384-
13851378
/* store the tuple for later use */
13861379
tuplestore_puttuple(tupstore,tuple);
13871380

13881381
heap_freetuple(tuple);
13891382

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

14071404
/* reset branch for next pass */
14081405
resetStringInfo(&branchstr);
@@ -1414,8 +1411,6 @@ build_tuplestore_recursively(char *key_fld,
14141411
xpfree(chk_branchstr.data);
14151412
xpfree(chk_current_key.data);
14161413
}
1417-
1418-
returntupstore;
14191414
}
14201415

14211416
/*
@@ -1488,34 +1483,25 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial
14881483
/*
14891484
* Check if spi sql tupdesc and return tupdesc are compatible
14901485
*/
1491-
staticbool
1486+
staticvoid
14921487
compatConnectbyTupleDescs(TupleDescret_tupdesc,TupleDescsql_tupdesc)
14931488
{
1494-
Oidret_atttypid;
1495-
Oidsql_atttypid;
1496-
1497-
/* check the key_fld types match */
1498-
ret_atttypid=ret_tupdesc->attrs[0]->atttypid;
1499-
sql_atttypid=sql_tupdesc->attrs[0]->atttypid;
1500-
if (ret_atttypid!=sql_atttypid)
1489+
/*
1490+
* Result must have at least 2 columns.
1491+
*/
1492+
if (sql_tupdesc->natts<2)
15011493
ereport(ERROR,
15021494
(errcode(ERRCODE_SYNTAX_ERROR),
15031495
errmsg("invalid return type"),
1504-
errdetail("SQL key field datatype does " \
1505-
"not match return key field datatype.")));
1496+
errdetail("Query must return at least two columns.")));
15061497

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

15171504
/* OK, the two tupdescs are compatible for our purposes */
1518-
return true;
15191505
}
15201506

15211507
/*

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp