From 74a96c696f896b1a586799b8599c323b3e437028 Mon Sep 17 00:00:00 2001 From: Quentin Jaquier Date: Tue, 20 Nov 2018 10:29:23 +0100 Subject: [PATCH 1/2] Add null pointer checker belief style --- .../test/resources/jdk6/squid-S12345678.json | 5 + .../NullDereferenceBeliefStyleCheck.java | 212 ++++++++++++++++++ .../l10n/java/rules/squid/S12345678_java.html | 1 + .../l10n/java/rules/squid/S12345678_java.json | 16 ++ .../NullDereferenceBeliefStyleCheck.java | 211 +++++++++++++++++ .../NullDereferenceBeliefStyleCheckTest.java | 32 +++ 6 files changed, 477 insertions(+) create mode 100644 its/ruling/src/test/resources/jdk6/squid-S12345678.json create mode 100644 java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.html create mode 100644 java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.json create mode 100644 java-checks/src/test/files/checks/NullDereferenceBeliefStyleCheck.java create mode 100644 java-checks/src/test/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheckTest.java diff --git a/its/ruling/src/test/resources/jdk6/squid-S12345678.json b/its/ruling/src/test/resources/jdk6/squid-S12345678.json new file mode 100644 index 00000000000..a31875ecab3 --- /dev/null +++ b/its/ruling/src/test/resources/jdk6/squid-S12345678.json @@ -0,0 +1,5 @@ +{ +'jdk6:java/awt/image/LookupOp.java':[ +245, +], +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java b/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java new file mode 100644 index 00000000000..66cc73b5380 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java @@ -0,0 +1,212 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2018 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import java.util.Deque; +import java.util.HashMap; +import java.util.HashSet; +import java.util.LinkedList; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; + +import org.sonar.check.Rule; +import org.sonar.java.cfg.CFG; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.Tree; +import org.sonar.plugins.java.api.tree.VariableTree; + +@Rule(key = "S12345678") +public class NullDereferenceBeliefStyleCheck extends IssuableSubscriptionVisitor { + + @Override + public List nodesToVisit() { + return ImmutableList.of(Tree.Kind.METHOD); + } + + @Override + public void visitNode(Tree tree) { + if (!hasSemantic()) { + return; + } + MethodTree methodTree = (MethodTree) tree; + if (methodTree.block() == null) { + return; + } + + CFG cfg = CFG.build(methodTree); + NullTracking nullTracking = NullTracking.analyse(cfg); + + for (CFG.Block block : cfg.blocks()) { + block.elements().forEach(element -> checkElement(element, nullTracking.getOut(block))); + } + } + + private void checkElement(Tree element, Set out) { + if(element.is(Tree.Kind.EQUAL_TO)){ + processEqualTo((BinaryExpressionTree) element, out); + } + } + + private void processEqualTo(BinaryExpressionTree element, Set out) { + if(element.rightOperand().is(Tree.Kind.NULL_LITERAL) && element.leftOperand().is(Tree.Kind.IDENTIFIER)) { + IdentifierTree id = (IdentifierTree) element.leftOperand(); + if(out.contains(id.symbol())) { + reportIssue(id, "Null possible"); + } + } + } + + private static class NullTracking { + private final CFG cfg; + private final Map> out = new HashMap<>(); + + private NullTracking(CFG cfg) { + this.cfg = cfg; + } + + private Set getOut(CFG.Block block) { + return out.get(block); + } + + private static NullTracking analyse(CFG cfg) { + NullTracking nullTracking = new NullTracking(cfg); + //Generate kill/gen for each block in isolation + Map> kill = new HashMap<>(); + Map> gen = new HashMap<>(); + + for (CFG.Block block : cfg.blocks()) { + Set blockKill = new HashSet<>(); + Set blockGen = new HashSet<>(); + + nullTracking.processBlockElements(block, blockKill, blockGen); + + kill.put(block, blockKill); + gen.put(block, blockGen); + } + nullTracking.analyzeCFG(kill, gen); + + // Make things immutable. + for (Map.Entry> blockSetEntry : nullTracking.out.entrySet()) { + blockSetEntry.setValue(ImmutableSet.copyOf(blockSetEntry.getValue())); + } + + return nullTracking; + } + + //Forward analysis + private void analyzeCFG(Map> kill, Map> gen) { + Deque workList = new LinkedList<>(); + workList.addAll(cfg.blocks()); + while (!workList.isEmpty()) { + CFG.Block block = workList.removeFirst(); + + Set blockIn; + + //Collect all predecessors out set + List> preds = block.predecessors().stream().map(out::get).filter(Objects::nonNull).collect(Collectors.toList()); + block.exceptions().stream().map(out::get).filter(Objects::nonNull).forEach(preds::add); + + if(!preds.isEmpty()){ + Set newBlockIn = new HashSet<>(preds.get(0)); + for (int i = 1; i < preds.size(); i++){ + newBlockIn = Sets.intersection(newBlockIn, preds.get(i)); + } + blockIn = new HashSet<>(newBlockIn); + } else { + blockIn = new HashSet<>(); + } + + // out = gen and (in - kill) + Set newOut = new HashSet<>(gen.get(block)); + newOut.addAll(Sets.difference(blockIn, kill.get(block))); + + if (newOut.equals(out.get(block))) { + continue; + } + out.put(block, newOut); + block.successors().forEach(workList::addLast); + } + } + + private void processBlockElements(CFG.Block block, Set blockKill, Set blockGen) { + // process elements from bottom to top + for (Tree element : block.elements()) { + switch (element.kind()) { + case ASSIGNMENT: + processAssignment((AssignmentExpressionTree) element, blockKill, blockGen); + break; + case MEMBER_SELECT: + processPointerUse(element, blockGen); + break; + case METHOD_INVOCATION: + processMethodInvocation((MethodInvocationTree) element, blockGen); + break; + case VARIABLE: + processVariable((VariableTree) element, blockKill, blockGen); + break; + default: + // Ignore other kind of elements, no change of gen/kill + } + } + } + + private void processVariable(VariableTree element, Set blockKill, Set blockGen) { + blockKill.add(element.symbol()); + blockGen.remove(element.symbol()); + } + + private void processMethodInvocation(MethodInvocationTree element, Set blockGen) { + if(element.methodSelect().is(Tree.Kind.MEMBER_SELECT)) { + MemberSelectExpressionTree methodSelect = (MemberSelectExpressionTree)element.methodSelect(); + processPointerUse(methodSelect.expression(), blockGen); + } + } + + private void processPointerUse(Tree element, Set blockGen) { + if(element.is(Tree.Kind.IDENTIFIER)) { + blockGen.add(((IdentifierTree) element).symbol()); + } + } + + private void processAssignment(AssignmentExpressionTree element, Set blockKill, Set blockGen) { + ExpressionTree lhs = element.variable(); + if (lhs.is(Tree.Kind.IDENTIFIER)) { + Symbol symbol = ((IdentifierTree) lhs).symbol(); + //if we see an assignment, we remove all previously used pointer (we don't know anything for them anymore) + blockKill.add(symbol); + blockGen.remove(symbol); + } + } + } +} diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.html b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.html new file mode 100644 index 00000000000..b2ceffff9bd --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.html @@ -0,0 +1 @@ +

Fake rule descr

diff --git a/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.json b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.json new file mode 100644 index 00000000000..2efa95e3073 --- /dev/null +++ b/java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S12345678_java.json @@ -0,0 +1,16 @@ +{ + "title": "Fake rule desc", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "convention" + ], + "defaultSeverity": "Minor", + "ruleSpecification": "RSPEC-100", + "sqKey": "S100", + "scope": "All" +} diff --git a/java-checks/src/test/files/checks/NullDereferenceBeliefStyleCheck.java b/java-checks/src/test/files/checks/NullDereferenceBeliefStyleCheck.java new file mode 100644 index 00000000000..c229e623969 --- /dev/null +++ b/java-checks/src/test/files/checks/NullDereferenceBeliefStyleCheck.java @@ -0,0 +1,211 @@ +class A { + int compliant(Object p, boolean b) { + if(p == null){ } //Compliant + p.toString(); //Compliant,even though we have not assign p + } + + int compliant2(Object p, boolean b) { + if (p == null) {//Compliant + p = new Object(); + } + p.toString(); //Compliant + } + + int compliant3() { + Object p = null; + p.toString(); //Compliant, FN. Our checker is a naive version, the goal is not to find these kind of obvious NP. + } + + int foo(Object p, boolean b) { + p.toString(); //Add the belief that p is not null + //... + + if(p == null) {} // Noncompliant + + if(a) { + if(p == null){ } // Noncompliant + p = new Object(); + } + + if(p == null) { } // Compliant, one path does assign new value to p. + } + + int foo2(Object p, boolean b) { + p = new Object(); + p.toString(); //Add the belief that p is not null + //... + if(p == null) {} // Noncompliant + } + + int foo3(Object p, boolean b) { + p.toString(); //Add the belief that p is not null + p = new Object(); + //... + if(p == null) {} // Compliant + } + + int foo4(Object p, boolean b) { + //... + if(p == null || p.toString().equals("a")){} // Compliant + } + + int foo5(Object p, boolean b) { + String p = new Object(); + //... + if(p == null) { } // Compliant + } + + //== LOOP =================================================== + + int loop(Object p, boolean b) { + for(int i = 0; i < 10; i ++){ + if(p == null){ } // Compliant + p.toString(); + } + } + + int loop2(Object p, boolean b) { + p.toString(); + for(int i = 0; i < 10; i ++){ + if(p == null){ } // Noncompliant + } + } + + int loop3(Object p, boolean b) { + p.toString(); + for(int i = 0; i < 10; i ++){ + if(b){ + p = new Object(); + } + } + if(p == null){ } // Compliant, one path can re-asign p + } + + int loop4(Object p, boolean b) { + if(p.toString().equals("a")) { + while (p == null) { // Noncompliant + + } + } + } + + + int loop5(Object p, boolean b) { + p.toString(); + int i = 2; + do { + if(p == null){ } // Noncompliant + i++; + } while(i < 10); + } + + private String loopFooString() { + for (;;) { + String s = "a"; + if (s == null) { + return "a"; + } + + s.toString(); + } + } + + private Map.Entry lowestEntry() { + for (; ; ) { + ConcurrentSkipListMap.Node n = loNode(); + if (!isBeforeEnd(n)) + return null; + Map.Entry e = n.createSnapshot(); + if (e != null) + return e; + } + } + + private String shadowString(String s) { + s.toString(); + + String s = ""; + if(s == null){ } // Compliant + } + + //== Exception ======================================= + + void foo(boolean a, Object b) { + b.toString(); + + try{ + doSomething(); + if(b == null){ } // Noncompliant + } catch(Exception e) { + doSomething(); + if(b == null){ } // Noncompliant + } finally { + doSomething(); + if(b == null){ } // Noncompliant + } + + if(b == null){ } // Noncompliant + } + + void foo(boolean a, Object b) { + b.toString(); + + try{ + b = new Object(); + } catch(Exception e) { + e.toString(); + } + + if(b == null){ } // Compliant + } + + void foo(boolean a, Object b) { + b.toString(); + + try{ + doSomething(); + } catch(Exception e) { + b = new Object(); + } + + if(b == null){ } // Compliant + } + + void foo(boolean a, Object b) { + b.toString(); + + try{ + doSomething(); + } catch(Exception e) { + doSomething(); + } finally { + b = new Object(); + } + + if(b == null){ } // Compliant + } + + void doSomething() { } + + + private static class A { + String s = ""; + + void foo() { + if(s == null){ // Compliant + s = "a"; + } + s.toString(); + changeS(); // An other method can change the value of s! + + if(s == null) { // Noncompliant, FP, this check make sense + + } + } + + void changeS() { + s = null; + } + + } +} \ No newline at end of file diff --git a/java-checks/src/test/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheckTest.java new file mode 100644 index 00000000000..a4aeda8c9a1 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheckTest.java @@ -0,0 +1,32 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2018 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks; + +import org.junit.Test; +import org.sonar.java.checks.verifier.JavaCheckVerifier; + +public class NullDereferenceBeliefStyleCheckTest { + + @Test + public void test() { + JavaCheckVerifier.verify("src/test/files/checks/NullDereferenceBeliefStyleCheck.java", new NullDereferenceBeliefStyleCheck()); + } + +} From c8bc7eccdcf3458f8e1d61f095ee35efed94735d Mon Sep 17 00:00:00 2001 From: Quentin Jaquier Date: Tue, 20 Nov 2018 18:12:31 +0100 Subject: [PATCH 2/2] Improve the rule Add new way to raide NP and remove FP when the identifier are fields --- .../NullDereferenceBeliefStyleCheck.java | 31 ++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java b/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java index 66cc73b5380..edc0f4dd6e1 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/NullDereferenceBeliefStyleCheck.java @@ -36,6 +36,7 @@ import org.sonar.java.cfg.CFG; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.semantic.Symbol; +import org.sonar.plugins.java.api.tree.ArrayAccessExpressionTree; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; import org.sonar.plugins.java.api.tree.BinaryExpressionTree; import org.sonar.plugins.java.api.tree.ExpressionTree; @@ -82,7 +83,7 @@ private void processEqualTo(BinaryExpressionTree element, Set out) { if(element.rightOperand().is(Tree.Kind.NULL_LITERAL) && element.leftOperand().is(Tree.Kind.IDENTIFIER)) { IdentifierTree id = (IdentifierTree) element.leftOperand(); if(out.contains(id.symbol())) { - reportIssue(id, "Null possible"); + reportIssue(id, "This check is either always false, or a null pointer has been raised before."); } } } @@ -166,12 +167,15 @@ private void processBlockElements(CFG.Block block, Set blockKill, Set b private void processPointerUse(Tree element, Set blockGen) { if(element.is(Tree.Kind.IDENTIFIER)) { - blockGen.add(((IdentifierTree) element).symbol()); + Symbol symbol = ((IdentifierTree)element).symbol(); + if(symbol == null || symbol.owner() == null || symbol.owner().type() == null){ + return; + } + if(!symbol.owner().type().isClass()){ + blockGen.add(symbol); + } } } @@ -203,9 +213,14 @@ private void processAssignment(AssignmentExpressionTree element, Set blo ExpressionTree lhs = element.variable(); if (lhs.is(Tree.Kind.IDENTIFIER)) { Symbol symbol = ((IdentifierTree) lhs).symbol(); - //if we see an assignment, we remove all previously used pointer (we don't know anything for them anymore) - blockKill.add(symbol); - blockGen.remove(symbol); + if(symbol == null || symbol.owner() == null || symbol.owner().type() == null){ + return; + } + if(!symbol.owner().type().isClass()) { + //if we see an assignment, we remove all previously used pointer (we don't know anything for them anymore) + blockKill.add(symbol); + blockGen.remove(symbol); + } } } }