Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit c0549ce

Browse files
committed
Teach contain_leaked_vars that assignment SubscriptingRefs are leaky.
array_get_element and array_get_slice qualify as leakproof, since they will silently return NULL for bogus subscripts. But array_set_element and array_set_slice throw errors for such cases, making them clearly not leakproof. contain_leaked_vars was evidently written with only the former case in mind, as it gave the wrong answer for assignment SubscriptingRefs (nee ArrayRefs). This would be a live security bug, were it not that assignment SubscriptingRefs can only occur in INSERT and UPDATE target lists, while we only care about leakproofness for qual expressions; so the wrong answer can't occur in practice. Still, that's a rather shaky answer for a security-related question; and maybe in future somebody will want to ask about leakproofness of a tlist. So it seems wise to fix and even back-patch this correction. (We would need some change here anyway for the upcoming generic-subscripting patch, since extensions might make different tradeoffs about whether to throw errors. Commit 558d77f attempted to lay groundwork for that by asking check_functions_in_node whether a SubscriptingRef contains leaky functions; but that idea fails now that the implementation methods of a SubscriptingRef are not SQL-visible functions that could be marked leakproof or not.) Back-patch to 9.6. While 9.5 has the same issue, the code's a bit different. It seems quite unlikely that we'd introduce any actual bug in the short time 9.5 has left to live, so the work/risk/reward balance isn't attractive for changing 9.5. Discussion: https://postgr.es/m/3143742.1607368115@sss.pgh.pa.us
1 parent c6f8d17 commit c0549ce

File tree

1 file changed

+17
-1
lines changed

1 file changed

+17
-1
lines changed

src/backend/optimizer/util/clauses.c

+17-1
Original file line numberDiff line numberDiff line change
@@ -1409,7 +1409,6 @@ contain_leaked_vars_walker(Node *node, void *context)
14091409
case T_ScalarArrayOpExpr:
14101410
case T_CoerceViaIO:
14111411
case T_ArrayCoerceExpr:
1412-
case T_SubscriptingRef:
14131412

14141413
/*
14151414
* If node contains a leaky function call, and there's any Var
@@ -1421,6 +1420,23 @@ contain_leaked_vars_walker(Node *node, void *context)
14211420
return true;
14221421
break;
14231422

1423+
case T_SubscriptingRef:
1424+
{
1425+
SubscriptingRef *sbsref = (SubscriptingRef *) node;
1426+
1427+
/*
1428+
* subscripting assignment is leaky, but subscripted fetches
1429+
* are not
1430+
*/
1431+
if (sbsref->refassgnexpr != NULL)
1432+
{
1433+
/* Node is leaky, so reject if it contains Vars */
1434+
if (contain_var_clause(node))
1435+
return true;
1436+
}
1437+
}
1438+
break;
1439+
14241440
case T_RowCompareExpr:
14251441
{
14261442
/*

0 commit comments

Comments
 (0)