8000 Fix incorrect permissions-checking code for extended statistics. · postgrespro/postgres@e5fc38a · GitHub
[go: up one dir, main page]
Skip to content

Commit e5fc38a

Browse files
tglsfdctglsfdc
committed
Fix incorrect permissions-checking code for extended statistics.
Commit a4d75c8 improved the extended-stats logic to allow extended stats to be collected on expressions not just bare Vars. To apply such stats, we first verify that the user has permissions to read all columns used in the stats. (If not, the query will likely fail at runtime, but the planner ought not do so.) That had to get extended to check permissions of columns appearing within such expressions, but the code for that was completely wrong: it applied pull_varattnos to the wrong pointer, leading to "unrecognized node type" failures. Furthermore, although you couldn't get to this because of that bug, it failed to account for the attnum offset applied by pull_varattnos. This escaped recognition so far because th 8000 e code in question is not reached when the user has whole-table SELECT privilege (which is the common case), and because only subexpressions not specially handled by statext_is_compatible_clause_internal() are at risk. I think a large part of the reason for this bug is under-documentation of what statext_is_compatible_clause() is doing and what its arguments are, so do some work on the comments to try to improve that. Per bug #17570 from Alexander Kozhemyakin. Patch by Richard Guo; comments and other cosmetic improvements by me. (Thanks also to Japin Li for diagnosis.) Back-patch to v14 where the bug came in. Discussion: https://postgr.es/m/17570-f2f2e0f4bccf0965@postgresql.org
1 parent e44dae0 commit e5fc38a

File tree

3 files changed

+94
-38
lines changed

3 files changed

+94
-38
lines changed

‎src/backend/statistics/extended_stats.c

