You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by jh...@apache.org on 2017/06/06 03:00:37 UTC
[2/5] calcite git commit: [CALCITE-1830] ProcessBuilder is security
sensitive; move it to test suite to prevent accidents
[CALCITE-1830] ProcessBuilder is security sensitive; move it to test suite to prevent accidents
Project: http://git-wip-us.apache.org/repos/asf/calcite/repo
Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/d393cab2
Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/d393cab2
Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/d393cab2
Branch: refs/heads/master
Commit: d393cab26e72f3c0de5eebcbeac5d94fff87a997
Parents: 4069013
Author: Julian Hyde <jh...@apache.org>
Authored: Mon Jun 5 11:38:12 2017 -0700
Committer: Julian Hyde <jh...@apache.org>
Committed: Mon Jun 5 15:53:34 2017 -0700
----------------------------------------------------------------------
.../main/java/org/apache/calcite/util/Util.java | 117 -------------------
.../concurrent/ConcurrentTestCommandScript.java | 78 ++++++++++++-
pom.xml | 2 +
src/main/config/forbidden-apis/signatures.txt | 9 ++
4 files changed, 86 insertions(+), 120 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/calcite/blob/d393cab2/core/src/main/java/org/apache/calcite/util/Util.java
----------------------------------------------------------------------
diff --git a/core/src/main/java/org/apache/calcite/util/Util.java b/core/src/main/java/org/apache/calcite/util/Util.java
index e0746e2..3430f01 100644
--- a/core/src/main/java/org/apache/calcite/util/Util.java
+++ b/core/src/main/java/org/apache/calcite/util/Util.java
@@ -41,8 +41,6 @@ import com.google.common.collect.ImmutableMap;
import org.slf4j.Logger;
-import java.io.BufferedInputStream;
-import java.io.BufferedOutputStream;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
@@ -1536,121 +1534,6 @@ public class Util {
}
/**
- * Runs an external application.
- *
- * @param cmdarray command and arguments, see {@link ProcessBuilder}
- * @param logger if not null, command and exit status will be logged
- * @param appInput if not null, data will be copied to application's stdin
- * @param appOutput if not null, data will be captured from application's
- * stdout and stderr
- * @return application process exit value
- * @throws IOException
- * @throws InterruptedException
- */
- @Deprecated // to be removed before 2.0
- public static int runApplication(
- String[] cmdarray,
- Logger logger,
- Reader appInput,
- Writer appOutput) throws IOException, InterruptedException {
- return runAppProcess(
- newAppProcess(cmdarray),
- logger,
- appInput,
- appOutput);
- }
-
- /**
- * Constructs a {@link ProcessBuilder} to run an external application.
- *
- * @param cmdarray command and arguments.
- * @return a ProcessBuilder.
- */
- @Deprecated // to be removed before 2.0
- public static ProcessBuilder newAppProcess(String[] cmdarray) {
- // Concatenate quoted words from cmdarray.
- // REVIEW mb 2/24/09 Why is this needed?
- StringBuilder buf = new StringBuilder();
- for (int i = 0; i < cmdarray.length; ++i) {
- if (i > 0) {
- buf.append(" ");
- }
- buf.append('"');
- buf.append(cmdarray[i]);
- buf.append('"');
- }
- String fullcmd = buf.toString();
- buf.setLength(0);
- return new ProcessBuilder(cmdarray);
- }
-
-
- /**
- * Runs an external application process.
- *
- * @param pb ProcessBuilder for the application; might be
- * returned by {@link #newAppProcess}.
- * @param logger if not null, command and exit status will be logged here
- * @param appInput if not null, data will be copied to application's stdin
- * @param appOutput if not null, data will be captured from application's
- * stdout and stderr
- * @return application process exit value
- * @throws IOException
- * @throws InterruptedException
- */
- public static int runAppProcess(
- ProcessBuilder pb,
- Logger logger,
- Reader appInput,
- Writer appOutput) throws IOException, InterruptedException {
- pb.redirectErrorStream(true);
- if (logger != null) {
- logger.info("start process: " + pb.command());
- }
- Process p = pb.start();
-
- // Setup the input/output streams to the subprocess.
- // The buffering here is arbitrary. Javadocs strongly encourage
- // buffering, but the size needed is very dependent on the
- // specific application being run, the size of the input
- // provided by the caller, and the amount of output expected.
- // Since this method is currently used only by unit tests,
- // large-ish fixed buffer sizes have been chosen. If this
- // method becomes used for something in production, it might
- // be better to have the caller provide them as arguments.
- if (appInput != null) {
- OutputStream out =
- new BufferedOutputStream(
- p.getOutputStream(),
- 100 * 1024);
- int c;
- while ((c = appInput.read()) != -1) {
- out.write(c);
- }
- out.flush();
- }
- if (appOutput != null) {
- InputStream in =
- new BufferedInputStream(
- p.getInputStream(),
- 100 * 1024);
- int c;
- while ((c = in.read()) != -1) {
- appOutput.write(c);
- }
- appOutput.flush();
- in.close();
- }
- p.waitFor();
-
- int status = p.exitValue();
- if (logger != null) {
- logger.info("exit status=" + status + " from " + pb.command());
- }
- return status;
- }
-
- /**
* Converts a list whose members are automatically down-cast to a given
* type.
*
http://git-wip-us.apache.org/repos/asf/calcite/blob/d393cab2/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java
----------------------------------------------------------------------
diff --git a/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java b/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java
index ab3f7b1..67b790a 100644
--- a/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java
+++ b/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java
@@ -20,13 +20,21 @@ import org.apache.calcite.jdbc.SqlTimeoutException;
import org.apache.calcite.util.Unsafe;
import org.apache.calcite.util.Util;
+import org.slf4j.Logger;
+
+import java.io.BufferedInputStream;
+import java.io.BufferedOutputStream;
import java.io.BufferedReader;
import java.io.BufferedWriter;
import java.io.File;
import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
import java.io.PrintWriter;
+import java.io.Reader;
import java.io.StringReader;
import java.io.StringWriter;
+import java.io.Writer;
import java.lang.reflect.Constructor;
import java.sql.Connection;
import java.sql.DriverManager;
@@ -275,6 +283,68 @@ public class ConcurrentTestCommandScript
}
/**
+ * Runs an external application process.
+ *
+ * @param pb ProcessBuilder for the application
+ * @param logger if not null, command and exit status will be logged here
+ * @param appInput if not null, data will be copied to application's stdin
+ * @param appOutput if not null, data will be captured from application's
+ * stdout and stderr
+ * @return application process exit value
+ */
+ static int runAppProcess(
+ ProcessBuilder pb,
+ Logger logger,
+ Reader appInput,
+ Writer appOutput) throws IOException, InterruptedException {
+ pb.redirectErrorStream(true);
+ if (logger != null) {
+ logger.info("start process: " + pb.command());
+ }
+ Process p = pb.start();
+
+ // Setup the input/output streams to the subprocess.
+ // The buffering here is arbitrary. Javadocs strongly encourage
+ // buffering, but the size needed is very dependent on the
+ // specific application being run, the size of the input
+ // provided by the caller, and the amount of output expected.
+ // Since this method is currently used only by unit tests,
+ // large-ish fixed buffer sizes have been chosen. If this
+ // method becomes used for something in production, it might
+ // be better to have the caller provide them as arguments.
+ if (appInput != null) {
+ OutputStream out =
+ new BufferedOutputStream(
+ p.getOutputStream(),
+ 100 * 1024);
+ int c;
+ while ((c = appInput.read()) != -1) {
+ out.write(c);
+ }
+ out.flush();
+ }
+ if (appOutput != null) {
+ InputStream in =
+ new BufferedInputStream(
+ p.getInputStream(),
+ 100 * 1024);
+ int c;
+ while ((c = in.read()) != -1) {
+ appOutput.write(c);
+ }
+ appOutput.flush();
+ in.close();
+ }
+ p.waitFor();
+
+ int status = p.exitValue();
+ if (logger != null) {
+ logger.info("exit status=" + status + " from " + pb.command());
+ }
+ return status;
+ }
+
+ /**
* Gets ready to execute: loads script FILENAME applying external variable
* BINDINGS
*/
@@ -1497,13 +1567,15 @@ public class ConcurrentTestCommandScript
// argv[0] is found on $PATH. Working directory is the script's home
// directory.
+ //
+ // WARNING: ProcessBuilder is security-sensitive. Its use is currently
+ // safe because this code is under "core/test". Developers must not move
+ // this code into "core/main".
ProcessBuilder pb = new ProcessBuilder(argv);
pb.directory(scriptDirectory);
try {
// direct stdout & stderr to the the threadWriter
- int status =
- Util.runAppProcess(
- pb, null, null, getThreadWriter(threadId));
+ int status = runAppProcess(pb, null, null, getThreadWriter(threadId));
if (status != 0) {
storeMessage(threadId,
"command " + command + ": exited with status " + status);
http://git-wip-us.apache.org/repos/asf/calcite/blob/d393cab2/pom.xml
----------------------------------------------------------------------
diff --git a/pom.xml b/pom.xml
index d2e9a3a..7fc3e9e 100644
--- a/pom.xml
+++ b/pom.xml
@@ -491,6 +491,8 @@ limitations under the License.
<exclude>**/*TokenManager.class</exclude>
<exclude>**/TokenMgrError.class</exclude>
<exclude>**/org/apache/calcite/runtime/Resources$Inst.class</exclude>
+ <exclude>**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.class</exclude>
+ <exclude>**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript$ShellCommand.class</exclude>
<exclude>**/org/apache/calcite/util/Unsafe.class</exclude>
</excludes>
</configuration>
http://git-wip-us.apache.org/repos/asf/calcite/blob/d393cab2/src/main/config/forbidden-apis/signatures.txt
----------------------------------------------------------------------
diff --git a/src/main/config/forbidden-apis/signatures.txt b/src/main/config/forbidden-apis/signatures.txt
index a0eb429..4c4a8c5 100644
--- a/src/main/config/forbidden-apis/signatures.txt
+++ b/src/main/config/forbidden-apis/signatures.txt
@@ -39,4 +39,13 @@ java.lang.Runtime#exit(int)
@defaultMessage Don't interrupt threads use FutureUtils#cancel(Future<T>) instead
java.util.concurrent.Future#cancel(boolean)
+@defaultMessage Spawning processes is a potential security issue
+java.lang.ProcessBuilder
+java.lang.Runtime#exec(java.lang.String)
+java.lang.Runtime#exec(java.lang.String[])
+java.lang.Runtime#exec(java.lang.String, java.lang.String[])
+java.lang.Runtime#exec(java.lang.String, java.lang.String[], java.io.File)
+java.lang.Runtime#exec(java.lang.String[], java.lang.String[])
+java.lang.Runtime#exec(java.lang.String[], java.lang.String[], java.io.File)
+
# End signatures.txt