Skip to content

Commit cfa9b6c

Browse files
author
patrick.allen.higgins
committed
Make Executor return ExecuteResult and fix deadlock in DefaultExecutor.
Executor previously returned a String of stdout only, which limited usefulness for many use cases, including the unit tests which needed to be able to check both stdout and stderr. Now both are returned to the client along with the exit status because that is often useful, too. The new method which has this behavior is named executeProgram(). The old method executeSystemCommand() has been unchanged for backward compatibility.
1 parent 5ca9bb8 commit cfa9b6c

4 files changed

Lines changed: 166 additions & 19 deletions

File tree

‎src/main/java/org/owasp/esapi/ExecuteResult.java‎

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* OWASP Enterprise Security API (ESAPI)
3+
*
4+
* This file is part of the Open Web Application Security Project (OWASP)
5+
* Enterprise Security API (ESAPI) project. For details, please see
6+
* <a href="http://www.owasp.org/index.php/ESAPI">http://www.owasp.org/index.php/ESAPI</a>.
7+
*
8+
* Copyright (c) 2010 - The OWASP Foundation
9+
*
10+
* The ESAPI is published by OWASP under the BSD license. You should read and accept the
11+
* LICENSE before you use, modify, and/or redistribute this software.
12+
*
13+
* @author Patrick Higgins
14+
* @created 2010
15+
*/
16+
package org.owasp.esapi;
17+
18+
/**
19+
* The ExecuteResult class encapsulates the pieces of data that can be returned
20+
* from a process executed by the Executor interface.
21+
*
22+
* This class is immutable for thread-safety.
23+
*
24+
* @author Patrick Higgins
25+
* @since Aug 25, 2010
26+
*/
27+
public class ExecuteResult {
28+
29+
private final int exitValue;
30+
private final String output;
31+
private final String errors;
32+
33+
/**
34+
* Constructs an ExecuteResult from the given values.
35+
*
36+
* @param exitValue
37+
* the code from java.lang.Process.exitValue()
38+
* @param output
39+
* the contents read from java.lang.Process.getInputStream()
40+
* @param errors
41+
* the contents read from java.lang.Process.getErrorStream()
42+
*/
43+
public ExecuteResult(int exitValue, String output, String errors) {
44+
this.exitValue = exitValue;
45+
this.output = output;
46+
this.errors = errors;
47+
}
48+
49+
/**
50+
* @return the code from java.lang.Process.exitValue()
51+
*/
52+
public int getExitValue() {
53+
return exitValue;
54+
}
55+
56+
/**
57+
* @return the contents read from java.lang.Process.getInputStream()
58+
*/
59+
public String getOutput() {
60+
return output;
61+
}
62+
63+
/**
64+
* @return the contents read from java.lang.Process.getErrorStream()
65+
*/
66+
public String getErrors() {
67+
return errors;
68+
}
69+
70+
}

