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

Commit5300adc

Browse files
tglsfdcpull[bot]
authored andcommitted
Fix incorrect permissions-checking code for extended statistics.
Commita4d75c8 improved the extended-stats logic to allow extendedstats to be collected on expressions not just bare Vars. To applysuch stats, we first verify that the user has permissions to read allcolumns used in the stats. (If not, the query will likely fail atruntime, but the planner ought not do so.) That had to get extendedto check permissions of columns appearing within such expressions,but the code for that was completely wrong: it applied pull_varattnosto 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 the code in question is notreached when the user has whole-table SELECT privilege (which is thecommon case), and because only subexpressions not specially handledby statext_is_compatible_clause_internal() are at risk.I think a large part of the reason for this bug is under-documentationof what statext_is_compatible_clause() is doing and what its argumentsare, 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 toJapin Li for diagnosis.) Back-patch to v14 where the bug came in.Discussion:https://postgr.es/m/17570-f2f2e0f4bccf0965@postgresql.org
1 parentef16d60 commit5300adc

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+
*
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
staticbool
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 wefound incompatible clause in the arguments, treat the
1536+
*If wefind 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
staticbool
15591592
statext_is_compatible_clause(PlannerInfo*root,Node*clause,Indexrelid,
15601593
Bitmapset**attnums,List**exprs)
15611594
{
15621595
RangeTblEntry*rte=root->simple_rte_array[relid];
1563-
RestrictInfo*rinfo= (RestrictInfo*)clause;
1596+
RestrictInfo*rinfo;
15641597
intclause_relid;
15651598
Oiduserid;
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+
intattnum=-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 thecolumns referenced by the clause */
1638-
intattnum=-1;
1678+
/*Undo theoffset */
1679+
AttrNumberattno=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*FROMtststats.priv_test_tbl;-- Permission denied
16111611

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

0 commit comments

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