Fix incorrect permissions-checking code for extended statistics.

tglsfdc · tglsfdc · commit e5fc38ac303d · 2022-08-05T12:46:44.000-04:00
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 the 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
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
@@ -1316,10 +1316,38 @@ choose_best_statistics(List *stats, char requiredkind, bool inh,
  * statext_is_compatible_clause_internal
  *		Determines if the clause is compatible with MCV lists.
  *
- * Does the heavy lifting of actually inspecting the clauses for
- * statext_is_compatible_clause. It needs to be split like this because
- * of recursion.  The attnums bitmap is an input/output parameter collecting
- * attribute numbers from all compatible clauses (recursively).
+ * To be compatible, the given clause must be a combination of supported
+ * clauses built from Vars or sub-expressions (where a sub-expression is
+ * something that exactly matches an expression found in statistics objects).
+ * This function recursively examines the clause and extracts any
+ * sub-expressions that will need to be matched against statistics.
+ *
+ * Currently, we only support the following types of clauses:
+ *
+ * (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
+ * the op is one of ("=", "<", ">", ">=", "<=")
+ *
+ * (b) (Var/Expr IS [NOT] NULL)
+ *
+ * (c) combinations using AND/OR/NOT
+ *
+ * (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
+ * op ALL (array))
+ *
+ * In the future, the range of supported clauses may be expanded to more
+ * complex cases, for example (Var op Var).
+ *
+ * Arguments:
+ * clause: (sub)clause to be inspected (bare clause, not a RestrictInfo)
+ * relid: rel that all Vars in clause must belong to
+ * *attnums: input/output parameter collecting attribute numbers of all
+ *		mentioned Vars.  Note that we do not offset the attribute numbers,
+ *		so we can't cope with system columns.
+ * *exprs: input/output parameter collecting primitive subclauses within
+ *		the clause tree
+ *
+ * Returns false if there is something we definitively can't handle.
+ * On true return, we can proceed to match the *exprs against statistics.
  */
 static bool
 statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
@@ -1343,10 +1371,14 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
 		if (var->varlevelsup > 0)
 			return false;
 
-		/* Also skip system attributes (we don't allow stats on those). */
+		/*
+		 * Also reject system attributes and whole-row Vars (we don't allow
+		 * stats on those).
+		 */
 		if (!AttrNumberIsForUserDefinedAttr(var->varattno))
 			return false;
 
+		/* OK, record the attnum for later permissions checks. */
 		*attnums = bms_add_member(*attnums, var->varattno);
 
 		return true;
@@ -1501,7 +1533,7 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
 		foreach(lc, expr->args)
 		{
 			/*
-			 * Had we found incompatible clause in the arguments, treat the
+			 * If we find an incompatible clause in the arguments, treat the
 			 * whole clause as incompatible.
 			 */
 			if (!statext_is_compatible_clause_internal(root,
@@ -1540,27 +1572,28 @@ statext_is_compatible_clause_internal(PlannerInfo *root, Node *clause,
  * statext_is_compatible_clause
  *		Determines if the clause is compatible with MCV lists.
  *
- * Currently, we only support the following types of clauses:
+ * See statext_is_compatible_clause_internal, above, for the basic rules.
+ * This layer deals with RestrictInfo superstructure and applies permissions
+ * checks to verify that it's okay to examine all mentioned Vars.
  *
- * (a) OpExprs of the form (Var/Expr op Const), or (Const op Var/Expr), where
- * the op is one of ("=", "<", ">", ">=", "<=")
+ * Arguments:
+ * clause: clause to be inspected (in RestrictInfo form)
+ * relid: rel that all Vars in clause must belong to
+ * *attnums: input/output parameter collecting attribute numbers of all
+ *		mentioned Vars.  Note that we do not offset the attribute numbers,
+ *		so we can't cope with system columns.
+ * *exprs: input/output parameter collecting primitive subclauses within
+ *		the clause tree
  *
- * (b) (Var/Expr IS [NOT] NULL)
- *
- * (c) combinations using AND/OR/NOT
- *
- * (d) ScalarArrayOpExprs of the form (Var/Expr op ANY (array)) or (Var/Expr
- * op ALL (array))
- *
- * In the future, the range of supported clauses may be expanded to more
- * complex cases, for example (Var op Var).
+ * Returns false if there is something we definitively can't handle.
+ * On true return, we can proceed to match the *exprs against statistics.
  */
 static bool
 statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
 							 Bitmapset **attnums, List **exprs)
 {
 	RangeTblEntry *rte = root->simple_rte_array[relid];
-	RestrictInfo *rinfo = (RestrictInfo *) clause;
+	RestrictInfo *rinfo;
 	int			clause_relid;
 	Oid			userid;
 
@@ -1589,8 +1622,9 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
 	}
 
 	/* Otherwise it must be a RestrictInfo. */
-	if (!IsA(rinfo, RestrictInfo))
+	if (!IsA(clause, RestrictInfo))
 		return false;
+	rinfo = (RestrictInfo *) clause;
 
 	/* Pseudoconstants are not really interesting here. */
 	if (rinfo->pseudoconstant)
@@ -1612,34 +1646,48 @@ statext_is_compatible_clause(PlannerInfo *root, Node *clause, Index relid,
 	 */
 	userid = rte->checkAsUser ? rte->checkAsUser : GetUserId();
 
+	/* Table-level SELECT privilege is sufficient for all columns */
 	if (pg_class_aclcheck(rte->relid, userid, ACL_SELECT) != ACLCHECK_OK)
 	{
 		Bitmapset  *clause_attnums = NULL;
+		int			attnum = -1;
 
-		/* Don't have table privilege, must check individual columns */
-		if (*exprs != NIL)
+		/*
+		 * We have to check per-column privileges.  *attnums has the attnums
+		 * for individual Vars we saw, but there may also be Vars within
+		 * subexpressions in *exprs.  We can use pull_varattnos() to extract
+		 * those, but there's an impedance mismatch: attnums returned by
+		 * pull_varattnos() are offset by FirstLowInvalidHeapAttributeNumber,
+		 * while attnums within *attnums aren't.  Convert *attnums to the
+		 * offset style so we can combine the results.
+		 */
+		while ((attnum = bms_next_member(*attnums, attnum)) >= 0)
 		{
-			pull_varattnos((Node *) exprs, relid, &clause_attnums);
-			clause_attnums = bms_add_members(clause_attnums, *attnums);
+			clause_attnums =
+				bms_add_member(clause_attnums,
+							   attnum - FirstLowInvalidHeapAttributeNumber);
 		}
-		else
-			clause_attnums = *attnums;
 
-		if (bms_is_member(InvalidAttrNumber, clause_attnums))
-		{
-			/* Have a whole-row reference, must have access to all columns */
-			if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
-										  ACLMASK_ALL) != ACLCHECK_OK)
-				return false;
-		}
-		else
+		/* Now merge attnums from *exprs into clause_attnums */
+		if (*exprs != NIL)
+			pull_varattnos((Node *) *exprs, relid, &clause_attnums);
+
+		attnum = -1;
+		while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
 		{
-			/* Check the columns referenced by the clause */
-			int			attnum = -1;
+			/* Undo the offset */
+			AttrNumber	attno = attnum + FirstLowInvalidHeapAttributeNumber;
 
-			while ((attnum = bms_next_member(clause_attnums, attnum)) >= 0)
+			if (attno == InvalidAttrNumber)
+			{
+				/* Whole-row reference, so must have access to all columns */
+				if (pg_attribute_aclcheck_all(rte->relid, userid, ACL_SELECT,
+											  ACLMASK_ALL) != ACLCHECK_OK)
+					return false;
+			}
+			else
 			{
-				if (pg_attribute_aclcheck(rte->relid, attnum, userid,
+				if (pg_attribute_aclcheck(rte->relid, attno, userid,
 										  ACL_SELECT) != ACLCHECK_OK)
 					return false;
 			}
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
@@ -3213,6 +3213,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
 SET SESSION AUTHORIZATION regress_stats_user1;
 SELECT * FROM tststats.priv_test_tbl; -- Permission denied
 ERROR:  permission denied for table priv_test_tbl
+-- Check individual columns if we don't have table privilege
+SELECT * FROM tststats.priv_test_tbl
+  WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
+ERROR:  permission denied for table priv_test_tbl
 -- Attempt to gain access using a leaky operator
 CREATE FUNCTION op_leak(int, int) RETURNS bool
     AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
@@ -1609,6 +1609,10 @@ GRANT USAGE ON SCHEMA tststats TO regress_stats_user1;
 SET SESSION AUTHORIZATION regress_stats_user1;
 SELECT * FROM tststats.priv_test_tbl; -- Permission denied
 
+-- Check individual columns if we don't have table privilege
+SELECT * FROM tststats.priv_test_tbl
+  WHERE a = 1 and tststats.priv_test_tbl.* > (1, 1) is not null;
+
 -- Attempt to gain access using a leaky operator
 CREATE FUNCTION op_leak(int, int) RETURNS bool
     AS 'BEGIN RAISE NOTICE ''op_leak => %, %'', $1, $2; RETURN $1 < $2; END'
