From f1b0e3c953e29418dbe8b7798cf827065dca4340 Mon Sep 17 00:00:00 2001 From: shubham ugare Date: Tue, 18 Jun 2019 02:01:00 +0530 Subject: [PATCH] Different approach for param analysis (#320) * Different approach for param analysis * Addressed comments --- .../jarinfer/DefinitelyDerefedParams.java | 60 +++++++++- .../uber/nullaway/jarinfer/JarInferTest.java | 104 +++++++++++++++++- 2 files changed, 153 insertions(+), 11 deletions(-) diff --git a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java index 98bb0e6c4d..092e35b133 100644 --- a/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java +++ b/jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java @@ -16,6 +16,7 @@ package com.uber.nullaway.jarinfer; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.ibm.wala.cfg.ControlFlowGraph; import com.ibm.wala.classLoader.IMethod; @@ -33,8 +34,12 @@ import com.ibm.wala.util.graph.GraphUtil; import com.ibm.wala.util.graph.dominators.Dominators; import com.ibm.wala.util.graph.impl.GraphInverter; +import com.ibm.wala.util.graph.traverse.DFS; +import com.sun.istack.internal.NotNull; import java.util.ArrayList; +import java.util.HashMap; import java.util.HashSet; +import java.util.Map; import java.util.Set; /** @@ -45,6 +50,7 @@ */ public class DefinitelyDerefedParams { private static final boolean DEBUG = false; + private boolean USE_EXTENDED_APPROACH = true; private static void LOG(boolean cond, String tag, String msg) { if (cond) System.out.println("[JI " + tag + "] " + msg); @@ -98,15 +104,61 @@ private static void LOG(boolean cond, String tag, String msg) { Set analyze() { // Get ExceptionPrunedCFG LOG(DEBUG, "DEBUG", "@ " + method.getSignature()); - Set derefedParamList = new HashSet(); prunedCFG = ExceptionPrunedCFG.make(cfg); // In case the only control flows are exceptional, simply return. if (prunedCFG.getNumberOfNodes() == 2 && prunedCFG.containsNode(cfg.entry()) && prunedCFG.containsNode(cfg.exit()) && GraphUtil.countEdges(prunedCFG) == 0) { - return derefedParamList; + return new HashSet<>(); } + + // Get number of params and value number of first param + int numParam = ir.getSymbolTable().getNumberOfParameters(); + int firstParamIndex = + method.isStatic() ? 1 : 2; // 1-indexed; v1 is 'this' for non-static methods + + Set derefedParamList; + if (USE_EXTENDED_APPROACH) { + derefedParamList = computeDerefParamList(numParam, firstParamIndex); + } else { + derefedParamList = computeDerefParamListUsingPDom(numParam, firstParamIndex); + } + return derefedParamList; + } + + @NotNull + private Set computeDerefParamList(int numParam, int firstParamIndex) { + Set derefedParamList = new HashSet<>(); + Map> blockToDerefSetMap = new HashMap<>(); + + // find which params are treated as being non-null inside each basic block + prunedCFG.forEach( + basicBlock -> { + Set derefParamSet = new HashSet<>(); + blockToDerefSetMap.put(basicBlock, derefParamSet); + checkForUseOfParams(derefParamSet, numParam, firstParamIndex, basicBlock); + }); + + // For each param p, if no path exists from the entry block to the exit block which traverses + // only basic blocks which do not require p being non-null (e.g. by dereferencing it), then + // mark p as @NonNull + for (int i = firstParamIndex; i <= numParam; i++) { + final Integer param = i - 1; + if (!DFS.getReachableNodes( + prunedCFG, + ImmutableList.of(prunedCFG.entry()), + basicBlock -> !blockToDerefSetMap.get(basicBlock).contains(param)) + .contains(prunedCFG.exit())) { + derefedParamList.add(param); + } + } + return derefedParamList; + } + + @NotNull + private Set computeDerefParamListUsingPDom(int numParam, int firstParamIndex) { + Set derefedParamList = new HashSet<>(); // Get Dominator Tree LOG(DEBUG, "DEBUG", "\tbuilding dominator tree..."); Graph domTree = Dominators.make(prunedCFG, prunedCFG.entry()).dominatorTree(); @@ -120,10 +172,6 @@ Set analyze() { LOG(DEBUG, "DEBUG", "\tfinding dereferenced params..."); ArrayList nodeQueue = new ArrayList(); nodeQueue.add(prunedCFG.exit()); - // Get number of params and value number of first param - int numParam = ir.getSymbolTable().getNumberOfParameters(); - int firstParamIndex = - method.isStatic() ? 1 : 2; // 1-indexed; v1 is 'this' for non-static methods LOG(DEBUG, "DEBUG", "param value numbers : " + firstParamIndex + " ... " + numParam); while (!nodeQueue.isEmpty()) { ISSABasicBlock node = nodeQueue.get(0); diff --git a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java index dba2eb854c..4ebf54da62 100644 --- a/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java +++ b/jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java @@ -79,12 +79,11 @@ private void testTemplate( Main.Result.OK, compileResult); DefinitelyDerefedParamsDriver driver = new DefinitelyDerefedParamsDriver(); + Map> result = + driver.run(temporaryFolder.getRoot().getAbsolutePath(), "L" + pkg.replaceAll("\\.", "/")); Assert.assertTrue( - testName + ": test failed!", - verify( - driver.run( - temporaryFolder.getRoot().getAbsolutePath(), "L" + pkg.replaceAll("\\.", "/")), - new HashMap<>(expected))); + testName + ": test failed! \n" + result + " does not match " + expected, + verify(result, new HashMap<>(expected))); } /** @@ -275,6 +274,101 @@ public void toyNullTestAPI() throws Exception { "}"); } + @Test + public void toyConditionalFlow() throws Exception { + testTemplate( + "toyNullTestAPI", + "toys", + "Foo", + ImmutableMap.of("toys.Foo:void test(String, String, String)", Sets.newHashSet(1, 2)), + "import com.google.common.base.Preconditions;", + "import java.util.Objects;", + "import org.junit.Assert;", + "class Foo {", + " private String foo;", + " public Foo(String str) {", + " if (str == null) str = \"foo\";", + " this.foo = str;", + " }", + " public void test(String s, String t, String u) {", + " if (s.length() >= 5) {", + " t.toString();", + " t = s;", + " } else {", + " Preconditions.checkNotNull(t);", + " u = t;", + " }", + " Objects.requireNonNull(u);", + " }", + "}"); + } + + @Test + public void toyConditionalFlow2() throws Exception { + testTemplate( + "toyNullTestAPI", + "toys", + "Foo", + ImmutableMap.of( + "toys.Foo:void test(Object, Object, Object, Object)", Sets.newHashSet(1, 4)), + "import com.google.common.base.Preconditions;", + "import java.util.Objects;", + "import org.junit.Assert;", + "class Foo {", + " private String foo;", + " public Foo(String str) {", + " if (str == null) str = \"foo\";", + " this.foo = str;", + " }", + " public void test(Object a, Object b, Object c, Object d) {", + " if (a != null) {", + " b.toString();", + " d.toString();", + " } else {", + " Preconditions.checkNotNull(c);", + " }", + " Objects.requireNonNull(a);", + " if (b != null) {", + " c.toString();", + " d.toString();", + " } else {", + " Preconditions.checkNotNull(b);", + " if (c != null) {", + " d.toString();", + " } else {", + " Preconditions.checkNotNull(d);", + " }", + " }", + " }", + "}"); + } + + @Test + public void toyReassigningTest() throws Exception { + testTemplate( + "toyNullTestAPI", + "toys", + "Foo", + ImmutableMap.of("toys.Foo:void test(String, String)", Sets.newHashSet(1)), + "import com.google.common.base.Preconditions;", + "import java.util.Objects;", + "import org.junit.Assert;", + "class Foo {", + " private String foo;", + " public Foo(String str) {", + " if (str == null) str = \"foo\";", + " this.foo = str;", + " }", + " public void test(String s, String t) {", + " Preconditions.checkNotNull(s);", + " if (t == null) {", + " t = s;", + " }", + " Objects.requireNonNull(t);", + " }", + "}"); + } + @Test public void toyJARAnnotatingClasses() throws Exception { testAnnotationInJarTemplate(