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

Commit290c2da

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 parent78f8c67 commit290c2da

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(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
@@ -56,7 +56,7 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
5656
boolrandomAccess);
5757
staticvoidvalidateConnectbyTupleDesc(TupleDesctupdesc,boolshow_branch,boolshow_serial);
5858
staticboolcompatCrosstabTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
59-
staticboolcompatConnectbyTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
59+
staticvoidcompatConnectbyTupleDescs(TupleDesctupdesc1,TupleDesctupdesc2);
6060
staticvoidget_normal_pair(float8*x1,float8*x2);
6161
staticTuplestorestate*connectby(char*relname,
6262
char*key_fld,
@@ -70,7 +70,7 @@ static Tuplestorestate *connectby(char *relname,
7070
MemoryContextper_query_ctx,
7171
boolrandomAccess,
7272
AttInMetadata*attinmeta);
73-
staticTuplestorestate*build_tuplestore_recursively(char*key_fld,
73+
staticvoidbuild_tuplestore_recursively(char*key_fld,
7474
char*parent_key_fld,
7575
char*relname,
7676
char*orderby_fld,
@@ -1180,28 +1180,28 @@ connectby(char *relname,
11801180
MemoryContextSwitchTo(oldcontext);
11811181

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

11991199
SPI_finish();
12001200

12011201
returntupstore;
12021202
}
12031203

1204-
staticTuplestorestate*
1204+
staticvoid
12051205
build_tuplestore_recursively(char*key_fld,
12061206
char*parent_key_fld,
12071207
char*relname,
@@ -1232,7 +1232,7 @@ build_tuplestore_recursively(char *key_fld,
12321232
HeapTupletuple;
12331233

12341234
if (max_depth>0&&level>max_depth)
1235-
returntupstore;
1235+
return;
12361236

12371237
initStringInfo(&sql);
12381238

@@ -1318,22 +1318,11 @@ build_tuplestore_recursively(char *key_fld,
13181318
StringInfoDatachk_branchstr;
13191319
StringInfoDatachk_current_key;
13201320

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

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

1351-
/* get the current keyand parent */
1340+
/* get the current key(might be NULL) */
13521341
current_key=SPI_getvalue(spi_tuple,spi_tupdesc,1);
1353-
appendStringInfo(&chk_current_key,"%s%s%s",branch_delim,current_key,branch_delim);
1354-
current_key_parent=pstrdup(SPI_getvalue(spi_tuple,spi_tupdesc,2));
1342+
1343+
/* get the parent key (might be NULL) */
1344+
current_key_parent=SPI_getvalue(spi_tuple,spi_tupdesc,2);
13551345

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

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

13631358
/* OK, extend the branch */
1364-
appendStringInfo(&branchstr,"%s%s",branch_delim,current_key);
1359+
if (current_key)
1360+
appendStringInfo(&branchstr,"%s%s",branch_delim,current_key);
13651361
current_branch=branchstr.data;
13661362

13671363
/* build a tuple */
1368-
values[0]=pstrdup(current_key);
1364+
values[0]=current_key;
13691365
values[1]=current_key_parent;
13701366
values[2]=current_level;
13711367
if (show_branch)
@@ -1381,30 +1377,31 @@ build_tuplestore_recursively(char *key_fld,
13811377

13821378
tuple=BuildTupleFromCStrings(attinmeta,values);
13831379

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

13901383
heap_freetuple(tuple);
13911384

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

14091406
/* reset branch for next pass */
14101407
resetStringInfo(&branchstr);
@@ -1416,8 +1413,6 @@ build_tuplestore_recursively(char *key_fld,
14161413
xpfree(chk_branchstr.data);
14171414
xpfree(chk_current_key.data);
14181415
}
1419-
1420-
returntupstore;
14211416
}
14221417

14231418
/*
@@ -1490,34 +1485,25 @@ validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial
14901485
/*
14911486
* Check if spi sql tupdesc and return tupdesc are compatible
14921487
*/
1493-
staticbool
1488+
staticvoid
14941489
compatConnectbyTupleDescs(TupleDescret_tupdesc,TupleDescsql_tupdesc)
14951490
{
1496-
Oidret_atttypid;
1497-
Oidsql_atttypid;
1498-
1499-
/* check the key_fld types match */
1500-
ret_atttypid=ret_tupdesc->attrs[0]->atttypid;
1501-
sql_atttypid=sql_tupdesc->attrs[0]->atttypid;
1502-
if (ret_atttypid!=sql_atttypid)
1491+
/*
1492+
* Result must have at least 2 columns.
1493+
*/
1494+
if (sql_tupdesc->natts<2)
15031495
ereport(ERROR,
15041496
(errcode(ERRCODE_SYNTAX_ERROR),
15051497
errmsg("invalid return type"),
1506-
errdetail("SQL key field datatype does " \
1507-
"not match return key field datatype.")));
1498+
errdetail("Query must return at least two columns.")));
15081499

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

15191506
/* OK, the two tupdescs are compatible for our purposes */
1520-
return true;
15211507
}
15221508

15231509
/*

0 commit comments

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