Lines changed: 86 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1316,10 +1316,38 @@ choose_best_statistics(List *stats, char requiredkind, bool inh,
13161316
* statext_is_compatible_clause_internal
13171317
* Determines if the clause is compatible with MCV lists.
13181318
*
1319-
* Does the heavy lifting of actually inspecting the clauses for
1320-
* statext_is_compatible_clause. It needs to be split like this because
1321-
* of recursion. The attnums bitmap is an input/output parameter collecting
1322-
* attribute numbers from all compatible clauses (recursively).
1319+
* To be compatible, the given clause must be a combination of supported
1320+
* clauses built from Vars or sub-expressions (where a sub-expression is
1321+
* something that exactly matches an expression found in statistics objects).
1322+
* This function recursively examines the clause and extracts any
1323+
* sub-expressions that will need to be matched against statistics.
1324+
*
1325+
* Currently, we only support the following types of clauses:
1326 8000 +
*
1327+
* (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
1328+
* the op is one of ("=", "<", ">", ">=", "<=")
1329+
*
1330+
* (b) (Var/Expr IS [NOT] NULL)
1331+
*
1332+
* (c) combinations using AND/OR/NOT
1333+
*
1334+
* (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
1335+
* op ALL (array))
1336+
*
1337+
* In the future, the range of supported clauses may be expanded to more
1338+
* complex cases, for example (Var op Var).
1339+
*
1340+
* Arguments:
1341+
* clause: (sub)clause to be inspected (bare clause, not a RestrictInfo)
1342+
* relid: rel that all Vars in clause must belong to
1343+
* *attnums: input/output parameter collecting attribute numbers of all
1344+
* mentioned Vars. Note that we do not offset the attribute numbers,
1345+
* so we can't cope with system columns.
1346+
* *exprs: input/output parameter collecting primitive subclauses within
1347+
* the clause tree
1348+
*
1349+
* Returns false if there is something we definitively can't handle.
1350+
* On true return, we can proceed to match the *exprs against statistics.
13231351
*/
13241352
static bool
13251353
statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
@@ -1343,10 +1371,14 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
13431371
if (var->varlevelsup > 0)
13441372
return false;
13451373

1346-
/* Also skip system attributes (we don't allow stats on those). */
1374+
/*
1375+
* Also reject system attributes and whole-row Vars (we don't allow
1376+
* stats on those).
1377+
*/
13471378
if (!AttrNumberIsForUserDefinedAttr(var->varattno))
13481379
return false;
13491380

1381+
/* OK, record the attnum for later permissions checks. */
13501382
*attnums = bms_add_member(*attnums, var->varattno);
13511383

13521384
return true;
@@ -1501,7 +1533,7 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
15011533
foreach(lc, expr->args)
15021534
{
15031535
/*
1504-
* Had we found incompatible clause in the arguments, treat the
1536+
* If we find an incompatible clause in the arguments, treat the
15051537
* whole clause as incompatible.
15061538
*/
15071539
if (!statext_is_compatible_clause_internal(root,
@@ -1540,27 +1572,28 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
15401572
* statext_is_compatible_clause
15411573
* Determines if the clause is compatible with MCV lists.
15421574
*
1543-
* Currently, we only support the following types of clauses:
1575+
* See statext_is_compatible_clause_internal, above, for the basic rules.
1576+
* This layer deals with RestrictInfo superstructure and applies permissions
1577+
* checks to verify that it's okay to examine all mentioned Vars.
15441578
*
1545-
* (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
1546-
* the op is one of ("=", "<", ">", ">=", "<=")
1579+
* Arguments:
1580+
* clause: clause to be inspected (in RestrictInfo form)
1581+
* relid: rel that all Vars in clause must belong to
1582+
* *attnums: input/output parameter collecting attribute numbers of all
1583+
* mentioned Vars. Note that we do not offset the attribute numbers,
1584+
* so we can't cope with system columns.
1585+
* *exprs: input/output parameter collecting primitive subclauses within
1586+
* the clause tree
15471587
*
1548-
* (b) (Var/Expr IS [NOT] NULL)
1549-
*
1550-
* (c) combinations using AND/OR/NOT
1551-
*
1552-
* (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
1553-
* op ALL (array))
1554-
*
1555-
* In the future, the range of supported clauses may be expanded to more
1556-
* complex cases, for example (Var op Var).
1588+
* Returns false if there is something we definitively can't handle.
1589+
* On true return, we can proceed to match the *exprs against statistics.
15571590
*/
15581591
static bool
15591592
statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
15601593
Bitmapset **attnums, List **exprs)
15611594
{
15621595
RangeTblEntry *rte = root->simple_rte_array[relid];
1563-
RestrictInfo *rinfo = (RestrictInfo *) clause;
1596+
RestrictInfo *rinfo;
15641597
int clause_relid;
15651598
Oid userid;
15661599

@@ -1589,8 +1622,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
15891622
}
15901623

15911624
/* Otherwise it must be a RestrictInfo. */
1592-
if (!IsA(rinfo, RestrictInfo))
1625+
if (!IsA(clause, RestrictInfo))
15931626
return false;
1627+
rinfo = (RestrictInfo *) clause;
15941628

15951629
/* Pseudoconstants are not really interesting here. */
15961630
if (rinfo->pseudoconstant)
@@ -1612,34 +1646,48 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
16121646
*/
16131647
userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
16141648

1649+
/* Table-level SELECT privilege is sufficient for all columns */
16151650
if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK)
16161651
{
16171652
Bitmapset *clause_attnums = NULL;
1653+
int attnum = -1;
16181654

1619-
/* Don't have table privilege, must check individual columns */
1620-
if (*exprs != NIL)
1655+
/*
1656+
* We have to check per-column privileges. *attnums has the attnums
1657+
* for individual Vars we saw, but there may also be Vars within
1658+
* subexpressions in *exprs. We can use pull_varattnos() to extract
1659+
* those, but there's an impedance mismatch: attnums returned by
1660+
* pull_varattnos() are offset by FirstLowInvalidHeapAttributeNumber,
1661+
* while attnums within *attnums aren't. Convert *attnums to the
1662+
* offset style so we can combine the results.
1663+
*/
1664+
while ((attnum = bms_next_member(*attnums, attnum)) >= 0)
16211665
{
1622-
pull_varattnos((Node *) exprs, relid, &clause_attnums);
1623-
clause_attnums = bms_add_members(clause_attnums, *attnums);
1666+
clause_attnums =
1667+
bms_add_member(clause_attnums,
1668+
attnum - FirstLowInvalidHeapAttributeNumber);
16241669
}
1625-
else
1626-
clause_attnums = *attnums;
16271670

1628-
if (bms_is_member(InvalidAttrNumber, clause_attnums))
1629-
{
1630-
/* Have a whole-row reference, must have access to all columns */
1631-
if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
1632-
ACLMASK_ALL) != ACLCHECK_OK)
1633-
return false;
1634-
}
1635-
else
1671+
/* Now merge attnums from *exprs into clause_attnums */
1672+
if (*exprs != NIL)
1673+
pull_varattnos((Node *) *exprs, relid, &clause_attnums);
1674+
1675+
attnum = -1;
1676+
while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
16361677
{
1637-
/* Check the columns referenced by the clause */
1638-
int attnum = -1;
1678+
/* Undo the offset */
1679+
AttrNumber attno = attnum + FirstLowInvalidHeapAttributeNumber;
16391680

1640-
while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
1681+
if (attno == InvalidAttrNumber)
1682+
{
1683+
/* Whole-row reference, so must have access to all columns */
1684+
if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
1685+
ACLMASK_ALL) != ACLCHECK_OK)
1686+
return false;
1687+
}
1688+
else
16411689
{
1642-
if (pg_attribute_aclcheck(rte->relid, attnum, userid,
1690+
if (pg_attribute_aclcheck(rte->relid, attno, userid,
16431691
ACL_SELECT) != ACLCHECK_OK)
16441692
return false;
16451693
}

‎src/test/regress/expected/stats_ext.out

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3213,6 +3213,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
32133213
SET SESSION AUTHORIZATION regress_stats_user1;
32143214
SELECT * FROM tststats.priv_test_tbl; -- Permission denied
32153215
ERROR: permission denied for table priv_test_tbl
3216+
-- Check individual columns if we don't have table privilege
3217+
SELECT * FROM tststats.priv_test_tbl
3218+
WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
3219+
ERROR: permission denied for table priv_test_tbl
32163220
-- Attempt to gain access using a leaky operator
32173221
CREATE FUNCTION op_leak(int, int) RETURNS bool
32183222
AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'

‎src/test/regress/sql/stats_ext.sql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1609,6 +1609,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
16091609
SET SESSION AUTHORIZATION regress_stats_user1;
16101610
SELECT * FROM tststats.priv_test_tbl; -- Permission denied
16111611

1612+
-- Check individual columns if we don't have table privilege
1613+
SELECT * FROM tststats.priv_test_tbl
1614+
WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
1615+
16121616
-- Attempt to gain access using a leaky operator
16131617
CREATE FUNCTION op_leak(int, int) RETURNS bool
16141618
AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'

0 commit comments

Comments
 (0)
0