Skip to content

Commit a574ff4

Browse files
Marcono1234Marcono1234
committed
Add Process-output-not-consumed.ql
1 parent 22edafd commit a574ff4

File tree

2 files changed

+121
-2
lines changed

2 files changed

+121
-2
lines changed

‎codeql-custom-queries-java/not-tested-queries/using-static-array-as-template.ql

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
/**
22
* Finds static arrays which appear to be used as template for an argument, e.g.:
3-
* ```
3+
* ```java
44
* class EchoPrinter {
5-
* static final String[] COMMAND = {"echo", NULL};
5+
* static final String[] COMMAND = {"echo", null};
66
*
77
* public static void echo(String text) {
88
* // Modifies static array without synchronization; concurrent use would

‎codeql-custom-queries-java/queries/error-prone/Process-output-not-consumed.ql

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,119 @@
1+
/**
2+
* Finds creation of a `Process` whose std-out and std-err output is not consumed.
3+
* Operating systems often only have a limited output buffer for these outputs, so when the
4+
* output is not consumed, these buffers become full and the process could hang because it
5+
* cannot write any further output. This can even lead to a deadlock when `Process.waitFor`
6+
* is used, because it waits for the process to finish and the process waits for the Java
7+
* program to consume the output.
8+
*
9+
* The proper way to consume the output is by:
10+
* - Using `ProcessBuilder.inheritIO()`
11+
* - Or using both `ProcessBuilder.redirectOutput` and `ProcessBuilder.redirectError` with something
12+
* other than `Redirect.PIPE`
13+
* - Or reading from the `Process` `errorReader()` / `getErrorStream()` and `inputReader()` / `getInputStream()`\
14+
* Important:
15+
* - Both std-out _and_ std-err must be consumed
16+
* - Output processing must start asynchronously _before_ `Process.waitFor` (or similar) is called
17+
* - std-out and std-err must be processed concurrently; consuming them in the same thread sequentially
18+
* could still lead to a deadlock since these streams normally only end when the process has terminated
19+
* - If a program is only interested in some part of the std-out and std-err output, it must still
20+
* continue reading it afterwards, even if it just discards the read data
21+
*
22+
* @id todo
23+
* @kind problem
24+
*/
25+
26+
import java
27+
import semmle.code.java.dataflow.DataFlow
28+
29+
// Note: This does not cover all cases where the Process output is not properly consumed, but it covers at least
30+
// the most obvious cases
31+
32+
/**
33+
* Gets an expression which creates a `Process` whose output is not redirected.
34+
*/
35+
Expr processWithoutRedirect() {
36+
// Call of `Runtime.exec`
37+
exists(Method execMethod |
38+
execMethod.getDeclaringType() instanceof TypeRuntime and
39+
execMethod.hasName("exec") and
40+
result.(MethodAccess).getMethod() = execMethod
41+
)
42+
or
43+
// Creation of `ProcessBuilder` followed by `ProcessBuilder.start()`
44+
exists(ClassInstanceExpr newProcessBuilder, MethodAccess startCall |
45+
newProcessBuilder.getConstructedType() instanceof TypeProcessBuilder and
46+
startCall.getMethod().hasStringSignature("start()") and
47+
// TODO Have to model value steps for ProcessBuilder methods to support chaining of builder calls?
48+
DataFlow::localExprFlow(newProcessBuilder, startCall.getQualifier()) and
49+
// And there is no usage of ProcessBuilder method which redirects streams
50+
not exists(MethodAccess redirectCall, Method redirectMethod |
51+
DataFlow::localExprFlow(newProcessBuilder, redirectCall.getQualifier()) and
52+
redirectCall.getMethod() = redirectMethod and
53+
(
54+
redirectMethod.hasName("inheritIO")
55+
or
56+
/* TODO: Maybe should consider whether both `redirectError` and `redirectOutput` are used? (and not just one of them) */
57+
// Don't consider `redirectErrorStream(boolean)`; it would still be necessary to read the combined output stream then
58+
redirectMethod.hasName(["redirectError", "redirectOutput"]) and
59+
redirectMethod.getNumberOfParameters() = 1 and
60+
// And not using `Redirect.PIPE`
61+
not exists(Field redirectPipe |
62+
redirectCall.getArgument(0).(FieldRead).getField() = redirectPipe and
63+
redirectPipe.getDeclaringType().hasQualifiedName("java.lang", "ProcessBuilder$Redirect") and
64+
redirectPipe.hasName("PIPE")
65+
)
66+
)
67+
) and
68+
result = startCall
69+
)
70+
}
71+
72+
Expr getAnEnclosingExpr(Expr e) {
73+
result = e.getParent*()
74+
or
75+
// Or anonymous class (or lambda) which contains expression
76+
result =
77+
getAnEnclosingExpr(e.getEnclosingCallable()
78+
.getDeclaringType()
79+
.(AnonymousClass)
80+
.getClassInstanceExpr())
81+
}
82+
83+
from Expr processCreation, MethodAccess waitForProcess
84+
where
85+
processCreation = processWithoutRedirect() and
86+
waitForProcess
87+
.getMethod()
88+
.hasName([
89+
"waitFor",
90+
// Also cover `onExit` assuming that caller might wait on it soon afterwards (before obtaining output streams)
91+
"onExit"
92+
]) and
93+
DataFlow::localExprFlow(processCreation, waitForProcess.getQualifier()) and
94+
// And no output stream method is called before waiting
95+
not exists(MethodAccess outputAccess |
96+
outputAccess
97+
.getMethod()
98+
.hasName(["errorReader", "getErrorStream", "inputReader", "getInputStream",])
99+
|
100+
// Output access before `waitFor` call
101+
DataFlow::localExprFlow(processCreation, outputAccess.getQualifier()) and
102+
DataFlow::localExprFlow(outputAccess.getQualifier(), waitForProcess.getQualifier())
103+
or
104+
// Or output access in anonymous class or lambda before `waitFor` call, by accessing Process stored in variable
105+
// (assuming that this anonymous class or lambda is used as concurrent task)
106+
exists(Variable var, Expr assignedValue, Expr outputAccessEnclosing |
107+
assignedValue = var.getAnAssignedValue() and
108+
// Make sure the order is correct (and the dataflow is for the same value): processCreation -> varAssign -> waitFor
109+
DataFlow::localExprFlow(processCreation, assignedValue) and
110+
DataFlow::localExprFlow(assignedValue, waitForProcess.getQualifier()) and
111+
outputAccess.getQualifier() = var.getAnAccess() and
112+
outputAccessEnclosing = getAnEnclosingExpr(outputAccess) and
113+
// Make sure the order is correct: processCreation -> outputAccess -> waitFor
114+
// Otherwise this could lead to false negatives when a variable is reused
115+
processCreation.getControlFlowNode().getASuccessor*() = outputAccessEnclosing and
116+
outputAccessEnclosing.getControlFlowNode().getASuccessor*() = waitForProcess
117+
)
118+
)
119+
select waitForProcess, "Waits for process without consuming its output"

0 commit comments

Comments
 (0)