‎src/main/java/org/owasp/esapi/Executor.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,8 @@ public interface Executor {
5151
* Implementations must change to the specified working
5252
* directory before invoking the command.
5353
*
54+
* @deprecated Replaced by executeProgram
55+
*
5456
* @param executable
5557
* the command to execute
5658
* @param params
@@ -67,4 +69,26 @@ public interface Executor {
6769
*/
6870
String executeSystemCommand(File executable, List params, File workdir, Codec codec) throws ExecutorException;
6971

72+
/**
73+
* Executes a system command after checking that the executable exists and
74+
* escaping all the parameters to ensure that injection is impossible.
75+
* Implementations must change to the specified working
76+
* directory before invoking the command.
77+
*
78+
* @param executable
79+
* the command to execute
80+
* @param params
81+
* the parameters of the command being executed
82+
* @param workdir
83+
* the working directory
84+
* @param codec
85+
* the codec to use to encode for the particular OS in use
86+
*
87+
* @return the results of the command being run
88+
*
89+
* @throws ExecutorException
90+
* the service exception
91+
*/
92+
ExecuteResult executeProgram(File executable, List params, File workdir, Codec codec) throws ExecutorException;
93+
7094
}

‎src/main/java/org/owasp/esapi/reference/DefaultExecutor.java‎

Lines changed: 63 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.List;
2424

2525
import org.owasp.esapi.ESAPI;
26+
import org.owasp.esapi.ExecuteResult;
2627
import org.owasp.esapi.Logger;
2728
import org.owasp.esapi.codecs.Codec;
2829
import org.owasp.esapi.errors.ExecutorException;
@@ -51,6 +52,15 @@ public class DefaultExecutor implements org.owasp.esapi.Executor {
5152
public DefaultExecutor() {
5253
}
5354

55+
/**
56+
* {@inheritDoc}
57+
*
58+
* <p>The reference implementation calls <tt>executeProgram( ... ).getOutput()</tt>.</p>
59+
*/
60+
public String executeSystemCommand(File executable, List params, File workdir, Codec codec) throws ExecutorException {
61+
return executeProgram(executable, params, workdir, codec).getOutput();
62+
}
63+
5464
/**
5565
* {@inheritDoc}
5666
*
@@ -63,7 +73,7 @@ public DefaultExecutor() {
6373
* the parameters. You MUST change this behavior if you are passing credit card numbers, TIN/SSN, or
6474
* health information through this reference implementation, such as to a credit card or HL7 gateway.</p>
6575
*/
66-
public String executeSystemCommand(File executable, List params, File workdir, Codec codec) throws ExecutorException {
76+
public ExecuteResult executeProgram(File executable, List params, File workdir, Codec codec) throws ExecutorException {
6777
try {
6878
logger.warning(Logger.SECURITY, true, "Initiating executable: " + executable + " " + params + " in " + workdir);
6979

@@ -89,7 +99,6 @@ public String executeSystemCommand(File executable, List params, File workdir, C
8999

90100
params.add(0, executable.getCanonicalPath());
91101
String[] command = (String[])params.toArray( new String[0] );
92-
Process process = Runtime.getRuntime().exec(command, new String[0], workdir);
93102

94103
// Future - this is how to implement this in Java 1.5+
95104
// ProcessBuilder pb = new ProcessBuilder(params);
@@ -100,16 +109,39 @@ public String executeSystemCommand(File executable, List params, File workdir, C
100109
// pb.redirectErrorStream(true);
101110
// Process process = pb.start();
102111

103-
String output = readStream( process.getInputStream() );
104-
String errors = readStream( process.getErrorStream() );
112+
final StringBuffer outputBuffer = new StringBuffer();
113+
final StringBuffer errorsBuffer = new StringBuffer();
114+
115+
final Process process = Runtime.getRuntime().exec(command, new String[0], workdir);
116+
try {
117+
ReadThread errorReader = new ReadThread(process.getErrorStream(), errorsBuffer);
118+
errorReader.start();
119+
readStream( process.getInputStream(), outputBuffer );
120+
errorReader.join();
121+
if (errorReader.exception != null) {
122+
throw errorReader.exception;
123+
}
124+
process.waitFor();
125+
} catch (Throwable e) {
126+
process.destroy();
127+
throw new ExecutorException("Execution failure", "Exception thrown during execution of system command: " + e.getMessage(), e);
128+
}
129+
130+
String output = outputBuffer.toString();
131+
String errors = errorsBuffer.toString();
132+
int exitValue = process.exitValue();
133+
105134
if ( errors != null && errors.length() > 0 ) {
106-
logger.warning( Logger.SECURITY, false, "Error during system command: " + errors );
135+
logger.warning( Logger.SECURITY, false, "Error during system command: " + errors );
136+
}
137+
if ( exitValue != 0 ) {
138+
logger.warning( Logger.SECURITY, false, "System command exited with non-zero status: " + exitValue );
107139
}
108140
logger.warning(Logger.SECURITY, true, "System command complete: " + params);
109-
return output;
110-
} catch (Exception e) {
141+
return new ExecuteResult(exitValue, output, errors);
142+
} catch (IOException e) {
111143
throw new ExecutorException("Execution failure", "Exception thrown during execution of system command: " + e.getMessage(), e);
112-
}
144+
}
113145
}
114146

115147
/**
@@ -121,15 +153,33 @@ public String executeSystemCommand(File executable, List params, File workdir, C
121153
* a string containing as many lines as the input stream contains, with newlines between lines
122154
* @throws IOException
123155
*/
124-
private String readStream( InputStream is ) throws IOException {
156+
private static void readStream( InputStream is, StringBuffer sb ) throws IOException {
125157
InputStreamReader isr = new InputStreamReader(is);
126158
BufferedReader br = new BufferedReader(isr);
127-
StringBuffer sb = new StringBuffer();
128159
String line;
129160
while ((line = br.readLine()) != null) {
130-
sb.append(line + "\n");
161+
sb.append(line).append("\n");
131162
}
132-
return sb.toString();
133163
}
134-
164+
165+
private static class ReadThread extends Thread {
166+
volatile IOException exception;
167+
private final InputStream stream;
168+
private final StringBuffer buffer;
169+
170+
ReadThread(InputStream stream, StringBuffer buffer) {
171+
this.stream = stream;
172+
this.buffer = buffer;
173+
}
174+
175+
public void run() {
176+
try {
177+
readStream(stream, buffer);
178+
} catch (IOException e) {
179+
exception = e;
180+
}
181+
}
182+
183+
}
184+
135185
}

‎src/test/java/org/owasp/esapi/reference/ExecutorTest.java‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,13 @@
2626
import junit.framework.TestSuite;
2727

2828
import org.owasp.esapi.ESAPI;
29+
import org.owasp.esapi.ExecuteResult;
2930
import org.owasp.esapi.Executor;
3031
import org.owasp.esapi.SecurityConfiguration;
3132
import org.owasp.esapi.SecurityConfigurationWrapper;
3233
import org.owasp.esapi.codecs.Codec;
33-
import org.owasp.esapi.codecs.WindowsCodec;
3434
import org.owasp.esapi.codecs.UnixCodec;
35+
import org.owasp.esapi.codecs.WindowsCodec;
3536
import org.owasp.esapi.util.FileTestUtils;
3637

3738

@@ -77,8 +78,8 @@ public class ExecutorTest extends TestCase
7778
*/
7879
private static class Conf extends SecurityConfigurationWrapper
7980
{
80-
private List allowedExes;
81-
private File workingDir;
81+
private final List allowedExes;
82+
private final File workingDir;
8283

8384
/**
8485
* Create wrapper with the specified allowed execs and
@@ -164,15 +165,17 @@ public static Test suite() {
164165
public void testExecuteJava() throws Exception
165166
{
166167
List params = new ArrayList();
167-
String result;
168+
ExecuteResult result;
168169

169170
// -version goes to stderr which executeSystemCommand doesn't read...
170171
// -help goes to stdout so we'll use that...
171172
params.add("-help");
172173

173-
result = instance.executeSystemCommand(JAVA_CMD, params, TMP_DIR, codec);
174+
result = instance.executeProgram(JAVA_CMD, params, TMP_DIR, codec);
174175
assertNotNull("result of java -version was null", result);
175-
assertTrue("result of java -help did not contain -version", result.indexOf("-version") >= 0);
176+
assertTrue("result of java -help did not contain -version",
177+
result.getOutput().indexOf("-version") >= 0 ||
178+
result.getErrors().indexOf("-version") >= 0);
176179
}
177180

178181
public void testExecuteJavaSemicolenInject() throws Exception

0 commit comments

Comments
 (0)