From 4bb37593714fa2c0eb7bd305b3b828837b8870b8 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Fri, 24 Jul 2020 17:05:04 +0300 Subject: [PATCH 1/8] [java] CloseResource false positive when resource included in return value --- .../rule/errorprone/CloseResourceRule.java | 679 +++++++++--------- .../rule/errorprone/xml/CloseResource.xml | 21 + 2 files changed, 376 insertions(+), 324 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index ac3bb0bd53..87453c06a1 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -7,7 +7,6 @@ package net.sourceforge.pmd.lang.java.rule.errorprone; import static net.sourceforge.pmd.properties.PropertyFactory.booleanProperty; import static net.sourceforge.pmd.properties.PropertyFactory.stringListProperty; -import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -26,29 +25,25 @@ import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTExpression; -import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters; import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; +import net.sourceforge.pmd.lang.java.ast.ASTLiteral; import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTName; +import net.sourceforge.pmd.lang.java.ast.ASTNullLiteral; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryExpression; import net.sourceforge.pmd.lang.java.ast.ASTPrimaryPrefix; import net.sourceforge.pmd.lang.java.ast.ASTPrimarySuffix; -import net.sourceforge.pmd.lang.java.ast.ASTReferenceType; import net.sourceforge.pmd.lang.java.ast.ASTResourceSpecification; import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; -import net.sourceforge.pmd.lang.java.ast.ASTStatementExpression; import net.sourceforge.pmd.lang.java.ast.ASTTryStatement; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; -import net.sourceforge.pmd.lang.java.ast.ASTVariableInitializer; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; -import net.sourceforge.pmd.lang.java.symboltable.VariableNameDeclaration; import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper; -import net.sourceforge.pmd.lang.symboltable.NameOccurrence; import net.sourceforge.pmd.properties.PropertyDescriptor; /** @@ -69,10 +64,10 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; */ public class CloseResourceRule extends AbstractJavaRule { - private Set types = new HashSet<>(); - private Set simpleTypes = new HashSet<>(); + private final Set types = new HashSet<>(); + private final Set simpleTypes = new HashSet<>(); - private Set closeTargets = new HashSet<>(); + private final Set closeTargets = new HashSet<>(); private static final PropertyDescriptor> CLOSE_TARGETS_DESCRIPTOR = stringListProperty("closeTargets") .desc("Methods which may close this resource") @@ -114,7 +109,7 @@ public class CloseResourceRule extends AbstractJavaRule { if (getProperty(CLOSE_TARGETS_DESCRIPTOR) != null) { closeTargets.addAll(getProperty(CLOSE_TARGETS_DESCRIPTOR)); } - if (getProperty(USE_CLOSE_AS_DEFAULT_TARGET) && !closeTargets.contains("close")) { + if (getProperty(USE_CLOSE_AS_DEFAULT_TARGET)) { closeTargets.add("close"); } if (getProperty(TYPES_DESCRIPTOR) != null) { @@ -147,144 +142,112 @@ public class CloseResourceRule extends AbstractJavaRule { } private void checkForResources(ASTMethodOrConstructorDeclaration node, Object data) { - List localVars = node.findDescendantsOfType(ASTLocalVariableDeclaration.class); - List vars = new ArrayList<>(); - Map ids = new HashMap<>(); - - // find all variable declarators - for (ASTLocalVariableDeclaration localVar : localVars) { - vars.addAll(localVar.findChildrenOfType(ASTVariableDeclarator.class)); + Map resVars = getResourceVariables(node); + for (Map.Entry resVarEntry : resVars.entrySet()) { + TypeNode resVarType = resVarEntry.getValue(); + ASTVariableDeclarator resVar = resVarEntry.getKey(); + if (isNotAllowedResourceType(resVarType) && isNotWrappingResourceMethodParameter(resVar, node) + && isResourceVariableUnclosed(resVar)) { + addCloseResourceViolation(resVar.getVariableId(), resVarType, data); + } } + } - // find all variable references to Connection objects + private Map getResourceVariables(ASTMethodOrConstructorDeclaration method) { + List vars = method.findDescendantsOfType(ASTVariableDeclarator.class); + Map resVars = new HashMap<>(); for (ASTVariableDeclarator var : vars) { - // get the type of the local var declaration - TypeNode type = ((ASTLocalVariableDeclaration) var.getParent()).getTypeNode(); - - if (type != null && isResourceTypeOrSubtype(type)) { - if (var.hasInitializer()) { - // figure out the runtime type. If the variable is initialized, take the type from there - ASTExpression expression = var.getInitializer().getFirstChildOfType(ASTExpression.class); - TypeNode runtimeType = expression; - if (!isMethodCall(expression) && runtimeType != null && runtimeType.getType() != null) { - type = runtimeType; - } - - // consider cases, when the streams are chained - // assumes, that the underlaying stream is always the first argument in the - // constructor call. - ASTExpression firstArgument = getAllocationFirstArgument(expression); - if (firstArgument != null) { - type = firstArgument; - } - } - - if (!isAllowedResourceType(type) && !isMethodParameter(var, node)) { - ids.put(var.getVariableId(), type); - } + TypeNode varType = getTypeOfVariable(var); + if (isResourceTypeOrSubtype(varType)) { + resVars.put(var, wrappedResourceTypeOrReturn(var, varType)); } } - - // if there are closables, ensure each is closed. - for (Map.Entry entry : ids.entrySet()) { - ASTVariableDeclaratorId variableId = entry.getKey(); - ensureClosed((ASTLocalVariableDeclaration) variableId.getParent().getParent(), variableId, - entry.getValue(), data); - } + return resVars; } - /** - * Checks whether the variable is initialized from a method parameter. - * @param var the variable that is being initialized - * @param methodOrCstor the method or constructor in which the variable is declared - * @return true if the variable is initialized from a method parameter. false - * otherwise. - */ - private boolean isMethodParameter(ASTVariableDeclarator var, ASTMethodOrConstructorDeclaration methodOrCstor) { - if (!var.hasInitializer()) { - return false; + private TypeNode getTypeOfVariable(ASTVariableDeclarator var) { + TypeNode declaredVarType = getDeclaredTypeOfVariable(var); + if (var.hasInitializer()) { + TypeNode runtimeVarType = getRuntimeTypeOfVariable(var); + return runtimeVarType != null ? runtimeVarType : declaredVarType; } - - boolean result = false; - ASTVariableInitializer initializer = var.getInitializer(); - ASTName name = initializer.getFirstDescendantOfType(ASTName.class); - if (name != null) { - ASTFormalParameters formalParameters = null; - if (methodOrCstor instanceof ASTMethodDeclaration) { - formalParameters = ((ASTMethodDeclaration) methodOrCstor).getFormalParameters(); - } else if (methodOrCstor instanceof ASTConstructorDeclaration) { - formalParameters = ((ASTConstructorDeclaration) methodOrCstor).getFormalParameters(); - } - if (formalParameters != null) { - List ids = formalParameters.findDescendantsOfType(ASTVariableDeclaratorId.class); - for (ASTVariableDeclaratorId id : ids) { - if (id.hasImageEqualTo(name.getImage()) && isResourceTypeOrSubtype(id)) { - result = true; - break; - } - } - } - } - return result; + return declaredVarType; } - private ASTExpression getAllocationFirstArgument(ASTExpression expression) { - List allocations = expression.findDescendantsOfType(ASTAllocationExpression.class); - ASTExpression firstArgument = null; + private TypeNode getDeclaredTypeOfVariable(ASTVariableDeclarator var) { + ASTLocalVariableDeclaration localVar = (ASTLocalVariableDeclaration) var.getParent(); + return localVar.getTypeNode(); + } - if (!allocations.isEmpty()) { - ASTArgumentList argumentList = allocations.get(allocations.size() - 1).getFirstDescendantOfType(ASTArgumentList.class); - if (argumentList != null) { - firstArgument = argumentList.getFirstChildOfType(ASTExpression.class); - } + private TypeNode getRuntimeTypeOfVariable(ASTVariableDeclarator var) { + ASTExpression initExpr = var.getInitializer().getFirstChildOfType(ASTExpression.class); + if (initExpr != null && isNotMethodCall(initExpr)) { + return initExpr.getType() != null ? initExpr : null; } + return null; + } - // the argument must not be a literal, it needs to be a Name referring to a variable - if (firstArgument != null && firstArgument.getFirstDescendantOfType(ASTName.class) != null) { - ASTName name = firstArgument.getFirstDescendantOfType(ASTName.class); + private boolean isNotMethodCall(ASTExpression expr) { + return !isMethodCall(expr); + } - Map> vars = firstArgument.getScope() - .getDeclarations(VariableNameDeclaration.class); - for (VariableNameDeclaration nameDecl : vars.keySet()) { - if (nameDecl.getName().equals(name.getImage()) && isResourceTypeOrSubtype(firstArgument)) { - return firstArgument; - } + private boolean isMethodCall(ASTExpression expression) { + return expression != null && expression.getNumChildren() > 0 + && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; + } + + private TypeNode wrappedResourceTypeOrReturn(ASTVariableDeclarator var, TypeNode defaultVal) { + if (var.hasInitializer()) { + TypeNode wrappedResType = getWrappedResourceType(var); + return wrappedResType != null ? wrappedResType : defaultVal; + } + return defaultVal; + } + + private TypeNode getWrappedResourceType(ASTVariableDeclarator var) { + ASTExpression initExpr = var.getInitializer().getFirstChildOfType(ASTExpression.class); + if (initExpr != null) { + ASTExpression wrappedType = getAllocationFirstArgumentVariable(initExpr); + return wrappedType != null && isResourceTypeOrSubtype(wrappedType) + ? wrappedType + : null; + } + return null; + } + + private ASTExpression getAllocationFirstArgumentVariable(ASTExpression expression) { + ASTAllocationExpression allocation = getLastAllocationExpression(expression); + if (allocation != null) { + ASTArgumentList argsList = allocation.getFirstDescendantOfType(ASTArgumentList.class); + if (argsList != null) { + ASTExpression firstArg = argsList.getFirstChildOfType(ASTExpression.class); + return isNotLiteral(firstArg) ? firstArg : null; } } return null; } - private boolean isMethodCall(ASTExpression expression) { - return expression != null - && expression.getNumChildren() > 0 - && expression.getChild(0) instanceof ASTPrimaryExpression - && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; + private ASTAllocationExpression getLastAllocationExpression(ASTExpression expression) { + List allocations = expression.findDescendantsOfType(ASTAllocationExpression.class); + if (!allocations.isEmpty()) { + int lastAllocationIndex = allocations.size() - 1; + return allocations.get(lastAllocationIndex); + } + return null; } - private boolean isResourceTypeOrSubtype(TypeNode refType) { - if (refType.getType() != null) { - for (String type : types) { - if (TypeHelper.isA(refType, type)) { - return true; - } - } - } else if (refType.getNumChildren() > 0 && refType.getChild(0) instanceof ASTReferenceType) { - // no type information (probably missing auxclasspath) - use simple types - ASTReferenceType ref = (ASTReferenceType) refType.getChild(0); - if (ref.getChild(0) instanceof ASTClassOrInterfaceType) { - ASTClassOrInterfaceType clazz = (ASTClassOrInterfaceType) ref.getChild(0); - if (simpleTypes.contains(toSimpleType(clazz.getImage())) && !clazz.isReferenceToClassSameCompilationUnit() - || types.contains(clazz.getImage()) && !clazz.isReferenceToClassSameCompilationUnit()) { - return true; - } - } - } - return false; + private boolean isNotLiteral(ASTExpression expression) { + ASTLiteral literal = expression.getFirstDescendantOfType(ASTLiteral.class); + return literal == null; + } + + private boolean isNotAllowedResourceType(TypeNode varType) { + return !isAllowedResourceType(varType); } private boolean isAllowedResourceType(TypeNode refType) { List allowedResourceTypes = getProperty(ALLOWED_RESOURCE_TYPES); - if (refType.getType() != null && allowedResourceTypes != null) { + if (allowedResourceTypes != null) { for (String type : allowedResourceTypes) { // the check here must be a exact type match, since subclasses may override close() // and actually require closing @@ -296,223 +259,192 @@ public class CloseResourceRule extends AbstractJavaRule { return false; } - private boolean hasNullInitializer(ASTLocalVariableDeclaration var) { - ASTVariableInitializer init = var.getFirstDescendantOfType(ASTVariableInitializer.class); - if (init != null) { - try { - List nulls = init - .findChildNodesWithXPath("Expression/PrimaryExpression/PrimaryPrefix/Literal/NullLiteral"); - return !nulls.isEmpty(); - } catch (JaxenException e) { - return false; + private boolean isNotWrappingResourceMethodParameter(ASTVariableDeclarator var, + ASTMethodOrConstructorDeclaration method) { + return !isWrappingResourceMethodParameter(var, method); + } + + /** + * Checks whether the variable is resource and initialized from a method parameter. + * @param var the resource variable that is being initialized + * @param method the method or constructor in which the variable is declared + * @return true if the variable is resource and initialized from a method parameter. false + * otherwise. + */ + private boolean isWrappingResourceMethodParameter(ASTVariableDeclarator var, ASTMethodOrConstructorDeclaration method) { + String wrappedVarName = getWrappedVariableName(var); + if (wrappedVarName != null) { + ASTFormalParameters methodParams = method.getFirstDescendantOfType(ASTFormalParameters.class); + if (methodParams != null) { + List ids = methodParams.findDescendantsOfType(ASTVariableDeclaratorId.class); + for (ASTVariableDeclaratorId id : ids) { + if (id.hasImageEqualTo(wrappedVarName) && isResourceTypeOrSubtype(id)) { + return true; + } + } } } return false; } - private void ensureClosed(ASTLocalVariableDeclaration var, ASTVariableDeclaratorId id, TypeNode type, Object data) { - // What are the chances of a Connection being instantiated in a - // for-loop init block? Anyway, I'm lazy! - String variableToClose = id.getImage(); - Node n = var; - - while (!(n instanceof ASTBlock) && !(n instanceof ASTConstructorDeclaration)) { - n = n.getParent(); - } - - Node top = n; - - List tryblocks = top.findDescendantsOfType(ASTTryStatement.class); - - boolean closed = false; - - ASTBlockStatement parentBlock = id.getFirstParentOfType(ASTBlockStatement.class); - - // look for try blocks below the line the variable was - // introduced and make sure there is a .close call in a finally - // block. - for (ASTTryStatement t : tryblocks) { - - // verifies that there are no critical statements between the - // variable declaration and - // the beginning of the try block. - ASTBlockStatement tryBlock = t.getFirstParentOfType(ASTBlockStatement.class); - // no need to check for critical statements, if - // the variable has been initialized with null - if (!hasNullInitializer(var) && parentBlock.getParent() == tryBlock.getParent()) { - - List blocks = parentBlock.getParent().findChildrenOfType(ASTBlockStatement.class); - int parentBlockIndex = blocks.indexOf(parentBlock); - int tryBlockIndex = blocks.indexOf(tryBlock); - boolean criticalStatements = false; - - for (int i = parentBlockIndex + 1; i < tryBlockIndex; i++) { - // assume variable declarations are not critical and assignments are not critical - ASTBlockStatement block = blocks.get(i); - ASTLocalVariableDeclaration varDecl = block - .getFirstDescendantOfType(ASTLocalVariableDeclaration.class); - ASTStatementExpression statementExpression = block.getFirstDescendantOfType(ASTStatementExpression.class); - - if (varDecl == null && (statementExpression == null - || statementExpression.getFirstChildOfType(ASTAssignmentOperator.class) == null)) { - criticalStatements = true; - break; - } - } - if (criticalStatements) { - break; - } - } - - ASTFinallyStatement finallyClause = t.getFinallyClause(); - if (t.getBeginLine() > id.getBeginLine() && finallyClause != null) { - ASTBlock finallyBody = finallyClause.getBody(); - List names = finallyBody.findDescendantsOfType(ASTName.class); - for (ASTName oName : names) { - String name = oName.getImage(); - if (name != null && name.contains(".")) { - String[] parts = name.split("\\."); - if (parts.length == 2) { - String methodName = parts[1]; - String varName = parts[0]; - if (varName.equals(variableToClose) && closeTargets.contains(methodName) - && nullCheckIfCondition(finallyBody, oName, varName)) { - closed = true; - break; - } - - } - } - } - if (closed) { - break; - } - - List exprs = finallyBody.findDescendantsOfType(ASTStatementExpression.class, true); - for (ASTStatementExpression stmt : exprs) { - ASTPrimaryExpression expr = stmt.getFirstChildOfType(ASTPrimaryExpression.class); - if (expr != null) { - ASTPrimaryPrefix prefix = expr.getFirstChildOfType(ASTPrimaryPrefix.class); - ASTPrimarySuffix suffix = expr.getFirstChildOfType(ASTPrimarySuffix.class); - if (prefix != null && suffix != null) { - if (prefix.getImage() == null) { - ASTName prefixName = prefix.getFirstChildOfType(ASTName.class); - if (prefixName != null && closeTargets.contains(prefixName.getImage())) { - // Found a call to a "close target" that is - // a direct - // method call without a "ClassName." - // prefix. - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - } else if (suffix.getImage() != null) { - String prefixPlusSuffix = prefix.getImage() + "." + suffix.getImage(); - if (closeTargets.contains(prefixPlusSuffix)) { - // Found a call to a "close target" that is - // a method call - // in the form "ClassName.methodName". - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - } - // look for primary suffix containing the close - // Targets elements. - // If the .close is executed in another class - // accessed by a method - // this form : - // getProviderInstance().closeConnexion(connexion) - // For this use case, we assume the variable is - // correctly closed - // in the other class since there is no way to - // really check it. - if (!closed) { - List suffixes = expr.findDescendantsOfType(ASTPrimarySuffix.class, true); - for (ASTPrimarySuffix oSuffix : suffixes) { - String suff = oSuffix.getImage(); - if (closeTargets.contains(suff)) { - closed = variableIsPassedToMethod(expr, variableToClose); - if (closed) { - break; - } - } - - } - } - } - } - } - if (closed) { - break; - } - } else if (t.isTryWithResources()) { - // maybe the variable is used as a resource - List names = t.getFirstChildOfType(ASTResourceSpecification.class).findDescendantsOfType(ASTName.class); - for (ASTName potentialUsage : names) { - if (potentialUsage.hasImageEqualTo(variableToClose)) { - closed = true; - break; - } - } - } - } - - if (!closed) { - // See if the variable is returned by the method, which means the - // method is a utility for creating the db resource, which means of - // course it can't be closed by the method, so it isn't an error. - List returns = top.findDescendantsOfType(ASTReturnStatement.class, true); - for (ASTReturnStatement returnStatement : returns) { - ASTName name = returnStatement.getFirstDescendantOfType(ASTName.class); - if (name != null && name.getImage().equals(variableToClose)) { - closed = true; - break; - } - } - } - - // if all is not well, complain - if (!closed) { - ASTLocalVariableDeclaration localVarDecl = id.getFirstParentOfType(ASTLocalVariableDeclaration.class); - Class typeClass = type.getType(); - if (typeClass != null) { - addViolation(data, id, typeClass.getSimpleName()); - } else if (localVarDecl != null && localVarDecl.getTypeNode() != null) { - addViolation(data, id, localVarDecl.getTypeNode().getTypeImage()); - } else { - addViolation(data, id, id.getVariableName()); - } + private String getWrappedVariableName(ASTVariableDeclarator var) { + if (var.hasInitializer()) { + ASTName varName = var.getInitializer().getFirstDescendantOfType(ASTName.class); + return varName != null ? varName.getImage() : null; } + return null; } - private boolean variableIsPassedToMethod(ASTPrimaryExpression expr, String variable) { - List methodParams = expr.findDescendantsOfType(ASTName.class, true); - for (ASTName pName : methodParams) { - String paramName = pName.getImage(); - // also check if we've got the a parameter (i.e if it's an argument - // !) - ASTArgumentList parentParam = pName.getFirstParentOfType(ASTArgumentList.class); - if (paramName.equals(variable) && parentParam != null) { + private boolean isResourceTypeOrSubtype(TypeNode refType) { + return refType.getType() != null + ? isNodeInstanceOfResourceType(refType) + : nodeHasReferenceToResourceType(refType); + } + + private boolean isNodeInstanceOfResourceType(TypeNode refType) { + for (String resType : types) { + if (TypeHelper.isA(refType, resType)) { return true; } } return false; } - private ASTIfStatement findIfStatement(ASTBlock enclosingBlock, Node node) { - ASTIfStatement ifStatement = node.getFirstParentOfType(ASTIfStatement.class); - List allIfStatements = enclosingBlock.findDescendantsOfType(ASTIfStatement.class); - if (ifStatement != null && allIfStatements.contains(ifStatement)) { - return ifStatement; + private boolean nodeHasReferenceToResourceType(TypeNode refType) { + ASTClassOrInterfaceType type = refType.getFirstDescendantOfType(ASTClassOrInterfaceType.class); + return type != null && isResourceTypeName(type.getImage()) && !type.isReferenceToClassSameCompilationUnit(); + } + + private boolean isResourceTypeName(String typeName) { + String simpleTypeName = toSimpleType(typeName); + return types.contains(typeName) || simpleTypes.contains(simpleTypeName); + } + + private boolean isResourceVariableUnclosed(ASTVariableDeclarator var) { + return !isResourceVariableClosed(var); + } + + private boolean isResourceVariableClosed(ASTVariableDeclarator var) { + Node methodOfVar = getMethodOfNode(var); + return hasTryStatementClosingResourceVariable(methodOfVar, var) + || isReturnedByMethod(var.getName(), methodOfVar); + } + + private Node getMethodOfNode(Node node) { + Node parent = node.getParent(); + while (isNotMethod(parent)) { + parent = parent.getParent(); } - return null; + return parent; + } + + private boolean isNotMethod(Node node) { + return !(node instanceof ASTBlock || node instanceof ASTConstructorDeclaration); + } + + private boolean hasTryStatementClosingResourceVariable(Node node, ASTVariableDeclarator var) { + List tryStatements = node.findDescendantsOfType(ASTTryStatement.class, true); + for (ASTTryStatement tryStatement : tryStatements) { + if (tryStatementClosesResourceVariable(tryStatement, var)) { + return true; + } + } + return false; + } + + private boolean tryStatementClosesResourceVariable(ASTTryStatement tryStatement, ASTVariableDeclarator var) { + if (tryStatement.getBeginLine() >= var.getBeginLine() && noneCriticalStatementsBetween(var, tryStatement)) { + if (hasFinallyClause(tryStatement)) { + ASTBlock finallyBody = tryStatement.getFinallyClause().getBody(); + return blockClosesResourceVariable(finallyBody, var.getName()); + } else if (tryStatement.isTryWithResources()) { + return isVariableSpecifiedInTryWithResource(var.getName(), tryStatement); + } + } + return false; + } + + private boolean noneCriticalStatementsBetween(ASTVariableDeclarator var, ASTTryStatement tryStatement) { + return !anyCriticalStatementBetween(var, tryStatement); + } + + private boolean anyCriticalStatementBetween(ASTVariableDeclarator var, ASTTryStatement tryStatement) { + ASTBlockStatement varBlockStatement = var.getFirstParentOfType(ASTBlockStatement.class); + ASTBlockStatement tryBlockStatement = tryStatement.getFirstParentOfType(ASTBlockStatement.class); + if (isNotNullInitialized(var) && areStatementsOfSameBlock(varBlockStatement, tryBlockStatement)) { + for (ASTBlockStatement bsBetween : getBlockStatementsBetween(varBlockStatement, tryBlockStatement)) { + if (isCriticalStatement(bsBetween)) { + return true; + } + } + } + return false; + } + + private boolean isNotNullInitialized(ASTVariableDeclarator var) { + return !hasNullInitializer(var); + } + + private boolean hasNullInitializer(ASTVariableDeclarator var) { + if (var.hasInitializer()) { + ASTPrimaryPrefix primaryPrefix = var.getInitializer().getFirstDescendantOfType(ASTPrimaryPrefix.class); + return primaryPrefix != null && primaryPrefix.hasDescendantOfType(ASTNullLiteral.class); + } + return false; + } + + private boolean areStatementsOfSameBlock(ASTBlockStatement bs0, ASTBlockStatement bs1) { + return bs0.getParent() == bs1.getParent(); + } + + private List getBlockStatementsBetween(ASTBlockStatement top, ASTBlockStatement bottom) { + List blockStatements = top.getParent().findChildrenOfType(ASTBlockStatement.class); + int topBSIndex = blockStatements.indexOf(top); + int bottomBSIndex = blockStatements.indexOf(bottom); + return blockStatements.subList(topBSIndex + 1, bottomBSIndex); + } + + private boolean isCriticalStatement(ASTBlockStatement blockStatement) { + boolean isVarDeclaration = blockStatement.hasDescendantOfType(ASTLocalVariableDeclaration.class); + boolean isAssignmentOperator = blockStatement.hasDescendantOfType(ASTAssignmentOperator.class); + return !isVarDeclaration && !isAssignmentOperator; + } + + private boolean hasFinallyClause(ASTTryStatement tryStatement) { + return tryStatement.getFinallyClause() != null; + } + + private boolean blockClosesResourceVariable(ASTBlock block, String variableToClose) { + return hasNotConditionalCloseCallOnVariable(block, variableToClose) + || hasMethodCallClosingResourceVariable(block, variableToClose); + } + + private boolean hasNotConditionalCloseCallOnVariable(ASTBlock block, String variableToClose) { + List operations = block.findDescendantsOfType(ASTName.class); + for (ASTName operation : operations) { + if (isCloseCallOnVariable(operation, variableToClose) + && isNotConditional(block, operation, variableToClose)) { + return true; + } + } + return false; + } + + private boolean isCloseCallOnVariable(ASTName op, String variableToClose) { + String opName = op.getImage(); + if (opName != null && opName.contains(".")) { + String[] parts = opName.split("\\."); + if (parts.length == 2) { + String methodName = parts[1]; + String varName = parts[0]; + return varName.equals(variableToClose) && closeTargets.contains(methodName); + } + } + return false; } /** - * Checks, whether the given node is inside a if condition, and if so, + * Checks, whether the given node is inside an if condition, and if so, * whether this is a null check for the given varName. * * @param enclosingBlock @@ -524,7 +456,7 @@ public class CloseResourceRule extends AbstractJavaRule { * @return true if no if condition is involved or if the if * condition is a null-check. */ - private boolean nullCheckIfCondition(ASTBlock enclosingBlock, Node node, String varName) { + private boolean isNotConditional(ASTBlock enclosingBlock, Node node, String varName) { ASTIfStatement ifStatement = findIfStatement(enclosingBlock, node); if (ifStatement != null) { try { @@ -539,4 +471,103 @@ public class CloseResourceRule extends AbstractJavaRule { } return true; } + + private ASTIfStatement findIfStatement(ASTBlock enclosingBlock, Node node) { + ASTIfStatement ifStatement = node.getFirstParentOfType(ASTIfStatement.class); + List allIfStatements = enclosingBlock.findDescendantsOfType(ASTIfStatement.class); + if (ifStatement != null && allIfStatements.contains(ifStatement)) { + return ifStatement; + } + return null; + } + + private boolean hasMethodCallClosingResourceVariable(ASTBlock block, String variableToClose) { + List expressions = block.findDescendantsOfType(ASTPrimaryExpression.class, true); + for (ASTPrimaryExpression expression : expressions) { + if (isMethodCallClosingResourceVariable(expression, variableToClose)) { + return true; + } + } + return false; + } + + private boolean isMethodCallClosingResourceVariable(ASTPrimaryExpression expression, String variableToClose) { + ASTPrimaryPrefix prefix = expression.getFirstDescendantOfType(ASTPrimaryPrefix.class); + ASTPrimarySuffix suffix = expression.getFirstDescendantOfType(ASTPrimarySuffix.class); + if (prefix != null && suffix != null) { + return (isCloseTargetMethodCall(prefix, suffix) || hasChainedCloseTargetMethodCall(expression)) + && variableIsPassedToMethod(variableToClose, expression); + } + return false; + } + + private boolean isCloseTargetMethodCall(ASTPrimaryPrefix prefix, ASTPrimarySuffix suffix) { + String methodCall = getMethodCallStr(prefix, suffix); + return methodCall != null && closeTargets.contains(methodCall); + } + + private String getMethodCallStr(ASTPrimaryPrefix prefix, ASTPrimarySuffix suffix) { + if (prefix.getImage() == null) { + ASTName name = prefix.getFirstDescendantOfType(ASTName.class); + return name != null ? name.getImage() : null; + } else if (suffix.getImage() != null) { + return prefix.getImage() + "." + suffix.getImage(); + } + return null; + } + + private boolean hasChainedCloseTargetMethodCall(ASTPrimaryExpression expr) { + List methodCalls = expr.findDescendantsOfType(ASTPrimarySuffix.class, true); + for (ASTPrimarySuffix methodCall : methodCalls) { + if (closeTargets.contains(methodCall.getImage())) { + return true; + } + } + return false; + } + + private boolean variableIsPassedToMethod(String varName, ASTPrimaryExpression methodCall) { + List methodCallArgs = methodCall.findDescendantsOfType(ASTName.class, true); + for (ASTName methodCallArg : methodCallArgs) { + if (isMethodCallArgument(methodCallArg) && methodCallArg.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private boolean isMethodCallArgument(ASTName varName) { + return varName.getFirstParentOfType(ASTArgumentList.class) != null; + } + + private boolean isVariableSpecifiedInTryWithResource(String varName, ASTTryStatement tryWithResource) { + List specifiedResources = tryWithResource.getFirstChildOfType(ASTResourceSpecification.class) + .findDescendantsOfType(ASTName.class); + for (ASTName res : specifiedResources) { + if (res.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private boolean isReturnedByMethod(String varName, Node method) { + List returns = method.findDescendantsOfType(ASTReturnStatement.class, true); + for (ASTReturnStatement returnStatement : returns) { + ASTName name = returnStatement.getFirstDescendantOfType(ASTName.class); + if (name != null && name.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private void addCloseResourceViolation(ASTVariableDeclaratorId id, TypeNode type, Object data) { + Class typeClass = type.getType(); + if (typeClass != null) { + addViolation(data, id, typeClass.getSimpleName()); + } else { + addViolation(data, id, id.getTypeNode().getTypeImage()); + } + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 72e25980fb..b2f7f6026a 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1292,6 +1292,27 @@ public class CloseResourceNullPointer { public void check(UnknownType param) { InputStream in = param; } +} + ]]> + + + + #2470 false-positive at lambda returned from method + 0 + bar() throws IOException { + InputStream inputStream = new FileInputStream("/test.txt"); + return () -> { + try { + return inputStream.read(); + } finally { + inputStream.close(); + } + }; + } } ]]> From aa3271978ff1b2c114adcb288bec304d4393df98 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Mon, 27 Jul 2020 23:45:02 +0300 Subject: [PATCH 2/8] CloseResource: a method call as an allocation argument bug fix --- .../rule/errorprone/CloseResourceRule.java | 20 +++++++++---------- .../rule/errorprone/xml/CloseResource.xml | 17 ++++++++++++++++ 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 87453c06a1..8a37cec3c4 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -187,15 +187,6 @@ public class CloseResourceRule extends AbstractJavaRule { return null; } - private boolean isNotMethodCall(ASTExpression expr) { - return !isMethodCall(expr); - } - - private boolean isMethodCall(ASTExpression expression) { - return expression != null && expression.getNumChildren() > 0 - && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; - } - private TypeNode wrappedResourceTypeOrReturn(ASTVariableDeclarator var, TypeNode defaultVal) { if (var.hasInitializer()) { TypeNode wrappedResType = getWrappedResourceType(var); @@ -221,7 +212,7 @@ public class CloseResourceRule extends AbstractJavaRule { ASTArgumentList argsList = allocation.getFirstDescendantOfType(ASTArgumentList.class); if (argsList != null) { ASTExpression firstArg = argsList.getFirstChildOfType(ASTExpression.class); - return isNotLiteral(firstArg) ? firstArg : null; + return isNotLiteral(firstArg) && isNotMethodCall(firstArg) ? firstArg : null; } } return null; @@ -241,6 +232,15 @@ public class CloseResourceRule extends AbstractJavaRule { return literal == null; } + private boolean isNotMethodCall(ASTExpression expr) { + return !isMethodCall(expr); + } + + private boolean isMethodCall(ASTExpression expression) { + return expression != null && expression.getNumChildren() > 0 + && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; + } + private boolean isNotAllowedResourceType(TypeNode varType) { return !isAllowedResourceType(varType); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index b2f7f6026a..bff188d98b 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1313,6 +1313,23 @@ public class Foo { } }; } +} + ]]> + + + + wrapped ByteArrayInputStream false-negative test + 1 + 5 + From 58c55d07ed9c9fb73184ac311e99b9cc81928040 Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Tue, 28 Jul 2020 10:58:45 +0300 Subject: [PATCH 3/8] CloseResource: addCloseResourceViolation fix --- .../java/rule/errorprone/CloseResourceRule.java | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 8a37cec3c4..f72efefd1b 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -563,11 +563,18 @@ public class CloseResourceRule extends AbstractJavaRule { } private void addCloseResourceViolation(ASTVariableDeclaratorId id, TypeNode type, Object data) { + String resTypeName = getResourceTypeName(id, type); + addViolation(data, id, resTypeName); + } + + private String getResourceTypeName(ASTVariableDeclaratorId varId, TypeNode type) { Class typeClass = type.getType(); - if (typeClass != null) { - addViolation(data, id, typeClass.getSimpleName()); - } else { - addViolation(data, id, id.getTypeNode().getTypeImage()); + if (typeClass == null) { + ASTLocalVariableDeclaration localVarDecl = varId.getFirstParentOfType(ASTLocalVariableDeclaration.class); + return localVarDecl != null && localVarDecl.getTypeNode() != null + ? localVarDecl.getTypeNode().getTypeImage() + : varId.getName(); } + return typeClass.getSimpleName(); } } From db8b1aebd378fc46979c8225946ab9d0e370a0ee Mon Sep 17 00:00:00 2001 From: Mykhailo Palahuta Date: Mon, 3 Aug 2020 17:14:44 +0300 Subject: [PATCH 4/8] CloseResource: close() should be in finally detection; try-with-resource var wrapping detection; wrapped type resolution fix --- .../rule/errorprone/CloseResourceRule.java | 216 ++++++++++++------ .../rule/errorprone/xml/CloseResource.xml | 76 +++--- 2 files changed, 193 insertions(+), 99 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index f72efefd1b..97b9af1885 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -7,6 +7,7 @@ package net.sourceforge.pmd.lang.java.rule.errorprone; import static net.sourceforge.pmd.properties.PropertyFactory.booleanProperty; import static net.sourceforge.pmd.properties.PropertyFactory.stringListProperty; +import java.util.ArrayList; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -25,9 +26,9 @@ import net.sourceforge.pmd.lang.java.ast.ASTBlockStatement; import net.sourceforge.pmd.lang.java.ast.ASTClassOrInterfaceType; import net.sourceforge.pmd.lang.java.ast.ASTConstructorDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTExpression; +import net.sourceforge.pmd.lang.java.ast.ASTFinallyStatement; import net.sourceforge.pmd.lang.java.ast.ASTFormalParameters; import net.sourceforge.pmd.lang.java.ast.ASTIfStatement; -import net.sourceforge.pmd.lang.java.ast.ASTLiteral; import net.sourceforge.pmd.lang.java.ast.ASTLocalVariableDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodDeclaration; import net.sourceforge.pmd.lang.java.ast.ASTMethodOrConstructorDeclaration; @@ -41,6 +42,7 @@ import net.sourceforge.pmd.lang.java.ast.ASTReturnStatement; import net.sourceforge.pmd.lang.java.ast.ASTTryStatement; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclarator; import net.sourceforge.pmd.lang.java.ast.ASTVariableDeclaratorId; +import net.sourceforge.pmd.lang.java.ast.JavaNode; import net.sourceforge.pmd.lang.java.ast.TypeNode; import net.sourceforge.pmd.lang.java.rule.AbstractJavaRule; import net.sourceforge.pmd.lang.java.typeresolution.TypeHelper; @@ -64,6 +66,9 @@ import net.sourceforge.pmd.properties.PropertyDescriptor; */ public class CloseResourceRule extends AbstractJavaRule { + private static final String WRAPPING_TRY_WITH_RES_VAR_MESSAGE = "it is recommended to wrap resource in try-with-resource declaration directly"; + private static final String CLOSE_IN_FINALLY_BLOCK_MESSAGE = "'' is not closed within a finally block, thus might not be closed at all in case of exceptions"; + private final Set types = new HashSet<>(); private final Set simpleTypes = new HashSet<>(); @@ -141,13 +146,14 @@ public class CloseResourceRule extends AbstractJavaRule { return super.visit(node, data); } - private void checkForResources(ASTMethodOrConstructorDeclaration node, Object data) { - Map resVars = getResourceVariables(node); + private void checkForResources(ASTMethodOrConstructorDeclaration methodOrConstructor, Object data) { + Map resVars = getResourceVariables(methodOrConstructor); for (Map.Entry resVarEntry : resVars.entrySet()) { - TypeNode resVarType = resVarEntry.getValue(); ASTVariableDeclarator resVar = resVarEntry.getKey(); - if (isNotAllowedResourceType(resVarType) && isNotWrappingResourceMethodParameter(resVar, node) - && isResourceVariableUnclosed(resVar)) { + TypeNode resVarType = resVarEntry.getValue(); + if (isWrappingResourceSpecifiedInTry(resVar)) { + addViolationWithMessage(data, resVar, WRAPPING_TRY_WITH_RES_VAR_MESSAGE); + } else if (shouldVarOfTypeBeClosedInMethod(resVar, resVarType, methodOrConstructor)) { addCloseResourceViolation(resVar.getVariableId(), resVarType, data); } } @@ -166,12 +172,8 @@ public class CloseResourceRule extends AbstractJavaRule { } private TypeNode getTypeOfVariable(ASTVariableDeclarator var) { - TypeNode declaredVarType = getDeclaredTypeOfVariable(var); - if (var.hasInitializer()) { - TypeNode runtimeVarType = getRuntimeTypeOfVariable(var); - return runtimeVarType != null ? runtimeVarType : declaredVarType; - } - return declaredVarType; + TypeNode runtimeType = getRuntimeTypeOfVariable(var); + return runtimeType != null ? runtimeType : getDeclaredTypeOfVariable(var); } private TypeNode getDeclaredTypeOfVariable(ASTVariableDeclarator var) { @@ -180,56 +182,58 @@ public class CloseResourceRule extends AbstractJavaRule { } private TypeNode getRuntimeTypeOfVariable(ASTVariableDeclarator var) { - ASTExpression initExpr = var.getInitializer().getFirstChildOfType(ASTExpression.class); - if (initExpr != null && isNotMethodCall(initExpr)) { - return initExpr.getType() != null ? initExpr : null; - } - return null; + ASTExpression initExpr = initializerExpressionOf(var); + return isRuntimeType(initExpr) ? initExpr : null; + } + + private boolean isRuntimeType(ASTExpression expr) { + return expr != null && isNotMethodCall(expr) && expr.getType() != null; } private TypeNode wrappedResourceTypeOrReturn(ASTVariableDeclarator var, TypeNode defaultVal) { - if (var.hasInitializer()) { - TypeNode wrappedResType = getWrappedResourceType(var); - return wrappedResType != null ? wrappedResType : defaultVal; - } - return defaultVal; + TypeNode wrappedResType = getWrappedResourceType(var); + return wrappedResType != null ? wrappedResType : defaultVal; } private TypeNode getWrappedResourceType(ASTVariableDeclarator var) { - ASTExpression initExpr = var.getInitializer().getFirstChildOfType(ASTExpression.class); + ASTExpression initExpr = initializerExpressionOf(var); if (initExpr != null) { - ASTExpression wrappedType = getAllocationFirstArgumentVariable(initExpr); - return wrappedType != null && isResourceTypeOrSubtype(wrappedType) - ? wrappedType - : null; - } - return null; - } - - private ASTExpression getAllocationFirstArgumentVariable(ASTExpression expression) { - ASTAllocationExpression allocation = getLastAllocationExpression(expression); - if (allocation != null) { - ASTArgumentList argsList = allocation.getFirstDescendantOfType(ASTArgumentList.class); - if (argsList != null) { - ASTExpression firstArg = argsList.getFirstChildOfType(ASTExpression.class); - return isNotLiteral(firstArg) && isNotMethodCall(firstArg) ? firstArg : null; + ASTAllocationExpression resAlloc = getLastResourceAllocation(initExpr); + if (resAlloc != null) { + ASTExpression firstArgRes = getFirstArgumentVariableIfResource(resAlloc); + return firstArgRes != null ? firstArgRes : resAlloc; } } return null; } - private ASTAllocationExpression getLastAllocationExpression(ASTExpression expression) { - List allocations = expression.findDescendantsOfType(ASTAllocationExpression.class); - if (!allocations.isEmpty()) { - int lastAllocationIndex = allocations.size() - 1; - return allocations.get(lastAllocationIndex); + private ASTExpression initializerExpressionOf(ASTVariableDeclarator var) { + return var.hasInitializer() + ? var.getInitializer().getFirstChildOfType(ASTExpression.class) + : null; + } + + private ASTAllocationExpression getLastResourceAllocation(ASTExpression expr) { + List allocations = expr.findDescendantsOfType(ASTAllocationExpression.class); + int lastAllocIndex = allocations.size() - 1; + for (int allocIndex = lastAllocIndex; allocIndex >= 0; allocIndex--) { + ASTAllocationExpression allocation = allocations.get(allocIndex); + if (isResourceTypeOrSubtype(allocation)) { + return allocation; + } } return null; } - private boolean isNotLiteral(ASTExpression expression) { - ASTLiteral literal = expression.getFirstDescendantOfType(ASTLiteral.class); - return literal == null; + private ASTExpression getFirstArgumentVariableIfResource(ASTAllocationExpression allocation) { + ASTArgumentList argsList = allocation.getFirstDescendantOfType(ASTArgumentList.class); + if (argsList != null) { + ASTExpression firstArg = argsList.getFirstChildOfType(ASTExpression.class); + return firstArg != null && isNotMethodCall(firstArg) && isResourceTypeOrSubtype(firstArg) + ? firstArg + : null; + } + return null; } private boolean isNotMethodCall(ASTExpression expr) { @@ -237,8 +241,30 @@ public class CloseResourceRule extends AbstractJavaRule { } private boolean isMethodCall(ASTExpression expression) { - return expression != null && expression.getNumChildren() > 0 - && expression.getChild(0).getFirstChildOfType(ASTPrimarySuffix.class) != null; + if (expression != null) { + ASTPrimaryExpression primaryExpression = expression.getFirstChildOfType(ASTPrimaryExpression.class); + return primaryExpression != null && primaryExpression.getFirstChildOfType(ASTPrimarySuffix.class) != null; + } + return false; + } + + private boolean isWrappingResourceSpecifiedInTry(ASTVariableDeclarator var) { + String wrappedVarName = getWrappedVariableName(var); + if (wrappedVarName != null) { + List tryContainers = var.getParentsOfType(ASTTryStatement.class); + for (ASTTryStatement tryContainer : tryContainers) { + if (isTryWithResourceSpecifyingVariable(tryContainer, wrappedVarName)) { + return true; + } + } + } + return false; + } + + private boolean shouldVarOfTypeBeClosedInMethod(ASTVariableDeclarator var, TypeNode type, + ASTMethodOrConstructorDeclaration method) { + return isNotAllowedResourceType(type) && isNotWrappingResourceMethodParameter(var, method) + && isResourceVariableUnclosed(var); } private boolean isNotAllowedResourceType(TypeNode varType) { @@ -354,11 +380,12 @@ public class CloseResourceRule extends AbstractJavaRule { private boolean tryStatementClosesResourceVariable(ASTTryStatement tryStatement, ASTVariableDeclarator var) { if (tryStatement.getBeginLine() >= var.getBeginLine() && noneCriticalStatementsBetween(var, tryStatement)) { + if (isTryWithResourceSpecifyingVariable(tryStatement, var.getName())) { + return true; + } if (hasFinallyClause(tryStatement)) { ASTBlock finallyBody = tryStatement.getFinallyClause().getBody(); return blockClosesResourceVariable(finallyBody, var.getName()); - } else if (tryStatement.isTryWithResources()) { - return isVariableSpecifiedInTryWithResource(var.getName(), tryStatement); } } return false; @@ -410,6 +437,34 @@ public class CloseResourceRule extends AbstractJavaRule { return !isVarDeclaration && !isAssignmentOperator; } + private boolean isTryWithResourceSpecifyingVariable(ASTTryStatement tryStatement, String varName) { + return tryStatement.isTryWithResources() && isVariableSpecifiedInTryWithResource(varName, tryStatement); + } + + private boolean isVariableSpecifiedInTryWithResource(String varName, ASTTryStatement tryWithResource) { + List specifiedResources = getResourcesSpecifiedInTryWith(tryWithResource); + for (JavaNode res : specifiedResources) { + if (res.hasImageEqualTo(varName)) { + return true; + } + } + return false; + } + + private List getResourcesSpecifiedInTryWith(ASTTryStatement tryWithResource) { + ASTResourceSpecification resSpecification = tryWithResource.getFirstChildOfType(ASTResourceSpecification.class); + List initializedVars = resSpecification + .findDescendantsOfType(ASTVariableDeclaratorId.class); + List specifiedVars = resSpecification.findDescendantsOfType(ASTName.class); + return combineNodeLists(initializedVars, specifiedVars); + } + + private List combineNodeLists(List list0, List list1) { + List nodeList = new ArrayList<>(list0); + nodeList.addAll(list1); + return nodeList; + } + private boolean hasFinallyClause(ASTTryStatement tryStatement) { return tryStatement.getFinallyClause() != null; } @@ -431,16 +486,8 @@ public class CloseResourceRule extends AbstractJavaRule { } private boolean isCloseCallOnVariable(ASTName op, String variableToClose) { - String opName = op.getImage(); - if (opName != null && opName.contains(".")) { - String[] parts = opName.split("\\."); - if (parts.length == 2) { - String methodName = parts[1]; - String varName = parts[0]; - return varName.equals(variableToClose) && closeTargets.contains(methodName); - } - } - return false; + String closedVar = getVariableClosedByMethodCall(op); + return variableToClose.equals(closedVar); } /** @@ -540,17 +587,6 @@ public class CloseResourceRule extends AbstractJavaRule { return varName.getFirstParentOfType(ASTArgumentList.class) != null; } - private boolean isVariableSpecifiedInTryWithResource(String varName, ASTTryStatement tryWithResource) { - List specifiedResources = tryWithResource.getFirstChildOfType(ASTResourceSpecification.class) - .findDescendantsOfType(ASTName.class); - for (ASTName res : specifiedResources) { - if (res.hasImageEqualTo(varName)) { - return true; - } - } - return false; - } - private boolean isReturnedByMethod(String varName, Node method) { List returns = method.findDescendantsOfType(ASTReturnStatement.class, true); for (ASTReturnStatement returnStatement : returns) { @@ -577,4 +613,42 @@ public class CloseResourceRule extends AbstractJavaRule { } return typeClass.getSimpleName(); } + + @Override + public Object visit(ASTPrimaryPrefix prefix, Object data) { + ASTName methodCall = prefix.getFirstChildOfType(ASTName.class); + if (methodCall != null) { + String closedVar = getVariableClosedByMethodCall(methodCall); + if (closedVar != null && isNotInFinallyBlock(prefix)) { + String violationMsg = closeInFinallyBlockMessageForVar(closedVar); + addViolationWithMessage(data, prefix, violationMsg); + } + } + return super.visit(prefix, data); + } + + private String getVariableClosedByMethodCall(ASTName methodCall) { + String[] callParts = getMethodCallParts(methodCall); + if (callParts != null) { + String varName = callParts[0]; + String methodName = callParts[1]; + return closeTargets.contains(methodName) ? varName : null; + } + return null; + } + + private String[] getMethodCallParts(ASTName methodCall) { + String methodCallStr = methodCall.getImage(); + return methodCallStr != null && methodCallStr.contains(".") + ? methodCallStr.split("\\.") + : null; + } + + private boolean isNotInFinallyBlock(ASTPrimaryPrefix prefix) { + return prefix.getFirstParentOfType(ASTFinallyStatement.class) == null; + } + + private String closeInFinallyBlockMessageForVar(String var) { + return "''" + var + CLOSE_IN_FINALLY_BLOCK_MESSAGE; + } } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index bff188d98b..3eea2bd412 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -377,7 +377,7 @@ public class Test { #947 CloseResource rule fails if field is marked with annotation - 2 + 3 #1375 CloseResource not detected properly - ok - 1 + 2 #1375 CloseResource not detected properly - false negative - 1 + 2 - - False-negative if only byte array is passed in as method parameter - 1 - 6 - - - NullPointerException if type of method parameter is not known 1 @@ -1320,7 +1296,10 @@ public class Foo { wrapped ByteArrayInputStream false-negative test 1 - 5 + 7 + + 'bos' is not closed within a finally block, thus might not be closed at all in case of exceptions + + + + + lambda doesn't close resource false-negative test + 1 + 4 + { + try { + int d = is.read(); + } catch (IOException e) { + e.printStackTrace(); + } + }; + } +} + ]]> + + + + don't wrap try-with-resource variable test + 1 + 5 + + it is recommended to wrap resource in try-with-resource declaration directly + + From 9e1370fac7b1babb781a14b3900ce7f43f2c2194 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 20 Aug 2020 19:07:52 +0200 Subject: [PATCH 5/8] [java] CloseResource - avoid duplicated violations --- .../rule/errorprone/CloseResourceRule.java | 16 +++++++++---- .../rule/errorprone/xml/CloseResource.xml | 23 +++++++++++++++---- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 97b9af1885..01444f86c5 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -69,10 +69,6 @@ public class CloseResourceRule extends AbstractJavaRule { private static final String WRAPPING_TRY_WITH_RES_VAR_MESSAGE = "it is recommended to wrap resource in try-with-resource declaration directly"; private static final String CLOSE_IN_FINALLY_BLOCK_MESSAGE = "'' is not closed within a finally block, thus might not be closed at all in case of exceptions"; - private final Set types = new HashSet<>(); - private final Set simpleTypes = new HashSet<>(); - - private final Set closeTargets = new HashSet<>(); private static final PropertyDescriptor> CLOSE_TARGETS_DESCRIPTOR = stringListProperty("closeTargets") .desc("Methods which may close this resource") @@ -97,6 +93,13 @@ public class CloseResourceRule extends AbstractJavaRule { "java.util.stream.DoubleStream") .build(); + private final Set types = new HashSet<>(); + private final Set simpleTypes = new HashSet<>(); + private final Set closeTargets = new HashSet<>(); + + // keeps track of already reported violations to avoid duplicated violations for the same variable + private final Set reportedVarNames = new HashSet<>(); + public CloseResourceRule() { definePropertyDescriptor(CLOSE_TARGETS_DESCRIPTOR); @@ -147,13 +150,16 @@ public class CloseResourceRule extends AbstractJavaRule { } private void checkForResources(ASTMethodOrConstructorDeclaration methodOrConstructor, Object data) { + reportedVarNames.clear(); Map resVars = getResourceVariables(methodOrConstructor); for (Map.Entry resVarEntry : resVars.entrySet()) { ASTVariableDeclarator resVar = resVarEntry.getKey(); TypeNode resVarType = resVarEntry.getValue(); if (isWrappingResourceSpecifiedInTry(resVar)) { + reportedVarNames.add(resVar.getVariableId().getName()); addViolationWithMessage(data, resVar, WRAPPING_TRY_WITH_RES_VAR_MESSAGE); } else if (shouldVarOfTypeBeClosedInMethod(resVar, resVarType, methodOrConstructor)) { + reportedVarNames.add(resVar.getVariableId().getName()); addCloseResourceViolation(resVar.getVariableId(), resVarType, data); } } @@ -619,7 +625,7 @@ public class CloseResourceRule extends AbstractJavaRule { ASTName methodCall = prefix.getFirstChildOfType(ASTName.class); if (methodCall != null) { String closedVar = getVariableClosedByMethodCall(methodCall); - if (closedVar != null && isNotInFinallyBlock(prefix)) { + if (closedVar != null && isNotInFinallyBlock(prefix) && !reportedVarNames.contains(closedVar)) { String violationMsg = closeInFinallyBlockMessageForVar(closedVar); addViolationWithMessage(data, prefix, violationMsg); } diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 3eea2bd412..1a04b972b3 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -377,7 +377,12 @@ public class Test { #947 CloseResource rule fails if field is marked with annotation - 3 + 2 + 8,9 + + Ensure that resources like this Connection object are closed after use + Ensure that resources like this Statement object are closed after use + #1375 CloseResource not detected properly - ok - 2 + 1 + 6 + + Ensure that resources like this ResultSet object are closed after use + #1375 CloseResource not detected properly - false negative - 2 + 1 + 6 + + Ensure that resources like this ResultSet object are closed after use + - + lambda doesn't close resource false-negative test 1 @@ -1333,7 +1346,7 @@ public class Foo { } ]]> - + don't wrap try-with-resource variable test 1 From 61ba8efcc1f056bee8b2774621833b78ed47e8f3 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Thu, 20 Aug 2020 19:10:01 +0200 Subject: [PATCH 6/8] [doc] Update release notes, fixes #2470, refs #2671 --- docs/pages/release_notes.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index c095e78be5..617a817f8d 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -34,6 +34,7 @@ This is a {{ site.pmd.release_type }} release. * [#2174](https://github.com/pmd/pmd/issues/2174): \[java] LawOfDemeter: False positive with 'this' pointer * [#2189](https://github.com/pmd/pmd/issues/2189): \[java] LawOfDemeter: False positive when casting to derived class * java-errorprone + * [#2470](https://github.com/pmd/pmd/issues/2470): \[java] CloseResource false positive when resource included in return value * [#2578](https://github.com/pmd/pmd/issues/2578): \[java] AvoidCallingFinalize detects some false positives * [#2634](https://github.com/pmd/pmd/issues/2634): \[java] NullPointerException in rule ProperCloneImplementation * java-performance @@ -60,6 +61,7 @@ This is a {{ site.pmd.release_type }} release. * [#2621](https://github.com/pmd/pmd/pull/2621): \[visualforce] add new safe resource for VfUnescapeEl - [Peter Chittum](https://github.com/pchittum) * [#2640](https://github.com/pmd/pmd/pull/2640): \[java] NullPointerException in rule ProperCloneImplementation - [Mykhailo Palahuta](https://github.com/Drofff) * [#2643](https://github.com/pmd/pmd/pull/2643): \[java] AvoidCallingFinalize detects some false positives (2578) - [Mykhailo Palahuta](https://github.com/Drofff) +* [#2671](https://github.com/pmd/pmd/pull/2671): \[java] CloseResource false positive when resource included in return value - [Mykhailo Palahuta](https://github.com/Drofff) {% endtocmaker %} From 36f61f44aa83eef19443ff20166fa914966da05b Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 21 Aug 2020 09:02:29 +0200 Subject: [PATCH 7/8] [java] CloseResource: fix false positive with close on not closeable --- .../rule/errorprone/CloseResourceRule.java | 2 +- .../rule/errorprone/xml/CloseResource.xml | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index 01444f86c5..e2f3b97045 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -623,7 +623,7 @@ public class CloseResourceRule extends AbstractJavaRule { @Override public Object visit(ASTPrimaryPrefix prefix, Object data) { ASTName methodCall = prefix.getFirstChildOfType(ASTName.class); - if (methodCall != null) { + if (methodCall != null && isNodeInstanceOfResourceType(methodCall)) { String closedVar = getVariableClosedByMethodCall(methodCall); if (closedVar != null && isNotInFinallyBlock(prefix) && !reportedVarNames.contains(closedVar)) { String violationMsg = closeInFinallyBlockMessageForVar(closedVar); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 1a04b972b3..9758e85511 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -1363,6 +1363,25 @@ public class Foo { int d = ois.read(); } } +} + ]]> + + + + false positive with close on not closeable + 0 + From b73b1e92f3951915cf25b9659e6ba51d4a80c106 Mon Sep 17 00:00:00 2001 From: Andreas Dangel Date: Fri, 21 Aug 2020 09:33:24 +0200 Subject: [PATCH 8/8] [java] CloseResource: add new property "closeNotInFinally" --- docs/pages/release_notes.md | 10 ++++++++++ .../lang/java/rule/errorprone/CloseResourceRule.java | 9 +++++++++ .../lang/java/rule/errorprone/xml/CloseResource.xml | 3 +++ 3 files changed, 22 insertions(+) diff --git a/docs/pages/release_notes.md b/docs/pages/release_notes.md index 7dee579f3b..43c3e9c56e 100644 --- a/docs/pages/release_notes.md +++ b/docs/pages/release_notes.md @@ -36,6 +36,16 @@ See also [[all] Ensure PMD/CPD uses tab width of 1 for tabs consistently #2656]( cases where the variable of the caught exception is reassigned. This practice is surprising and prevents further evolution of the code like multi-catch. +#### Modified Rules + +* The Java rule {% rule "java/errorprone/CloseResource" %} (`java-errorprone`) has a new property + `closeNotInFinally`. With this property set to `true` the rule will also find calls to close a + resource, which are not in a finally-block of a try-statement. If a resource is not closed within a + finally block, it might not be closed at all in case of exceptions. + + As this new detection would yield many new violations, it is disabled by default. It might be + enabled in a later version of PMD. + #### Deprecated Rules * The Java rule {% rule "java/errorprone/DataflowAnomalyAnalysis" %} (`java-errorprone`) diff --git a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java index e2f3b97045..25bf71385a 100644 --- a/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java +++ b/pmd-java/src/main/java/net/sourceforge/pmd/lang/java/rule/errorprone/CloseResourceRule.java @@ -93,6 +93,10 @@ public class CloseResourceRule extends AbstractJavaRule { "java.util.stream.DoubleStream") .build(); + private static final PropertyDescriptor DETECT_CLOSE_NOT_IN_FINALLY = + booleanProperty("closeNotInFinally") + .desc("Detect if 'close' (or other closeTargets) is called outside of a finally-block").defaultValue(false).build(); + private final Set types = new HashSet<>(); private final Set simpleTypes = new HashSet<>(); private final Set closeTargets = new HashSet<>(); @@ -106,6 +110,7 @@ public class CloseResourceRule extends AbstractJavaRule { definePropertyDescriptor(TYPES_DESCRIPTOR); definePropertyDescriptor(USE_CLOSE_AS_DEFAULT_TARGET); definePropertyDescriptor(ALLOWED_RESOURCE_TYPES); + definePropertyDescriptor(DETECT_CLOSE_NOT_IN_FINALLY); } @Override @@ -622,6 +627,10 @@ public class CloseResourceRule extends AbstractJavaRule { @Override public Object visit(ASTPrimaryPrefix prefix, Object data) { + if (!getProperty(DETECT_CLOSE_NOT_IN_FINALLY)) { + return super.visit(prefix, data); + } + ASTName methodCall = prefix.getFirstChildOfType(ASTName.class); if (methodCall != null && isNodeInstanceOfResourceType(methodCall)) { String closedVar = getVariableClosedByMethodCall(methodCall); diff --git a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml index 9758e85511..3b3894818f 100644 --- a/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml +++ b/pmd-java/src/test/resources/net/sourceforge/pmd/lang/java/rule/errorprone/xml/CloseResource.xml @@ -377,6 +377,7 @@ public class Test { #947 CloseResource rule fails if field is marked with annotation + true 2 8,9 @@ -548,6 +549,7 @@ public class Bar { #1375 CloseResource not detected properly - ok + true 1 6 @@ -1308,6 +1310,7 @@ public class Foo { wrapped ByteArrayInputStream false-negative test + true 1 7