Skip to content

Commit 3a2ae77

Browse files
DidierBessetbenzonico
DidierBesset
authored and
benzonico
committed
SONARJAVA-1416 - Generalization of unclosed opened resources
1 parent 3d6feed commit 3a2ae77

6 files changed

Lines changed: 259 additions & 31 deletions

File tree

‎its/ruling/src/test/resources/sonar-server/squid-S2095.json‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
{
2+
'org.codehaus.sonar:sonar-server:src/main/java/org/sonar/server/platform/BackendCleanup.java':[
3+
67,
4+
114,
5+
142,
6+
152,
7+
],
8+
}

‎java-squid/src/main/java/org/sonar/java/se/ExplodedGraphWalker.java‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -523,13 +523,15 @@ private void executeIdentifier(IdentifierTree tree) {
523523
}
524524

525525
private void executeMemberSelect(MemberSelectExpressionTree mse) {
526-
if (!"class".equals(mse.identifier().name())) {
526+
if (!mse.parent().is(Tree.Kind.METHOD_INVOCATION)) {
527+
if (!"class".equals(mse.identifier().name())) {
527528

528-
ProgramState.Pop unstackMSE = programState.unstackValue(1);
529-
programState = unstackMSE.state;
529+
ProgramState.Pop unstackMSE = programState.unstackValue(1);
530+
programState = unstackMSE.state;
531+
}
532+
SymbolicValue mseValue = constraintManager.createSymbolicValue(mse);
533+
programState = programState.stackValue(mseValue);
530534
}
531-
SymbolicValue mseValue = constraintManager.createSymbolicValue(mse);
532-
programState = programState.stackValue(mseValue);
533535
}
534536

535537
public void clearStack(Tree tree) {

‎java-squid/src/main/java/org/sonar/java/se/ProgramState.java‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929

3030
import javax.annotation.CheckForNull;
3131
import javax.annotation.Nullable;
32+
3233
import java.util.ArrayList;
3334
import java.util.Collections;
3435
import java.util.Deque;
@@ -42,11 +43,22 @@
4243

4344
public class ProgramState {
4445

46+
public static class ConstrainedValue {
47+
48+
public final SymbolicValue value;
49+
public final ObjectConstraint constraint;
50+
51+
ConstrainedValue(SymbolicValue value, ObjectConstraint constraint) {
52+
this.value = value;
53+
this.constraint = constraint;
54+
}
55+
}
56+
4557
public static class Pop {
4658

4759
public final ProgramState state;
48-
4960
public final List<SymbolicValue> values;
61+
5062
public Pop(ProgramState programState, List<SymbolicValue> result) {
5163
state = programState;
5264
values = result;
@@ -329,6 +341,38 @@ public SymbolicValue getValue(Symbol symbol) {
329341
return values.get(symbol);
330342
}
331343

344+
public List<ObjectConstraint> getConstraints(final Object state) {
345+
final List<ObjectConstraint> result = new ArrayList<>();
346+
constraints.forEach(new PMap.Consumer<SymbolicValue, Object>() {
347+
@Override
348+
public void accept(SymbolicValue value, Object valueConstraint) {
349+
if (valueConstraint instanceof ObjectConstraint) {
350+
ObjectConstraint constraint = (ObjectConstraint) valueConstraint;
351+
if (constraint.hasStatus(state)) {
352+
result.add(constraint);
353+
}
354+
}
355+
}
356+
});
357+
return result;
358+
}
359+
360+
public List<ConstrainedValue> getValuesWithConstraints(final Object state) {
361+
final List<ConstrainedValue> result = new ArrayList<>();
362+
constraints.forEach(new PMap.Consumer<SymbolicValue, Object>() {
363+
@Override
364+
public void accept(SymbolicValue value, Object valueConstraint) {
365+
if (valueConstraint instanceof ObjectConstraint) {
366+
ObjectConstraint constraint = (ObjectConstraint) valueConstraint;
367+
if (constraint.hasStatus(state)) {
368+
result.add(new ConstrainedValue(value, constraint));
369+
}
370+
}
371+
}
372+
});
373+
return result;
374+
}
375+
332376
public List<ObjectConstraint> getFieldConstraints(final Object state) {
333377
final Set<SymbolicValue> valuesAssignedToFields = getFieldValues();
334378
final List<ObjectConstraint> result = new ArrayList<>();

‎java-squid/src/main/java/org/sonar/java/se/checks/UnclosedResourcesCheck.java‎

Lines changed: 110 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import org.sonar.java.se.ConstraintManager;
2929
import org.sonar.java.se.ObjectConstraint;
3030
import org.sonar.java.se.ProgramState;
31+
import org.sonar.java.se.ProgramState.ConstrainedValue;
3132
import org.sonar.java.se.SymbolicValue;
3233
import org.sonar.java.se.SymbolicValueFactory;
3334
import org.sonar.plugins.java.api.JavaFileScanner;
@@ -43,10 +44,14 @@
4344
import org.sonar.plugins.java.api.tree.NewClassTree;
4445
import org.sonar.plugins.java.api.tree.ReturnStatementTree;
4546
import org.sonar.plugins.java.api.tree.Tree;
47+
import org.sonar.plugins.java.api.tree.TryStatementTree;
48+
import org.sonar.plugins.java.api.tree.VariableTree;
4649
import org.sonar.squidbridge.annotations.ActivatedByDefault;
4750
import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
4851
import org.sonar.squidbridge.annotations.SqaleSubCharacteristic;
4952

53+
import javax.annotation.Nullable;
54+
5055
import java.util.Iterator;
5156
import java.util.List;
5257
import java.util.Map;
@@ -61,20 +66,23 @@
6166
@SqaleConstantRemediation("5min")
6267
public class UnclosedResourcesCheck extends SECheck implements JavaFileScanner {
6368

64-
private enum States {
69+
private enum Status {
6570
OPENED, CLOSED;
6671
}
6772

73+
private static final String JAVA_IO_AUTO_CLOSEABLE = "java.lang.AutoCloseable";
6874
private static final String JAVA_IO_CLOSEABLE = "java.io.Closeable";
6975
private static final String[] IGNORED_CLOSEABLE_SUBTYPES = {
7076
"java.io.ByteArrayOutputStream",
7177
"java.io.ByteArrayInputStream",
72-
"java.io.StringReader",
73-
"java.io.StringWriter",
7478
"java.io.CharArrayReader",
7579
"java.io.CharArrayWriter",
80+
"java.io.StringReader",
81+
"java.io.StringWriter",
7682
"com.sun.org.apache.xml.internal.security.utils.UnsyncByteArrayOutputStream"
7783
};
84+
private static final String JAVA_SQL_STATEMENT = "java.sql.Statement";
85+
private static final String[] JDBC_RESOURCE_CREATIONS = {"java.sql.Connection", JAVA_SQL_STATEMENT};
7886

7987
static boolean needsClosing(Type type) {
8088
for (String ignoredTypes : IGNORED_CLOSEABLE_SUBTYPES) {
@@ -86,7 +94,7 @@ static boolean needsClosing(Type type) {
8694
}
8795

8896
static boolean isCloseable(Type type) {
89-
return type.isSubtypeOf(JAVA_IO_CLOSEABLE);
97+
return type.isSubtypeOf(JAVA_IO_AUTO_CLOSEABLE) || type.isSubtypeOf(JAVA_IO_CLOSEABLE);
9098
}
9199

92100
static boolean isOpeningResource(NewClassTree syntaxNode) {
@@ -97,18 +105,33 @@ static boolean isOpeningResource(NewClassTree syntaxNode) {
97105
return needsClosing(type);
98106
}
99107

100-
static boolean isWithinTryHeader(NewClassTree syntaxNode) {
108+
static boolean isWithinTryHeader(Tree syntaxNode) {
101109
final Tree parent = syntaxNode.parent();
102-
if (parent != null) {
103-
final Tree grandParent = parent.parent();
104-
if (grandParent != null) {
105-
final Tree greatGrandParent = grandParent.parent();
106-
return greatGrandParent != null && greatGrandParent.is(Tree.Kind.TRY_STATEMENT);
107-
}
110+
if (parent.is(Tree.Kind.VARIABLE)) {
111+
return isTryStatementResource((VariableTree) parent);
108112
}
109113
return false;
110114
}
111115

116+
static boolean isTryStatementResource(VariableTree variable) {
117+
final TryStatementTree tryStatement = getEnclosingTryStatement(variable);
118+
if (tryStatement != null) {
119+
return tryStatement.resources().contains(variable);
120+
}
121+
return false;
122+
}
123+
124+
private static TryStatementTree getEnclosingTryStatement(Tree syntaxNode) {
125+
Tree parent = syntaxNode.parent();
126+
while (parent != null) {
127+
if (parent.is(Tree.Kind.TRY_STATEMENT)) {
128+
return (TryStatementTree) parent;
129+
}
130+
parent = parent.parent();
131+
}
132+
return null;
133+
}
134+
112135
private static class PostStatementVisitor extends CheckerTreeNodeVisitor {
113136

114137
public PostStatementVisitor(CheckerContext context) {
@@ -120,10 +143,30 @@ public void visitNewClass(NewClassTree syntaxNode) {
120143
if (isOpeningResource(syntaxNode)) {
121144
final SymbolicValue instanceValue = programState.peekValue();
122145
if (!(instanceValue instanceof ResourceWrapperSymbolicValue)) {
123-
programState = programState.addConstraint(instanceValue, new ObjectConstraint(false, false, syntaxNode, States.OPENED));
146+
programState = programState.addConstraint(instanceValue, new ObjectConstraint(false, false, syntaxNode, Status.OPENED));
124147
}
125148
}
126149
}
150+
151+
@Override
152+
public void visitMethodInvocation(MethodInvocationTree syntaxNode) {
153+
if (syntaxNode.methodSelect().is(Tree.Kind.MEMBER_SELECT) && needsClosing(syntaxNode.symbolType())) {
154+
final ExpressionTree targetExpression = ((MemberSelectExpressionTree) syntaxNode.methodSelect()).expression();
155+
if (targetExpression.is(Tree.Kind.IDENTIFIER) && !isWithinTryHeader(syntaxNode)
156+
&& (syntaxNode.symbol().isStatic() || isJdbcResourceCreation(targetExpression))) {
157+
programState = programState.addConstraint(programState.peekValue(), new ObjectConstraint(false, false, syntaxNode, Status.OPENED));
158+
}
159+
}
160+
}
161+
162+
private static boolean isJdbcResourceCreation(ExpressionTree targetExpression) {
163+
for (String creator : JDBC_RESOURCE_CREATIONS) {
164+
if (targetExpression.symbolType().is(creator)) {
165+
return true;
166+
}
167+
}
168+
return false;
169+
}
127170
}
128171

129172
public static class ResourceWrapperSymbolicValue extends SymbolicValue {
@@ -135,11 +178,6 @@ public ResourceWrapperSymbolicValue(int id, SymbolicValue dependent) {
135178
this.dependent = dependent;
136179
}
137180

138-
@Override
139-
public boolean references(SymbolicValue other) {
140-
return dependent.equals(other) || dependent.references(other);
141-
}
142-
143181
@Override
144182
public SymbolicValue wrappedValue() {
145183
return dependent.wrappedValue();
@@ -163,6 +201,8 @@ public SymbolicValue createSymbolicValue(int counter, Tree syntaxNode) {
163201
static class PreStatementVisitor extends CheckerTreeNodeVisitor {
164202

165203
private static final String CLOSE_METHOD_NAME = "close";
204+
private static final String GET_MORE_RESULTS_METHOD_NAME = "getMoreResults";
205+
private static final String GET_RESULT_SET = "getResultSet";
166206

167207
private final ConstraintManager constraintManager;
168208

@@ -221,23 +261,54 @@ public void visitAssignmentExpression(AssignmentExpressionTree syntaxNode) {
221261

222262
@Override
223263
public void visitMethodInvocation(MethodInvocationTree syntaxNode) {
264+
final ExpressionTree methodSelect = syntaxNode.methodSelect();
224265
if (isClosingResource(syntaxNode.symbol())) {
225-
ExpressionTree methodSelect = syntaxNode.methodSelect();
226266
if (methodSelect.is(Tree.Kind.MEMBER_SELECT)) {
227267
final ExpressionTree targetExpression = ((MemberSelectExpressionTree) methodSelect).expression();
228268
if (targetExpression.is(Tree.Kind.IDENTIFIER)) {
229269
final IdentifierTree identifier = (IdentifierTree) targetExpression;
230-
final SymbolicValue target = programState.getValue(identifier.symbol());
231-
programState = closeResource(programState, target);
270+
programState = closeResource(programState, programState.getValue(identifier.symbol()));
232271
} else {
233272
programState = closeResource(programState, programState.peekValue());
234273
}
235274
}
275+
} else if (syntaxNode.methodSelect().is(Tree.Kind.MEMBER_SELECT) && isOpeningResultSet(syntaxNode.symbol())) {
276+
final SymbolicValue value = getTargetValue(syntaxNode);
277+
constraintManager.setValueFactory(new WrappedValueFactory(value));
278+
} else if (isClosingResultSets(syntaxNode.symbol())) {
279+
if (methodSelect.is(Tree.Kind.MEMBER_SELECT)) {
280+
final SymbolicValue value = getTargetValue(syntaxNode);
281+
closeResultSetsRelatedTo(value);
282+
}
236283
} else {
237284
closeArguments(syntaxNode.arguments(), 1);
238285
}
239286
}
240287

288+
private SymbolicValue getTargetValue(MethodInvocationTree syntaxNode) {
289+
final ExpressionTree targetExpression = ((MemberSelectExpressionTree) syntaxNode.methodSelect()).expression();
290+
final SymbolicValue value;
291+
if (targetExpression.is(Tree.Kind.IDENTIFIER)) {
292+
final IdentifierTree identifier = (IdentifierTree) targetExpression;
293+
value = programState.getValue(identifier.symbol());
294+
} else {
295+
value = programState.peekValue();
296+
}
297+
return value;
298+
}
299+
300+
private void closeResultSetsRelatedTo(SymbolicValue value) {
301+
final List<ConstrainedValue> constrainedValues = programState.getValuesWithConstraints(Status.OPENED);
302+
for (ConstrainedValue constrainedValue : constrainedValues) {
303+
if (constrainedValue.value instanceof ResourceWrapperSymbolicValue) {
304+
ResourceWrapperSymbolicValue rValue = (ResourceWrapperSymbolicValue) constrainedValue.value;
305+
if (value.equals(rValue.dependent)) {
306+
programState = programState.addConstraint(rValue, constrainedValue.constraint.withStatus(Status.CLOSED));
307+
}
308+
}
309+
}
310+
}
311+
241312
private void closeArguments(final Arguments arguments, int stackOffset) {
242313
final List<SymbolicValue> values = programState.peekValues(arguments.size() + stackOffset);
243314
final List<SymbolicValue> argumentValues = values.subList(stackOffset, values.size());
@@ -246,11 +317,11 @@ private void closeArguments(final Arguments arguments, int stackOffset) {
246317
}
247318
}
248319

249-
private static ProgramState closeResource(ProgramState programState, final SymbolicValue target) {
320+
private static ProgramState closeResource(ProgramState programState, @Nullable final SymbolicValue target) {
250321
if (target != null) {
251322
ObjectConstraint oConstraint = openedConstraint(programState, target);
252323
if (oConstraint != null) {
253-
return programState.addConstraint(target.wrappedValue(), oConstraint.withStatus(States.CLOSED));
324+
return programState.addConstraint(target.wrappedValue(), oConstraint.withStatus(Status.CLOSED));
254325
}
255326
}
256327
return programState;
@@ -260,7 +331,7 @@ private static ObjectConstraint openedConstraint(ProgramState programState, Symb
260331
final Object constraint = programState.getConstraint(value.wrappedValue());
261332
if (constraint instanceof ObjectConstraint) {
262333
ObjectConstraint oConstraint = (ObjectConstraint) constraint;
263-
if (oConstraint.hasStatus(States.OPENED)) {
334+
if (oConstraint.hasStatus(Status.OPENED)) {
264335
return oConstraint;
265336
}
266337
}
@@ -270,6 +341,14 @@ private static ObjectConstraint openedConstraint(ProgramState programState, Symb
270341
private static boolean isClosingResource(Symbol methodInvocationSymbol) {
271342
return methodInvocationSymbol.isMethodSymbol() && CLOSE_METHOD_NAME.equals(methodInvocationSymbol.name());
272343
}
344+
345+
private static boolean isClosingResultSets(Symbol methodInvocationSymbol) {
346+
return methodInvocationSymbol.isMethodSymbol() && GET_MORE_RESULTS_METHOD_NAME.equals(methodInvocationSymbol.name());
347+
}
348+
349+
private static boolean isOpeningResultSet(Symbol methodInvocationSymbol) {
350+
return methodInvocationSymbol.isMethodSymbol() && GET_RESULT_SET.equals(methodInvocationSymbol.name());
351+
}
273352
}
274353

275354
@Override
@@ -296,12 +375,18 @@ public ProgramState checkPostStatement(CheckerContext context, Tree syntaxNode)
296375

297376
@Override
298377
public void checkEndOfExecutionPath(CheckerContext context, ConstraintManager constraintManager) {
299-
final List<ObjectConstraint> constraints = context.getState().getFieldConstraints(States.OPENED);
378+
final List<ObjectConstraint> constraints = context.getState().getFieldConstraints(Status.OPENED);
300379
for (ObjectConstraint constraint : constraints) {
301380
Tree syntaxNode = constraint.syntaxNode();
302-
if (syntaxNode instanceof NewClassTree) {
381+
if (syntaxNode.is(Tree.Kind.NEW_CLASS)) {
303382
context.reportIssue(syntaxNode, this, "Close this \"" + ((NewClassTree) syntaxNode).identifier() + "\".");
383+
} else if (syntaxNode.is(Tree.Kind.METHOD_INVOCATION)) {
384+
context.reportIssue(syntaxNode, this, "Close this \"" + toString((MethodInvocationTree) syntaxNode) + "\".");
304385
}
305386
}
306387
}
388+
389+
private static String toString(MethodInvocationTree syntaxNode) {
390+
return syntaxNode.symbolType().name();
391+
}
307392
}

0 commit comments

Comments
 (0)