You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@spark.apache.org by an...@apache.org on 2015/06/03 23:47:17 UTC
spark git commit: [MINOR] [UI] Improve confusing message on log page
Repository: spark
Updated Branches:
refs/heads/master 20a26b595 -> c6a6dd0d0
[MINOR] [UI] Improve confusing message on log page
It's good practice to check if the input path is in the directory
we expect to avoid potentially confusing error messages.
Project: http://git-wip-us.apache.org/repos/asf/spark/repo
Commit: http://git-wip-us.apache.org/repos/asf/spark/commit/c6a6dd0d
Tree: http://git-wip-us.apache.org/repos/asf/spark/tree/c6a6dd0d
Diff: http://git-wip-us.apache.org/repos/asf/spark/diff/c6a6dd0d
Branch: refs/heads/master
Commit: c6a6dd0d0736d548ff9f255e5ed5df45b29c46c1
Parents: 20a26b5
Author: Andrew Or <an...@databricks.com>
Authored: Wed Jun 3 12:10:12 2015 -0700
Committer: Andrew Or <an...@databricks.com>
Committed: Wed Jun 3 14:47:09 2015 -0700
----------------------------------------------------------------------
.../apache/spark/deploy/worker/ui/LogPage.scala | 9 +++
.../scala/org/apache/spark/util/Utils.scala | 16 +++++
.../spark/deploy/worker/ui/LogPageSuite.scala | 36 +++++++----
.../org/apache/spark/util/UtilsSuite.scala | 65 ++++++++++++++++++++
4 files changed, 115 insertions(+), 11 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/spark/blob/c6a6dd0d/core/src/main/scala/org/apache/spark/deploy/worker/ui/LogPage.scala
----------------------------------------------------------------------
diff --git a/core/src/main/scala/org/apache/spark/deploy/worker/ui/LogPage.scala b/core/src/main/scala/org/apache/spark/deploy/worker/ui/LogPage.scala
index dc2bee6..53f8f9a 100644
--- a/core/src/main/scala/org/apache/spark/deploy/worker/ui/LogPage.scala
+++ b/core/src/main/scala/org/apache/spark/deploy/worker/ui/LogPage.scala
@@ -17,6 +17,8 @@
package org.apache.spark.deploy.worker.ui
+import java.io.File
+import java.net.URI
import javax.servlet.http.HttpServletRequest
import scala.xml.Node
@@ -135,6 +137,13 @@ private[ui] class LogPage(parent: WorkerWebUI) extends WebUIPage("logPage") with
return ("Error: Log type must be one of " + supportedLogTypes.mkString(", "), 0, 0, 0)
}
+ // Verify that the normalized path of the log directory is in the working directory
+ val normalizedUri = new URI(logDirectory).normalize()
+ val normalizedLogDir = new File(normalizedUri.getPath)
+ if (!Utils.isInDirectory(workDir, normalizedLogDir)) {
+ return ("Error: invalid log directory " + logDirectory, 0, 0, 0)
+ }
+
try {
val files = RollingFileAppender.getSortedRolledOverFiles(logDirectory, logType)
logDebug(s"Sorted log files of type $logType in $logDirectory:\n${files.mkString("\n")}")
http://git-wip-us.apache.org/repos/asf/spark/blob/c6a6dd0d/core/src/main/scala/org/apache/spark/util/Utils.scala
----------------------------------------------------------------------
diff --git a/core/src/main/scala/org/apache/spark/util/Utils.scala b/core/src/main/scala/org/apache/spark/util/Utils.scala
index 693e1a0..5f13241 100644
--- a/core/src/main/scala/org/apache/spark/util/Utils.scala
+++ b/core/src/main/scala/org/apache/spark/util/Utils.scala
@@ -2227,6 +2227,22 @@ private[spark] object Utils extends Logging {
}
}
+ /**
+ * Return whether the specified file is a parent directory of the child file.
+ */
+ def isInDirectory(parent: File, child: File): Boolean = {
+ if (child == null || parent == null) {
+ return false
+ }
+ if (!child.exists() || !parent.exists() || !parent.isDirectory()) {
+ return false
+ }
+ if (parent.equals(child)) {
+ return true
+ }
+ isInDirectory(parent, child.getParentFile)
+ }
+
}
private [util] class SparkShutdownHookManager {
http://git-wip-us.apache.org/repos/asf/spark/blob/c6a6dd0d/core/src/test/scala/org/apache/spark/deploy/worker/ui/LogPageSuite.scala
----------------------------------------------------------------------
diff --git a/core/src/test/scala/org/apache/spark/deploy/worker/ui/LogPageSuite.scala b/core/src/test/scala/org/apache/spark/deploy/worker/ui/LogPageSuite.scala
index 572360d..72eaffb 100644
--- a/core/src/test/scala/org/apache/spark/deploy/worker/ui/LogPageSuite.scala
+++ b/core/src/test/scala/org/apache/spark/deploy/worker/ui/LogPageSuite.scala
@@ -19,7 +19,7 @@ package org.apache.spark.deploy.worker.ui
import java.io.{File, FileWriter}
-import org.mockito.Mockito.mock
+import org.mockito.Mockito.{mock, when}
import org.scalatest.PrivateMethodTester
import org.apache.spark.SparkFunSuite
@@ -28,33 +28,47 @@ class LogPageSuite extends SparkFunSuite with PrivateMethodTester {
test("get logs simple") {
val webui = mock(classOf[WorkerWebUI])
+ val tmpDir = new File(sys.props("java.io.tmpdir"))
+ val workDir = new File(tmpDir, "work-dir")
+ workDir.mkdir()
+ when(webui.workDir).thenReturn(workDir)
val logPage = new LogPage(webui)
// Prepare some fake log files to read later
val out = "some stdout here"
val err = "some stderr here"
- val tmpDir = new File(sys.props("java.io.tmpdir"))
- val tmpOut = new File(tmpDir, "stdout")
- val tmpErr = new File(tmpDir, "stderr")
- val tmpRand = new File(tmpDir, "random")
+ val tmpOut = new File(workDir, "stdout")
+ val tmpErr = new File(workDir, "stderr")
+ val tmpErrBad = new File(tmpDir, "stderr") // outside the working directory
+ val tmpOutBad = new File(tmpDir, "stdout")
+ val tmpRand = new File(workDir, "random")
write(tmpOut, out)
write(tmpErr, err)
+ write(tmpOutBad, out)
+ write(tmpErrBad, err)
write(tmpRand, "1 6 4 5 2 7 8")
// Get the logs. All log types other than "stderr" or "stdout" will be rejected
val getLog = PrivateMethod[(String, Long, Long, Long)]('getLog)
val (stdout, _, _, _) =
- logPage invokePrivate getLog(tmpDir.getAbsolutePath, "stdout", None, 100)
+ logPage invokePrivate getLog(workDir.getAbsolutePath, "stdout", None, 100)
val (stderr, _, _, _) =
- logPage invokePrivate getLog(tmpDir.getAbsolutePath, "stderr", None, 100)
+ logPage invokePrivate getLog(workDir.getAbsolutePath, "stderr", None, 100)
val (error1, _, _, _) =
- logPage invokePrivate getLog(tmpDir.getAbsolutePath, "random", None, 100)
+ logPage invokePrivate getLog(workDir.getAbsolutePath, "random", None, 100)
val (error2, _, _, _) =
- logPage invokePrivate getLog(tmpDir.getAbsolutePath, "does-not-exist.txt", None, 100)
+ logPage invokePrivate getLog(workDir.getAbsolutePath, "does-not-exist.txt", None, 100)
+ // These files exist, but live outside the working directory
+ val (error3, _, _, _) =
+ logPage invokePrivate getLog(tmpDir.getAbsolutePath, "stderr", None, 100)
+ val (error4, _, _, _) =
+ logPage invokePrivate getLog(tmpDir.getAbsolutePath, "stdout", None, 100)
assert(stdout === out)
assert(stderr === err)
- assert(error1.startsWith("Error"))
- assert(error2.startsWith("Error"))
+ assert(error1.startsWith("Error: Log type must be one of "))
+ assert(error2.startsWith("Error: Log type must be one of "))
+ assert(error3.startsWith("Error: invalid log directory"))
+ assert(error4.startsWith("Error: invalid log directory"))
}
/** Write the specified string to the file. */
http://git-wip-us.apache.org/repos/asf/spark/blob/c6a6dd0d/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
----------------------------------------------------------------------
diff --git a/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala b/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
index a867cf8..a61ea39 100644
--- a/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
+++ b/core/src/test/scala/org/apache/spark/util/UtilsSuite.scala
@@ -608,4 +608,69 @@ class UtilsSuite extends SparkFunSuite with ResetSystemProperties with Logging {
manager.runAll()
assert(output.toList === List(4, 3, 2))
}
+
+ test("isInDirectory") {
+ val tmpDir = new File(sys.props("java.io.tmpdir"))
+ val parentDir = new File(tmpDir, "parent-dir")
+ val childDir1 = new File(parentDir, "child-dir-1")
+ val childDir1b = new File(parentDir, "child-dir-1b")
+ val childFile1 = new File(parentDir, "child-file-1.txt")
+ val childDir2 = new File(childDir1, "child-dir-2")
+ val childDir2b = new File(childDir1, "child-dir-2b")
+ val childFile2 = new File(childDir1, "child-file-2.txt")
+ val childFile3 = new File(childDir2, "child-file-3.txt")
+ val nullFile: File = null
+ parentDir.mkdir()
+ childDir1.mkdir()
+ childDir1b.mkdir()
+ childDir2.mkdir()
+ childDir2b.mkdir()
+ childFile1.createNewFile()
+ childFile2.createNewFile()
+ childFile3.createNewFile()
+
+ // Identity
+ assert(Utils.isInDirectory(parentDir, parentDir))
+ assert(Utils.isInDirectory(childDir1, childDir1))
+ assert(Utils.isInDirectory(childDir2, childDir2))
+
+ // Valid ancestor-descendant pairs
+ assert(Utils.isInDirectory(parentDir, childDir1))
+ assert(Utils.isInDirectory(parentDir, childFile1))
+ assert(Utils.isInDirectory(parentDir, childDir2))
+ assert(Utils.isInDirectory(parentDir, childFile2))
+ assert(Utils.isInDirectory(parentDir, childFile3))
+ assert(Utils.isInDirectory(childDir1, childDir2))
+ assert(Utils.isInDirectory(childDir1, childFile2))
+ assert(Utils.isInDirectory(childDir1, childFile3))
+ assert(Utils.isInDirectory(childDir2, childFile3))
+
+ // Inverted ancestor-descendant pairs should fail
+ assert(!Utils.isInDirectory(childDir1, parentDir))
+ assert(!Utils.isInDirectory(childDir2, parentDir))
+ assert(!Utils.isInDirectory(childDir2, childDir1))
+ assert(!Utils.isInDirectory(childFile1, parentDir))
+ assert(!Utils.isInDirectory(childFile2, parentDir))
+ assert(!Utils.isInDirectory(childFile3, parentDir))
+ assert(!Utils.isInDirectory(childFile2, childDir1))
+ assert(!Utils.isInDirectory(childFile3, childDir1))
+ assert(!Utils.isInDirectory(childFile3, childDir2))
+
+ // Non-existent files or directories should fail
+ assert(!Utils.isInDirectory(parentDir, new File(parentDir, "one.txt")))
+ assert(!Utils.isInDirectory(parentDir, new File(parentDir, "one/two.txt")))
+ assert(!Utils.isInDirectory(parentDir, new File(parentDir, "one/two/three.txt")))
+
+ // Siblings should fail
+ assert(!Utils.isInDirectory(childDir1, childDir1b))
+ assert(!Utils.isInDirectory(childDir1, childFile1))
+ assert(!Utils.isInDirectory(childDir2, childDir2b))
+ assert(!Utils.isInDirectory(childDir2, childFile2))
+
+ // Null files should fail without throwing NPE
+ assert(!Utils.isInDirectory(parentDir, nullFile))
+ assert(!Utils.isInDirectory(childFile3, nullFile))
+ assert(!Utils.isInDirectory(nullFile, parentDir))
+ assert(!Utils.isInDirectory(nullFile, childFile3))
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@spark.apache.org
For additional commands, e-mail: commits-help@spark.apache.org