dongjoon-hyun commented on code in PR #58604:
URL: https://github.com/apache/spark/pull/58604#discussion_r3957190535
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,37 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or
scripts when the
+ * restricted execution mode is enabled. Unlike the checks in
`checkAnalysis0`, this walks the
+ * whole plan -- including already-analyzed sub-plans reused from a
temporary view or a cached
+ * Dataset -- and descends into subquery plans, so these features cannot
slip through on a plan
+ * that was analyzed while the mode was off.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until
optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right
function name.
+ case _: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId("try_reflect")} function")
+ case c: CallMethodViaReflection =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId(c.prettyName)} function")
+ case s: SubqueryExpression =>
+ checkPlan(s.plan)
+ case _ =>
+ }
+ def checkPlan(p: LogicalPlan): Unit = p.foreach {
Review Comment:
`p.foreach` follows `children`, and that misses the query of every
`AnalysisOnlyCommand`.
`checkAnalysis` runs *after* `markAsAnalyzed`, and at that point
(`Command.scala`):
```scala
override final def children: Seq[LogicalPlan] = if (isAnalyzed) Nil else
childrenToAnalyze
override def innerChildren: Seq[QueryPlan[_]] = if (isAnalyzed)
childrenToAnalyze else Nil
```
so the body has moved out of `children` into `innerChildren`. That covers
`V2CreateTableAsSelectPlan` (CTAS / RTAS), `CacheTableAsSelect`, `CreateView` /
`CreateViewCommand`, and `AlterViewAs` / `AlterViewAsCommand`.
So these look like they are not rejected:
```sql
CREATE TABLE t AS SELECT reflect('java.lang.Runtime', 'getRuntime');
CACHE TABLE t AS SELECT TRANSFORM(a) USING 'evil.sh' AS (b) FROM src;
```
and there is no second chance later: the stored `query` is already analyzed,
so `Analyzer.executeAndCheck` returns early (`if (plan.analyzed) plan`) and
`checkAnalysis` never runs again before the command executes it. `CREATE VIEW`
is less severe since the body is re-analyzed on read, but CTAS and `CACHE TABLE
... AS SELECT` run the query right there.
Descending into `innerChildren` as well should close this:
```scala
def checkPlan(p: LogicalPlan): Unit = p.foreach {
case _: ScriptTransformation =>
throw QueryCompilationErrors.restrictedModeFeatureError(
"The TRANSFORM ... USING clause")
case node =>
node.expressions.foreach(checkExpression)
node.innerChildren.foreach {
case inner: LogicalPlan => checkPlan(inner)
case _ =>
}
}
```
Worth a test for each of CTAS and `CACHE TABLE ... AS SELECT`.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala:
##########
@@ -339,6 +342,37 @@ trait CheckAnalysis extends LookupCatalog with
QueryErrorsBase with PlanToString
plan.setAnalyzed()
}
+ /**
+ * Rejects SQL features that load or execute externally provided code or
scripts when the
+ * restricted execution mode is enabled. Unlike the checks in
`checkAnalysis0`, this walks the
+ * whole plan -- including already-analyzed sub-plans reused from a
temporary view or a cached
+ * Dataset -- and descends into subquery plans, so these features cannot
slip through on a plan
+ * that was analyzed while the mode was off.
+ */
+ private def checkRestrictedMode(plan: LogicalPlan): Unit = {
+ def checkExpression(expr: Expression): Unit = expr.foreach {
+ // A try_reflect call stays a RuntimeReplaceable wrapper until
optimization, so match it
+ // before its CallMethodViaReflection replacement to report the right
function name.
+ case _: TryReflect =>
+ throw QueryCompilationErrors.restrictedModeFeatureError(
+ s"The ${toSQLId("try_reflect")} function")
Review Comment:
Nit: `TryReflect.prettyName` is already `"try_reflect"`, so this could be
`case t: TryReflect => ... toSQLId(t.prettyName)`, matching the
`CallMethodViaReflection` branch just below.
(The ordering comment above it is accurate, by the way --
`InheritAnalysisRules` makes `replacement` the child, and `Expression.foreach`
is pre-order, so the wrapper is matched first.)
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project,
ScriptInputOutputSchema, ScriptTransformation}
+import org.apache.spark.sql.internal.{SQLConf, StaticSQLConf}
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = StaticSQLConf.RESTRICTED_MODE_ENABLED.key
+ // The config appears in the error message double-quoted, as `toSQLConf`
renders it.
+ private val configName = "\"" + restricted + "\""
+
+ // The mode is a static config, so `withSQLConf` (which rejects static keys)
cannot toggle it.
+ // Set it directly on the active conf instead, restoring the previous value
afterwards.
+ private def withRestrictedMode[T](enabled: Boolean)(f: => T): T = {
Review Comment:
This helper writes the static config straight onto the active `SQLConf`,
which is a reasonable way to toggle it in a catalyst unit test -- but it also
means nothing in the suite covers the guarantee this PR is built on, namely
that a session *cannot* turn the mode off.
A test asserting that `SET spark.sql.restrictedMode.enabled=false` fails
with `CANNOT_MODIFY_CONFIG` would be worth adding (in `sql/core`, since it
needs a session).
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/RestrictedModeSuite.scala:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.catalyst.analysis
+
+import org.apache.spark.sql.AnalysisException
+import org.apache.spark.sql.catalyst.expressions.{AttributeReference, Literal}
+import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project,
ScriptInputOutputSchema, ScriptTransformation}
+import org.apache.spark.sql.internal.{SQLConf, StaticSQLConf}
+import org.apache.spark.sql.types.StringType
+
+class RestrictedModeSuite extends AnalysisTest {
+
+ private val restricted = StaticSQLConf.RESTRICTED_MODE_ENABLED.key
+ // The config appears in the error message double-quoted, as `toSQLConf`
renders it.
+ private val configName = "\"" + restricted + "\""
+
+ // The mode is a static config, so `withSQLConf` (which rejects static keys)
cannot toggle it.
+ // Set it directly on the active conf instead, restoring the previous value
afterwards.
+ private def withRestrictedMode[T](enabled: Boolean)(f: => T): T = {
+ val conf = SQLConf.get
+ val previous = if (conf.contains(restricted))
Some(conf.getConfString(restricted)) else None
+ conf.setConfString(restricted, enabled.toString)
+ try f finally {
+ previous match {
+ case Some(value) => conf.setConfString(restricted, value)
+ case None => conf.unsetConf(restricted)
+ }
+ }
+ }
+
+ private def functionProject(name: String): LogicalPlan =
+ Project(
+ Seq(UnresolvedAlias(
+ UnresolvedFunction(
+ name,
+ Seq(Literal("java.lang.Math"), Literal("abs"), Literal(-1)),
+ isDistinct = false))),
+ TestRelations.testRelation)
+
+ private def transformPlan: ScriptTransformation =
+ ScriptTransformation(
+ "cat",
+ Seq(AttributeReference("value", StringType)()),
+ TestRelations.testRelation,
+ ScriptInputOutputSchema(Nil, Nil, None, None, Nil, Nil, None, None,
schemaLess = false))
+
+ private def checkRestrictedError(e: AnalysisException, feature: String):
Unit = {
+ checkError(
+ exception = e,
+ condition = "UNSUPPORTED_FEATURE.SQL_RESTRICTED_MODE",
+ parameters = Map("feature" -> feature, "config" -> configName))
+ }
+
+ test("reflect / java_method / try_reflect are rejected only when restricted
mode is enabled") {
+ Seq("reflect", "java_method", "try_reflect").foreach { fn =>
+ withRestrictedMode(true) {
+ val analyzer = getAnalyzer
+ val e = intercept[AnalysisException] {
+ analyzer.checkAnalysis(analyzer.execute(functionProject(fn)))
+ }
+ checkRestrictedError(e, s"The `$fn` function")
+ }
+ withRestrictedMode(false) {
+ val analyzer = getAnalyzer
+ analyzer.checkAnalysis(analyzer.execute(functionProject(fn)))
+ }
+ }
+ }
+
+ test("TRANSFORM ... USING is rejected only when restricted mode is enabled")
{
+ withRestrictedMode(true) {
+ val analyzer = getAnalyzer
+ val e = intercept[AnalysisException] {
+ analyzer.checkAnalysis(analyzer.execute(transformPlan))
+ }
+ checkRestrictedError(e, "The TRANSFORM ... USING clause")
+ }
+ withRestrictedMode(false) {
+ val analyzer = getAnalyzer
+ // Analysis succeeds (no restricted-mode error) when the profile is off.
+ analyzer.checkAnalysis(analyzer.execute(transformPlan))
+ }
+ }
+
+ test("restricted mode is enforced even for an already-analyzed sub-plan") {
Review Comment:
This exercises a path that cannot occur at runtime.
`Analyzer.executeAndCheck` starts with
```scala
if (plan.analyzed) {
plan
} else {
```
so a top-level plan that is entirely analyzed never reaches `checkAnalysis`
at all -- passing one in directly only tests the traversal in isolation.
The case that does happen is an analyzed *sub*-plan under a fresh parent
(referencing a temp view, reusing a Dataset). Wrapping `analyzed` in something
like a `Project` before calling `checkAnalysis` would cover the real shape, and
would also be the case that `checkAnalysis0`'s `case p if p.analyzed` skips --
which is the reason this method exists.
Related: now that the config is static it cannot change within a JVM, so "a
plan that was analyzed while the mode was off" (here and in the
`checkRestrictedMode` scaladoc) cannot really arise. The traversal is still
worth having for the `p.analyzed` skip, but the justification in both comments
overstates the case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
