Add basic flow analysis support to lint
This changeset adds in the ASM analysis library (an optional part of
the ASM package lint is already using to process bytecode). It also
adds some basic flow analysis to the SecureRandom detector to detect
whether a given dispatch to a field of type java.util.Random is
actually pointing to a java.security.SecureRandom, in which case it
flags calls on it to setSeed() where the argument is a fixed integer.
Change-Id: If85ab9f8db0e801a01f1a3ea845865b4f98e259c
diff --git a/lint/cli/.classpath b/lint/cli/.classpath
index ff20329..b5483f0 100644
--- a/lint/cli/.classpath
+++ b/lint/cli/.classpath
@@ -7,6 +7,7 @@
<classpathentry combineaccessrules="false" kind="src" path="/lint-checks"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-tree-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
+ <classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-analysis-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/guava-tools/guava-10.0.1.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/guava-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/lombok-ast/lombok-ast-0.2.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/lombok-ast/src.zip"/>
<classpathentry kind="output" path="bin"/>
diff --git a/lint/cli/Android.mk b/lint/cli/Android.mk
index ce6c5ad..8a46d48 100644
--- a/lint/cli/Android.mk
+++ b/lint/cli/Android.mk
@@ -16,6 +16,7 @@
lombok-ast-0.2 \
asm-tools \
asm-tree-tools \
+ asm-analysis-tools \
guava-tools
LOCAL_MODULE := lint
diff --git a/lint/cli/etc/manifest.txt b/lint/cli/etc/manifest.txt
index 0cb09ef..2b7965a 100644
--- a/lint/cli/etc/manifest.txt
+++ b/lint/cli/etc/manifest.txt
@@ -1,2 +1,2 @@
Main-Class: com.android.tools.lint.Main
-Class-Path: common.jar lint_api.jar lint_checks.jar asm-4.0.jar asm-tree-4.0.jar guava-10.0.1.jar lombok-ast-0.2.jar
+Class-Path: common.jar lint_api.jar lint_checks.jar asm-4.0.jar asm-tree-4.0.jar asm-analysis-4.0.jar guava-10.0.1.jar lombok-ast-0.2.jar
diff --git a/lint/libs/lint_checks/.classpath b/lint/libs/lint_checks/.classpath
index 848a8aa..29f6e7c 100644
--- a/lint/libs/lint_checks/.classpath
+++ b/lint/libs/lint_checks/.classpath
@@ -7,6 +7,7 @@
<classpathentry combineaccessrules="false" kind="src" path="/AndroidPrefs"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-tree-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
+ <classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/asm-tools/asm-analysis-4.0.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/asm-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/guava-tools/guava-10.0.1.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/guava-tools/src.zip"/>
<classpathentry kind="var" path="ANDROID_SRC/prebuilts/tools/common/lombok-ast/lombok-ast-0.2.jar" sourcepath="/ANDROID_SRC/prebuilts/tools/common/lombok-ast/src.zip"/>
<classpathentry kind="output" path="bin"/>
diff --git a/lint/libs/lint_checks/Android.mk b/lint/libs/lint_checks/Android.mk
index a0342de..dcd440d 100644
--- a/lint/libs/lint_checks/Android.mk
+++ b/lint/libs/lint_checks/Android.mk
@@ -15,9 +15,10 @@
lombok-ast-0.2 \
androidprefs \
lint_api \
- asm-tools \
- asm-tree-tools \
- guava-tools
+ asm-tools \
+ asm-tree-tools \
+ asm-analysis-tools \
+ guava-tools
LOCAL_MODULE := lint_checks
LOCAL_MODULE_TAGS := optional
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/ControlFlowGraph.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ControlFlowGraph.java
new file mode 100644
index 0000000..435c3dc
--- /dev/null
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/ControlFlowGraph.java
@@ -0,0 +1,325 @@
+/*
+ * Copyright (C) 2012 The Android Open Source Project
+ *
+ * Licensed under the Eclipse Public License, Version 1.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.eclipse.org/org/documents/epl-v10.php
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.android.tools.lint.checks;
+
+import com.android.annotations.NonNull;
+import com.android.annotations.Nullable;
+import com.google.common.collect.Maps;
+
+import org.objectweb.asm.tree.AbstractInsnNode;
+import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.FrameNode;
+import org.objectweb.asm.tree.InsnList;
+import org.objectweb.asm.tree.LabelNode;
+import org.objectweb.asm.tree.LineNumberNode;
+import org.objectweb.asm.tree.MethodInsnNode;
+import org.objectweb.asm.tree.MethodNode;
+import org.objectweb.asm.tree.TryCatchBlockNode;
+import org.objectweb.asm.tree.analysis.Analyzer;
+import org.objectweb.asm.tree.analysis.AnalyzerException;
+import org.objectweb.asm.tree.analysis.BasicInterpreter;
+
+import java.lang.reflect.Field;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * A {@linkplain ControlFlowGraph} is a graph containing a node for each
+ * instruction in a method, and an edge for each possible control flow; usually
+ * just "next" for the instruction following the current instruction, but in the
+ * case of a branch such as an "if", multiple edges to each successive location,
+ * or with a "goto", a single edge to the jumped-to instruction.
+ * <p>
+ * It also adds edges for abnormal control flow, such as the possibility of a
+ * method call throwing a runtime exception.
+ */
+public class ControlFlowGraph {
+ /** Map from instructions to nodes */
+ private Map<AbstractInsnNode, Node> mNodeMap;
+
+ /**
+ * Creates a new {@link ControlFlowGraph} and populates it with the flow
+ * control for the given method. If the optional {@code initial} parameter is
+ * provided with an existing graph, then the graph is simply populated, not
+ * created. This allows subclassing of the graph instance, if necessary.
+ *
+ * @param initial usually null, but can point to an existing instance of a
+ * {@link ControlFlowGraph} in which that graph is reused (but
+ * populated with new edges)
+ * @param classNode the class containing the method to be analyzed
+ * @param method the method to be analyzed
+ * @return a {@link ControlFlowGraph} with nodes for the control flow in the
+ * given method
+ * @throws AnalyzerException if the underlying bytecode library is unable to
+ * analyze the method bytecode
+ */
+ @NonNull
+ public static ControlFlowGraph create(
+ @Nullable ControlFlowGraph initial,
+ @NonNull ClassNode classNode,
+ @NonNull MethodNode method) throws AnalyzerException {
+ final ControlFlowGraph graph = initial != null ? initial : new ControlFlowGraph();
+ final InsnList instructions = method.instructions;
+ graph.mNodeMap = Maps.newHashMapWithExpectedSize(instructions.size());
+
+ // Create a flow control graph using ASM4's analyzer. According to the ASM 4 guide
+ // (download.forge.objectweb.org/asm/asm4-guide.pdf) there are faster ways to construct
+ // it, but those require a lot more code.
+ Analyzer analyzer = new Analyzer(new BasicInterpreter()) {
+ @Override
+ protected void newControlFlowEdge(int insn, int successor) {
+ // Update the information as of whether the this object has been
+ // initialized at the given instruction.
+ AbstractInsnNode from = instructions.get(insn);
+ AbstractInsnNode to = instructions.get(successor);
+ graph.add(from, to);
+ }
+
+ @Override
+ protected boolean newControlFlowExceptionEdge(int insn, TryCatchBlockNode tcb) {
+ AbstractInsnNode from = instructions.get(insn);
+ graph.exception(from, tcb);
+ return super.newControlFlowExceptionEdge(insn, tcb);
+ }
+
+ @Override
+ protected boolean newControlFlowExceptionEdge(int insn, int successor) {
+ AbstractInsnNode from = instructions.get(insn);
+ AbstractInsnNode to = instructions.get(successor);
+ graph.exception(from, to);
+ return super.newControlFlowExceptionEdge(insn, successor);
+ }
+ };
+
+ analyzer.analyze(classNode.name, method);
+ return graph;
+ }
+
+ /** A {@link Node} is a node in the control flow graph for a method, pointing to
+ * the instruction and its possible successors */
+ public static class Node {
+ /** The instruction */
+ public final AbstractInsnNode instruction;
+ /** Any normal successors (e.g. following instruction, or goto or conditional flow) */
+ public final List<Node> successors = new ArrayList<Node>(2);
+ /** Any abnormal successors (e.g. the handler to go to following an exception) */
+ public final List<Node> exceptions = new ArrayList<Node>(1);
+
+ /** A tag for use during depth-first-search iteration of the graph etc */
+ public int visit;
+
+ /**
+ * Constructs a new control graph node
+ *
+ * @param instruction the instruction to associate with this node
+ */
+ public Node(@NonNull AbstractInsnNode instruction) {
+ this.instruction = instruction;
+ }
+
+ void addSuccessor(@NonNull Node node) {
+ if (!successors.contains(node)) {
+ successors.add(node);
+ }
+ }
+
+ void addExceptionPath(@NonNull Node node) {
+ if (!exceptions.contains(node)) {
+ exceptions.add(node);
+ }
+ }
+
+ /**
+ * Represents this instruction as a string, for debugging purposes
+ *
+ * @param includeAdjacent whether it should include a display of
+ * adjacent nodes as well
+ * @return a string representation
+ */
+ @NonNull
+ public String toString(boolean includeAdjacent) {
+ StringBuilder sb = new StringBuilder();
+
+ sb.append(getId(instruction));
+ sb.append(':');
+
+ if (instruction instanceof LabelNode) {
+ //LabelNode l = (LabelNode) instruction;
+ //sb.append('L' + l.getLabel().getOffset() + ":");
+ //sb.append('L' + l.getLabel().info + ":");
+ sb.append("LABEL");
+ } else if (instruction instanceof LineNumberNode) {
+ sb.append("LINENUMBER " + ((LineNumberNode) instruction).line);
+ } else if (instruction instanceof FrameNode) {
+ sb.append("FRAME");
+ } else {
+ int opcode = instruction.getOpcode();
+ // AbstractVisitor isn't available unless debug/util is included,
+ boolean printed = false;
+ try {
+ Class<?> c = Class.forName("org.objectweb.asm.util"); //$NON-NLS-1$
+ Field field = c.getField("OPCODES");
+ String[] OPCODES = (String[]) field.get(null);
+ printed = true;
+ if (opcode > 0 && opcode <= OPCODES.length) {
+ sb.append(OPCODES[opcode]);
+ if (instruction.getType() == AbstractInsnNode.METHOD_INSN) {
+ sb.append("(" + ((MethodInsnNode)instruction).name + ")");
+ }
+ }
+ } catch (Throwable t) {
+ // debug not installed: just do toString() on the instructions
+ }
+ if (!printed) {
+ sb.append(instruction.toString());
+ }
+ }
+
+ if (includeAdjacent) {
+ if (successors != null && !successors.isEmpty()) {
+ sb.append(" Next:");
+ for (Node s : successors) {
+ sb.append(' ');
+ sb.append(s.toString(false));
+ }
+ }
+
+ if (exceptions != null && !exceptions.isEmpty()) {
+ sb.append(" Exceptions:");
+ for (Node s : exceptions) {
+ sb.append(' ');
+ sb.append(s.toString(false));
+ }
+ }
+ sb.append('\n');
+ }
+
+ return sb.toString();
+ }
+ }
+
+ /** Adds an exception flow to this graph */
+ protected void add(@NonNull AbstractInsnNode from, @NonNull AbstractInsnNode to) {
+ getNode(from).addSuccessor(getNode(to));
+ }
+
+ /** Adds an exception flow to this graph */
+ protected void exception(@NonNull AbstractInsnNode from, @NonNull AbstractInsnNode to) {
+ // For now, these edges appear useless; we also get more specific
+ // information via the TryCatchBlockNode which we use instead.
+ //getNode(from).addExceptionPath(getNode(to));
+ }
+
+ /** Adds an exception try block node to this graph */
+ protected void exception(@NonNull AbstractInsnNode from, @NonNull TryCatchBlockNode tcb) {
+ // Add tcb's to all instructions in the range
+ LabelNode start = tcb.start;
+ LabelNode end = tcb.end; // exclusive
+
+ // Add exception edges for all method calls in the range
+ AbstractInsnNode curr = start;
+ Node handlerNode = getNode(tcb.handler);
+ while (curr != end && curr != null) {
+ if (curr.getType() == AbstractInsnNode.METHOD_INSN) {
+ // Method call; add exception edge to handler
+ if (tcb.type == null) {
+ // finally block: not an exception path
+ getNode(curr).addSuccessor(handlerNode);
+ } else {
+ getNode(curr).addExceptionPath(handlerNode);
+ }
+ }
+ curr = curr.getNext();
+ }
+ }
+
+ /**
+ * Looks up (and if necessary) creates a graph node for the given instruction
+ *
+ * @param instruction the instruction
+ * @return the control flow graph node corresponding to the given
+ * instruction
+ */
+ @NonNull
+ public Node getNode(@NonNull AbstractInsnNode instruction) {
+ Node node = mNodeMap.get(instruction);
+ if (node == null) {
+ node = new Node(instruction);
+ mNodeMap.put(instruction, node);
+ }
+
+ return node;
+ }
+
+ /**
+ * Creates a human readable version of the graph
+ *
+ * @param start the starting instruction, or null if not known or to use the
+ * first instruction
+ * @return a string version of the graph
+ */
+ @NonNull
+ public String toString(@Nullable Node start) {
+ StringBuilder sb = new StringBuilder();
+
+ AbstractInsnNode curr = null;
+ if (start != null) {
+ curr = start.instruction;
+ } else {
+ if (mNodeMap.isEmpty()) {
+ return "<empty>";
+ } else {
+ curr = mNodeMap.keySet().iterator().next();
+ while (curr.getPrevious() != null) {
+ curr = curr.getPrevious();
+ }
+ }
+ }
+
+ while (curr != null) {
+ Node n = mNodeMap.get(curr);
+ if (n != null) {
+ sb.append(n.toString(true));
+ }
+ curr = curr.getNext();
+ }
+
+ return sb.toString();
+ }
+
+ @Override
+ public String toString() {
+ return toString(null);
+ }
+
+ // ---- For debugging only ----
+
+ private static Map<Object, String> sIds = null;
+ private static int sNextId = 1;
+ private static String getId(Object object) {
+ if (sIds == null) {
+ sIds = Maps.newHashMap();
+ }
+ String id = sIds.get(object);
+ if (id == null) {
+ id = Integer.toString(sNextId++);
+ sIds.put(object, id);
+ }
+ return id;
+ }
+}
+
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecureRandomDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecureRandomDetector.java
index 79f5d47..aee8468 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecureRandomDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/SecureRandomDetector.java
@@ -28,10 +28,17 @@
import com.android.tools.lint.detector.api.Severity;
import com.sun.xml.internal.ws.org.objectweb.asm.Opcodes;
+import org.objectweb.asm.Type;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.InsnList;
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
+import org.objectweb.asm.tree.analysis.Analyzer;
+import org.objectweb.asm.tree.analysis.AnalyzerException;
+import org.objectweb.asm.tree.analysis.BasicInterpreter;
+import org.objectweb.asm.tree.analysis.BasicValue;
+import org.objectweb.asm.tree.analysis.Frame;
import java.util.Arrays;
import java.util.List;
@@ -58,6 +65,7 @@
private static final String SET_SEED = "setSeed"; //$NON-NLS-1$
private static final String OWNER_SECURE_RANDOM = "java/security/SecureRandom"; //$NON-NLS-1$
private static final String OWNER_RANDOM = "java/util/Random"; //$NON-NLS-1$
+ private static final String VM_SECURE_RANDOM = 'L' + OWNER_SECURE_RANDOM + ';';
/** Method description for a method that takes a long argument (no return type specified */
private static final String LONG_ARG = "(J)"; //$NON-NLS-1$
@@ -91,6 +99,37 @@
} else if (owner.equals(OWNER_RANDOM) && desc.startsWith(LONG_ARG)) {
// Called setSeed(long) on an instanceof a Random object. Flag this if the instance
// is likely a SecureRandom.
+
+ // Track allocations such that we know whether the type of the call
+ // is on a SecureRandom rather than a Random
+ Analyzer analyzer = new Analyzer(new BasicInterpreter() {
+ @Override
+ public BasicValue newValue(Type type) {
+ if (type != null && type.getDescriptor().equals(VM_SECURE_RANDOM)) {
+ return new BasicValue(type);
+ }
+ return super.newValue(type);
+ }
+ });
+ try {
+ Frame[] frames = analyzer.analyze(classNode.name, method);
+ InsnList instructions = method.instructions;
+ Frame frame = frames[instructions.indexOf(call)];
+ int stackSlot = frame.getStackSize();
+ for (Type type : Type.getArgumentTypes(desc)) {
+ stackSlot -= type.getSize();
+ }
+ BasicValue stackValue = (BasicValue) frame.getStack(stackSlot);
+ Type type = stackValue.getType();
+ if (type != null && type.getDescriptor().equals(VM_SECURE_RANDOM)) {
+ checkValidSetSeed(context, call);
+ }
+ } catch (AnalyzerException e) {
+ context.log(e, null);
+ }
+ } else if (owner.equals(OWNER_RANDOM) && desc.startsWith(LONG_ARG)) {
+ // Called setSeed(long) on an instanceof a Random object. Flag this if the instance
+ // is likely a SecureRandom.
// TODO
}
}
diff --git a/lint/libs/lint_checks/src/com/android/tools/lint/checks/WakelockDetector.java b/lint/libs/lint_checks/src/com/android/tools/lint/checks/WakelockDetector.java
index 7ed0edf..c828eba 100644
--- a/lint/libs/lint_checks/src/com/android/tools/lint/checks/WakelockDetector.java
+++ b/lint/libs/lint_checks/src/com/android/tools/lint/checks/WakelockDetector.java
@@ -19,18 +19,25 @@
import com.android.annotations.NonNull;
import com.android.annotations.Nullable;
+import com.android.tools.lint.checks.ControlFlowGraph.Node;
import com.android.tools.lint.detector.api.Category;
import com.android.tools.lint.detector.api.ClassContext;
import com.android.tools.lint.detector.api.Context;
import com.android.tools.lint.detector.api.Detector;
import com.android.tools.lint.detector.api.Detector.ClassScanner;
import com.android.tools.lint.detector.api.Issue;
+import com.android.tools.lint.detector.api.LintUtils;
import com.android.tools.lint.detector.api.Scope;
import com.android.tools.lint.detector.api.Severity;
+import org.objectweb.asm.Opcodes;
+import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.ClassNode;
+import org.objectweb.asm.tree.InsnList;
+import org.objectweb.asm.tree.JumpInsnNode;
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
+import org.objectweb.asm.tree.analysis.AnalyzerException;
import java.util.Arrays;
import java.util.List;
@@ -68,6 +75,12 @@
private static final String RELEASE_METHOD = "release"; //$NON-NLS-1$
private static final String ACQUIRE_METHOD = "acquire"; //$NON-NLS-1$
+ /** Print diagnostics during analysis (display flow control graph etc).
+ * Make sure you add the asm-debug or asm-util jars to the runtime classpath
+ * as well since the opcode integer to string mapping display routine looks for
+ * it via reflection. */
+ private static boolean DEBUG = false;
+
/** Constructs a new {@link WakelockDetector} */
public WakelockDetector() {
}
@@ -113,7 +126,7 @@
// performing an acquire/release block, where there are code paths
// between the acquire and release which can result in the
// release call not getting reached.
- // TODO: Implement this.
+ checkFlow(context, classNode, method, call);
}
} else if (name.equals(RELEASE_METHOD)) {
mHasRelease = true;
@@ -130,4 +143,229 @@
}
}
}
+
+ private void checkFlow(@NonNull ClassContext context, @NonNull ClassNode classNode,
+ @NonNull MethodNode method, @NonNull MethodInsnNode acquire) {
+ // Track allocations such that we know whether the type of the call
+ // is on a SecureRandom rather than a Random
+ final InsnList instructions = method.instructions;
+ MethodInsnNode release = null;
+
+ // Find release call
+ for (int i = 0, n = instructions.size(); i < n; i++) {
+ AbstractInsnNode instruction = instructions.get(i);
+ int type = instruction.getType();
+ if (type == AbstractInsnNode.METHOD_INSN) {
+ MethodInsnNode call = (MethodInsnNode) instruction;
+ if (call.name.equals(RELEASE_METHOD) &&
+ call.owner.equals(WAKELOCK_OWNER)) {
+ release = call;
+ break;
+ }
+ }
+ }
+
+ if (release == null) {
+ // Didn't find both acquire and release in this method; no point in doing
+ // local flow analysis
+ return;
+ }
+
+ try {
+ MyGraph graph = new MyGraph();
+ ControlFlowGraph.create(graph, classNode, method);
+
+ if (DEBUG) {
+ // Requires util package
+ //ClassNode clazz = classNode;
+ //clazz.accept(new TraceClassVisitor(new PrintWriter(System.out)));
+ System.out.println(graph.toString(graph.getNode(acquire)));;
+ }
+
+ int status = dfs(graph.getNode(acquire));
+ if ((status & SEEN_RETURN) != 0) {
+ String message;
+ if ((status & SEEN_EXCEPTION) != 0) {
+ message = "The release() call is not always reached (via exceptional flow)";
+ } else {
+ message = "The release() call is not always reached";
+ }
+
+ context.report(ISSUE, method, context.getLocation(release), message, null);
+ }
+ } catch (AnalyzerException e) {
+ context.log(e, null);
+ }
+ }
+
+ private static int SEEN_TARGET = 1;
+ private static int SEEN_BRANCH = 2;
+ private static int SEEN_EXCEPTION = 4;
+ private static int SEEN_RETURN = 8;
+
+ /** TODO RENAME */
+ private static class MyGraph extends ControlFlowGraph {
+ @Override
+ protected void add(@NonNull AbstractInsnNode from, @NonNull AbstractInsnNode to) {
+ if (from.getOpcode() == Opcodes.IFNULL) {
+ JumpInsnNode jump = (JumpInsnNode) from;
+ if (jump.label == to) {
+ // Skip jump targets on null if it's surrounding the release call
+ //
+ // if (lock != null) {
+ // lock.release();
+ // }
+ //
+ // The above shouldn't be considered a scenario where release() may not
+ // be called.
+ AbstractInsnNode next = LintUtils.getNextInstruction(from);
+ if (next != null && next.getType() == AbstractInsnNode.VAR_INSN) {
+ next = LintUtils.getNextInstruction(next);
+ if (next != null && next.getType() == AbstractInsnNode.METHOD_INSN) {
+ MethodInsnNode method = (MethodInsnNode) next;
+ if (method.name.equals(RELEASE_METHOD) &&
+ method.owner.equals(WAKELOCK_OWNER)) {
+ // This isn't entirely correct; this will also trigger
+ // for "if (lock == null) { lock.release(); }" but that's
+ // not likely (and caught by other null checking in tools)
+ return;
+ }
+ }
+ }
+ }
+ }
+
+ super.add(from, to);
+ }
+ }
+
+ /** Search from the given node towards the target; return false if we reach
+ * an exit point such as a return or a call on the way there that is not within
+ * a try/catch clause.
+ *
+ * @param node the current node
+ * @return true if the target was reached
+ * XXX RETURN VALUES ARE WRONG AS OF RIGHT NOW
+ */
+ protected int dfs(ControlFlowGraph.Node node) {
+ AbstractInsnNode instruction = node.instruction;
+ if (instruction.getType() == AbstractInsnNode.JUMP_INSN) {
+ int opcode = instruction.getOpcode();
+ if (opcode == Opcodes.RETURN || opcode == Opcodes.ARETURN
+ || opcode == Opcodes.LRETURN || opcode == Opcodes.IRETURN
+ || opcode == Opcodes.DRETURN || opcode == Opcodes.FRETURN
+ || opcode == Opcodes.ATHROW) {
+ if (DEBUG) {
+ System.out.println("Found exit via explicit return: " //$NON-NLS-1$
+ + node.toString(false));
+ }
+ return SEEN_RETURN;
+ }
+ }
+
+ if (!DEBUG) {
+ // There are no cycles, so no *NEED* for this, though it does avoid
+ // researching shared labels. However, it makes debugging harder (no re-entry)
+ // so this is only done when debugging is off
+ if (node.visit != 0) {
+ return 0;
+ }
+ node.visit = 1;
+ }
+
+ // Look for the target. This is any method call node which is a release on the
+ // lock (later also check it's the same instance, though that's harder).
+ // This is because finally blocks tend to be inlined so from a single try/catch/finally
+ // with a release() in the finally, the bytecode can contain multiple repeated
+ // (inlined) release() calls.
+ if (instruction.getType() == AbstractInsnNode.METHOD_INSN) {
+ MethodInsnNode method = (MethodInsnNode) instruction;
+ if (method.name.equals(RELEASE_METHOD) && method.owner.equals(WAKELOCK_OWNER)) {
+ return SEEN_TARGET;
+ } else if (method.name.equals(ACQUIRE_METHOD) && method.owner.equals(WAKELOCK_OWNER)) {
+ // OK
+ } else {
+ // Some non acquire/release method call: if this is not associated with a
+ // try-catch block, it would mean the exception would exit the method,
+ // which would be an error
+ if (node.exceptions == null || node.exceptions.isEmpty()) {
+ // Look up the corresponding frame, if any
+ AbstractInsnNode curr = method.getPrevious();
+ boolean foundFrame = false;
+ while (curr != null) {
+ if (curr.getType() == AbstractInsnNode.FRAME) {
+ foundFrame = true;
+ break;
+ }
+ curr = curr.getPrevious();
+ }
+
+ if (!foundFrame) {
+ if (DEBUG) {
+ System.out.println("Found exit via unguarded method call: " //$NON-NLS-1$
+ + node.toString(false));
+ }
+ return SEEN_RETURN;
+ }
+ }
+ }
+ }
+
+ // if (node.instruction is a call, and the call is not caught by
+ // a try/catch block (provided the release is not inside the try/catch block)
+ // then return false
+ int status = 0;
+
+ boolean implicitReturn = true;
+ List<Node> successors = node.successors;
+ List<Node> exceptions = node.exceptions;
+ if (exceptions != null) {
+ if (!exceptions.isEmpty()) {
+ implicitReturn = false;
+ }
+ for (Node successor : exceptions) {
+ status = dfs(successor) | status;
+ if ((status & SEEN_RETURN) != 0) {
+ if (DEBUG) {
+ System.out.println("Found exit via exception: " //$NON-NLS-1$
+ + node.toString(false));
+ }
+ return status;
+ }
+ }
+
+ if (status != 0) {
+ status |= SEEN_EXCEPTION;
+ }
+ }
+
+ if (successors != null) {
+ if (!successors.isEmpty()) {
+ implicitReturn = false;
+ if (successors.size() > 1) {
+ status |= SEEN_BRANCH;
+ }
+ }
+ for (Node successor : successors) {
+ status = dfs(successor) | status;
+ if ((status & SEEN_RETURN) != 0) {
+ if (DEBUG) {
+ System.out.println("Found exit via branches: " //$NON-NLS-1$
+ + node.toString(false));
+ }
+ return status;
+ }
+ }
+ }
+
+ if (implicitReturn) {
+ status |= SEEN_RETURN;
+ if (DEBUG) {
+ System.out.println("Found exit: via implicit return: " //$NON-NLS-1$
+ + node.toString(false));
+ }
+ }
+
+ return status;
+ }
}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecureRandomDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecureRandomDetectorTest.java
index 2fbdaad..67fc3c1 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecureRandomDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/SecureRandomDetectorTest.java
@@ -32,11 +32,10 @@
"SecureRandomTest.java:15: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().\n" +
"SecureRandomTest.java:16: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().\n" +
"SecureRandomTest.java:17: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().\n" +
- "SecureRandomTest.java:18: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().",
- // Missing errors on lines 28 and 40, using flow analysis to determine that
- // setSeed on a random instance is really a call on a SecureRandom instance,
- // and using flow analysis to determine that the seed byte array passed into
- // the SecureRandom constructor is static.
+ "SecureRandomTest.java:18: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().\n" +
+ "SecureRandomTest.java:28: Warning: Do not call setSeed() on a SecureRandom with a fixed seed: it is not secure. Use getSeed().",
+ // Missing error on line 40, using flow analysis to determine that the seed byte
+ // array passed into the SecureRandom constructor is static.
lintProject(
"bytecode/.classpath=>.classpath",
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/WakelockDetectorTest.java b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/WakelockDetectorTest.java
index d0662cb..0b39eb6 100644
--- a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/WakelockDetectorTest.java
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/WakelockDetectorTest.java
@@ -50,4 +50,59 @@
"bytecode/WakelockActivity2.class.data=>bin/classes/test/pkg/WakelockActivity2.class"
));
}
+
+ public void test3() throws Exception {
+ assertEquals(
+ "WakelockActivity3.java:13: Warning: The release() call is not always reached",
+
+ lintProject(
+ "bytecode/.classpath=>.classpath",
+ "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+ "res/layout/onclick.xml=>res/layout/onclick.xml",
+ "bytecode/WakelockActivity3.java.txt=>src/test/pkg/WakelockActivity3.java",
+ "bytecode/WakelockActivity3.class.data=>bin/classes/test/pkg/WakelockActivity3.class"
+ ));
+ }
+
+ public void test4() throws Exception {
+ assertEquals(
+ "WakelockActivity4.java:10: Warning: The release() call is not always reached",
+
+ lintProject(
+ "bytecode/.classpath=>.classpath",
+ "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+ "res/layout/onclick.xml=>res/layout/onclick.xml",
+ "bytecode/WakelockActivity4.java.txt=>src/test/pkg/WakelockActivity4.java",
+ "bytecode/WakelockActivity4.class.data=>bin/classes/test/pkg/WakelockActivity4.class"
+ ));
+ }
+
+ public void test5() throws Exception {
+ assertEquals(
+ // Missing 13
+ "WakelockActivity5.java:13: Warning: The release() call is not always reached",
+
+ lintProject(
+ "bytecode/.classpath=>.classpath",
+ "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+ "res/layout/onclick.xml=>res/layout/onclick.xml",
+ "bytecode/WakelockActivity5.java.txt=>src/test/pkg/WakelockActivity5.java",
+ "bytecode/WakelockActivity5.class.data=>bin/classes/test/pkg/WakelockActivity5.class"
+ ));
+ }
+
+ public void test6() throws Exception {
+ assertEquals(
+ "WakelockActivity6.java:19: Warning: The release() call is not always reached\n" +
+ "WakelockActivity6.java:28: Warning: The release() call is not always reached\n" +
+ "WakelockActivity6.java:65: Warning: The release() call is not always reached",
+
+ lintProject(
+ "bytecode/.classpath=>.classpath",
+ "bytecode/AndroidManifest.xml=>AndroidManifest.xml",
+ "res/layout/onclick.xml=>res/layout/onclick.xml",
+ "bytecode/WakelockActivity6.java.txt=>src/test/pkg/WakelockActivity6.java",
+ "bytecode/WakelockActivity6.class.data=>bin/classes/test/pkg/WakelockActivity6.class"
+ ));
+ }
}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.class.data
new file mode 100644
index 0000000..b430519
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.class.data
Binary files differ
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.java.txt
new file mode 100644
index 0000000..8a842eb
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity3.java.txt
@@ -0,0 +1,19 @@
+package test.pkg;
+
+import android.app.Activity;
+import android.os.PowerManager;
+
+public class WakelockActivity3 extends Activity {
+ void wrongFlow() {
+ PowerManager manager = (PowerManager) getSystemService(POWER_SERVICE);
+ PowerManager.WakeLock lock =
+ manager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "Test");
+ lock.acquire();
+ randomCall();
+ lock.release(); // Should be in finally block
+ }
+
+ static void randomCall() {
+ System.out.println("test");
+ }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.class.data
new file mode 100644
index 0000000..8905203
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.class.data
Binary files differ
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.java.txt
new file mode 100644
index 0000000..9d6331f
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity4.java.txt
@@ -0,0 +1,27 @@
+package test.pkg;
+
+import android.app.Activity;
+import android.os.PowerManager;
+
+public class WakelockActivity4 extends Activity {
+ void wrongFlow2() {
+ getLock().acquire();
+ randomCall();
+ getLock().release(); // Should be in finally block
+ }
+
+ private PowerManager.WakeLock mLock;
+
+ PowerManager.WakeLock getLock() {
+ if (mLock == null) {
+ PowerManager manager = (PowerManager) getSystemService(POWER_SERVICE);
+ mLock = manager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "Test");
+ }
+
+ return mLock;
+ }
+
+ static void randomCall() {
+ System.out.println("test");
+ }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.class.data
new file mode 100644
index 0000000..9eca365
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.class.data
Binary files differ
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.java.txt
new file mode 100644
index 0000000..060f2b1
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity5.java.txt
@@ -0,0 +1,19 @@
+package test.pkg;
+
+import android.app.Activity;
+import android.os.PowerManager;
+
+public class WakelockActivity5 extends Activity {
+ void wrongFlow() {
+ PowerManager manager = (PowerManager) getSystemService(POWER_SERVICE);
+ PowerManager.WakeLock lock =
+ manager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "Test");
+ lock.acquire();
+ randomCall();
+ lock.release(); // Should be in finally block
+ }
+
+ static void randomCall() {
+ System.out.println("test");
+ }
+}
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.class.data b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.class.data
new file mode 100644
index 0000000..51fdf69
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.class.data
Binary files differ
diff --git a/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.java.txt b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.java.txt
new file mode 100644
index 0000000..a122fa3
--- /dev/null
+++ b/lint/libs/lint_checks/tests/src/com/android/tools/lint/checks/data/bytecode/WakelockActivity6.java.txt
@@ -0,0 +1,71 @@
+package test.pkg;
+
+import com.example.test3.BuildConfig;
+
+import android.annotation.SuppressLint;
+import android.app.Activity;
+import android.os.PowerManager;
+import android.os.PowerManager.WakeLock;;
+
+public class WakelockActivity6 extends Activity {
+ void wrongFlow1() {
+ PowerManager manager = (PowerManager) getSystemService(POWER_SERVICE);
+ PowerManager.WakeLock lock =
+ manager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, "Test");
+ lock.acquire();
+ if (getTaskId() == 50) {
+ randomCall();
+ } else {
+ lock.release(); // Wrong
+ }
+ }
+
+ void wrongFlow2(PowerManager.WakeLock lock) {
+ lock.acquire();
+ if (getTaskId() == 50) {
+ randomCall();
+ } else {
+ lock.release(); // Wrong
+ }
+ }
+
+ void okFlow1(WakeLock lock) {
+ lock.acquire();
+ try {
+ randomCall();
+ } catch (Exception e) {
+ e.printStackTrace();
+ } finally {
+ lock.release(); // OK
+ }
+ }
+
+ public void checkNullGuard(WakeLock lock) {
+ lock.acquire();
+ if (lock != null) {
+ lock.release(); // OK
+ }
+ }
+
+ @SuppressLint("Wakelock")
+ public void checkDisabled1(PowerManager.WakeLock lock) {
+ lock.acquire();
+ randomCall();
+ lock.release(); // Wrong, but disabled
+ }
+
+ void wrongFlow3(WakeLock lock) {
+ int id = getTaskId();
+ lock.acquire();
+ if (id < 50) {
+ System.out.println(1);
+ } else {
+ System.out.println(2);
+ }
+ lock.release(); // Wrong
+ }
+
+ static void randomCall() {
+ System.out.println("test");
+ }
+}