Skip to content

Commit 51c47ae

Browse files
jsotuyodjsotuyod
committed
Improve how we navigate the stack
 - We are now much better at getting fields used for instance detector. - We no longer break when using non-constant keys, but report it.
1 parent c1062c2 commit 51c47ae

File tree

7 files changed

+228
-85
lines changed

7 files changed

+228
-85
lines changed

‎pom.xml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@
5656
</distributionManagement>
5757

5858
<properties>
59-
<android-version>24.4.0-beta1</android-version>
59+
<android-version>25.1.0</android-version>
6060
<fingbugs-annotations-version>3.0.0</fingbugs-annotations-version>
6161
<asm-all-version>5.0.3</asm-all-version>
6262
<guava-version>17.0</guava-version>

‎src/main/java/com/monits/linters/FactoryMethodDetector.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
import com.android.tools.lint.detector.api.Issue;
3232
import com.android.tools.lint.detector.api.Scope;
3333
import com.android.tools.lint.detector.api.Severity;
34-
import com.google.common.collect.Sets;
34+
import com.google.common.collect.ImmutableSet;
3535

3636
public class FactoryMethodDetector extends Detector implements Detector.ClassScanner {
3737

@@ -50,7 +50,7 @@ public class FactoryMethodDetector extends Detector implements Detector.ClassSca
5050
new Implementation(FactoryMethodDetector.class, Scope.CLASS_FILE_SCOPE));
5151

5252
private static final Set<String> FRAGMENT_TYPE =
53-
Sets.newHashSet("android/support/v4/app/Fragment", "android/app/Fragment");
53+
ImmutableSet.of("android/support/v4/app/Fragment", "android/app/Fragment");
5454

5555
@Override
5656
@Nullable

‎src/main/java/com/monits/linters/InstanceStateDetector.java

Lines changed: 133 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Map.Entry;
2424
import java.util.Set;
2525

26+
import javax.annotation.CheckForNull;
2627
import javax.annotation.Nonnull;
2728
import javax.annotation.Nullable;
2829

@@ -47,7 +48,7 @@
4748
import com.android.tools.lint.detector.api.Issue;
4849
import com.android.tools.lint.detector.api.Scope;
4950
import com.android.tools.lint.detector.api.Severity;
50-
import com.google.common.collect.Sets;
51+
import com.google.common.collect.ImmutableSet;
5152
import com.monits.linters.instancestate.holder.InstanceStateHolder;
5253

5354
/**
@@ -66,6 +67,7 @@ public class InstanceStateDetector extends Detector implements Detector.ClassSca
6667
"The method %s expected a %s type, but the field %s is a %s type";
6768
public static final String RESTORED_WITH_DIFERENT_TYPES =
6869
"The field %s is a %s type, but the method %s is returning a %s type";
70+
public static final String NON_CONSTANT_KEY = "The key being used to access the bundle is non-constant";
6971

7072
public static final Issue MISSING_SAVED_INSTANCE_STATES = Issue.create("missingSavedOrRestoredInstanceState",
7173
"Missing saved or restored instance states",
@@ -90,16 +92,23 @@ public class InstanceStateDetector extends Detector implements Detector.ClassSca
9092
"Check if the type saved or restored is valid",
9193
Category.CORRECTNESS, 6, Severity.ERROR,
9294
new Implementation(InstanceStateDetector.class, Scope.CLASS_FILE_SCOPE));
95+
96+
public static final Issue KEY_IS_NOT_CONSTANT = Issue.create("instanceStateKeyIsNotConstant",
97+
"Key used to access bundle data is not constant",
98+
"Non constant keys may lead to inconsistent data persistance / recovery",
99+
Category.CORRECTNESS, 6, Severity.WARNING,
100+
new Implementation(InstanceStateDetector.class, Scope.CLASS_FILE_SCOPE));
93101

94-
private static final Set<String> METHOD_SAVE_INSTANCES = Sets.newHashSet("onSaveInstanceState");
102+
103+
private static final Set<String> METHOD_SAVE_INSTANCES = ImmutableSet.of("onSaveInstanceState");
95104
private static final Set<String> METHOD_RESTORE_INSTANCES =
96-
Sets.newHashSet(
97-
// Common methods
98-
"onCreate",
99-
// Activity methods
100-
"onPostCreate", "onRestoreInstanceState",
101-
// Fragment methods
102-
"onActivityCreated", "onCreateView", "onViewCreated");
105+
ImmutableSet.of(
106+
// Common methods
107+
"onCreate",
108+
// Activity methods
109+
"onPostCreate", "onRestoreInstanceState",
110+
// Fragment methods
111+
"onActivityCreated", "onCreateView", "onViewCreated");
103112

104113
private static final String ANDROID_BUNDLE_PATH = "android/os/Bundle";
105114

@@ -161,23 +170,24 @@ private Set<MethodNode> checkMethodCall(@Nonnull final ClassContext context, @No
161170

162171
final AbstractInsnNode[] instructions = methodToIterate.instructions.toArray();
163172
for (final AbstractInsnNode abstractInsnNode : instructions) {
164-
if (abstractInsnNode instanceof MethodInsnNode
173+
if (abstractInsnNode instanceof MethodInsnNode) {
174+
final MethodInsnNode methodNode = (MethodInsnNode) abstractInsnNode;
165175
// Ignore same method (ej super.onCreate(...))
166-
&& !((MethodInsnNode) abstractInsnNode).name.equals(call.name)) {
167-
final String descriptor = ((MethodInsnNode) abstractInsnNode).desc;
168-
// check if the parameter has a bundle paramenter
169-
if (descriptor.substring(descriptor.indexOf('(') + 1, descriptor.indexOf(')'))
170-
.contains("Landroid/os/Bundle;")) {
171-
for (final MethodNode element : (List<MethodNode>) classNode.methods) {
172-
// find the methodNode
173-
if (element instanceof MethodNode
174-
&& element.name.equals(((MethodInsnNode) abstractInsnNode).name)) {
175-
methods.add(element);
176+
if (!methodNode.name.equals(call.name)) {
177+
final String descriptor = methodNode.desc;
178+
// check if the parameter has a bundle parameter
179+
if (descriptor.substring(descriptor.indexOf('(') + 1, descriptor.indexOf(')'))
180+
.contains("Landroid/os/Bundle;")) {
181+
for (final MethodNode element : (List<MethodNode>) classNode.methods) {
182+
// find the methodNode
183+
if (element instanceof MethodNode && element.name.equals(methodNode.name)) {
184+
methods.add(element);
185+
}
176186
}
187+
} else {
188+
//we have something that we can pass to checkInstruction
189+
checkInstruction(context, classNode, methodToIterate, saveRestoreMethod, methodNode);
177190
}
178-
} else {
179-
//we have something that we can pass to checkInstruction
180-
checkInstruction(context, classNode, methodToIterate, saveRestoreMethod, abstractInsnNode);
181191
}
182192
}
183193
}
@@ -195,9 +205,9 @@ private Set<MethodNode> checkMethodCall(@Nonnull final ClassContext context, @No
195205
*/
196206
public void checkInstruction(@Nonnull final ClassContext context, @Nonnull final ClassNode classNode,
197207
@Nonnull final MethodNode currentMethod, @Nonnull final MethodNode originaryMethod,
198-
@Nonnull final AbstractInsnNode instruction) {
208+
@Nonnull final MethodInsnNode instruction) {
199209

200-
if (!ANDROID_BUNDLE_PATH.equals(((MethodInsnNode) instruction).owner)
210+
if (!ANDROID_BUNDLE_PATH.equals(instruction.owner)
201211
|| instruction.getOpcode() != Opcodes.INVOKEVIRTUAL) {
202212
return;
203213
}
@@ -207,13 +217,13 @@ public void checkInstruction(@Nonnull final ClassContext context, @Nonnull final
207217
}
208218

209219
// ignore those method that no have params
210-
final String descriptor = ((MethodInsnNode) instruction).desc;
220+
final String descriptor = instruction.desc;
211221
if (descriptor.substring(descriptor.indexOf('(') + 1, descriptor.indexOf(')')).isEmpty()) {
212222
return;
213223
}
214224

215225
// Ignore containsKey method
216-
if ("containsKey".equals(((MethodInsnNode) instruction).name)) {
226+
if ("containsKey".equals(instruction.name)) {
217227
return;
218228
}
219229

@@ -223,16 +233,28 @@ public void checkInstruction(@Nonnull final ClassContext context, @Nonnull final
223233
}
224234

225235
if (METHOD_SAVE_INSTANCES.contains(originaryMethod.name)) {
226-
final String bundleKey = getBundleKey(instruction);
227-
if (savedStates.containsKey(bundleKey)) {
228-
context.report(OVERWRITING_INSTANCE_STATES, currentMethod, instruction,
229-
context.getLocation(instruction), String.format(ALREADY_SAVED, bundleKey));
236+
// TODO : Check call is actually to a save method!
237+
final String bundleKey = getBundleKeyForMethodCall(instruction);
238+
if (bundleKey == null) {
239+
context.report(KEY_IS_NOT_CONSTANT, currentMethod, instruction,
240+
context.getLocation(instruction), NON_CONSTANT_KEY);
230241
} else {
231-
savedStates.put(bundleKey, new InstanceStateHolder(instruction, currentMethod));
242+
if (savedStates.containsKey(bundleKey)) {
243+
context.report(OVERWRITING_INSTANCE_STATES, currentMethod, instruction,
244+
context.getLocation(instruction), String.format(ALREADY_SAVED, bundleKey));
245+
} else {
246+
savedStates.put(bundleKey, new InstanceStateHolder(instruction, currentMethod));
247+
}
232248
}
233249
} else if (METHOD_RESTORE_INSTANCES.contains(originaryMethod.name)) {
234-
final String bundleKey = getBundleKey(instruction);
235-
restoredStates.put(bundleKey, new InstanceStateHolder(instruction, currentMethod));
250+
// TODO : Check call is actually to a restore method!
251+
final String bundleKey = getBundleKeyForMethodCall(instruction);
252+
if (bundleKey == null) {
253+
context.report(KEY_IS_NOT_CONSTANT, currentMethod, instruction,
254+
context.getLocation(instruction), NON_CONSTANT_KEY);
255+
} else {
256+
restoredStates.put(bundleKey, new InstanceStateHolder(instruction, currentMethod));
257+
}
236258
}
237259
}
238260

@@ -303,22 +325,82 @@ private VarInsnNode getOwnerNode(@Nonnull final AbstractInsnNode instruction, fi
303325
return (VarInsnNode) varNode;
304326
}
305327

306-
@Nonnull
307-
private String getBundleKey(@Nonnull final AbstractInsnNode instruction) {
328+
@CheckForNull
329+
private String getBundleKeyForMethodCall(@Nonnull final MethodInsnNode instruction) {
330+
int expectedArgs = instruction.name.startsWith("put") ? 2 : 1;
308331
AbstractInsnNode node = instruction;
309-
310-
// get the key used
311-
while (!(node instanceof LdcInsnNode)) {
332+
333+
// Lookup the stack, until we find the first argument, which is always the key
334+
while (expectedArgs > 0) {
312335
node = node.getPrevious();
336+
337+
if (node.getOpcode() > 0 && node.getOpcode() <= 0x2d) {
338+
// *const*, ldc*, *push and *load* up to aload_3; all add a single value to the stack
339+
expectedArgs--;
340+
} else if (node.getOpcode() <= 0x35) {
341+
// load form array, consume 2 elements form stack, and pushes one back
342+
expectedArgs++;
343+
} else if (node.getOpcode() <= 0x4e) {
344+
// *store* moves one element form the the stack to the local var table
345+
expectedArgs++;
346+
} else if (node.getOpcode() <= 0x56) {
347+
// *astore* moves one element form the the stack to a local array
348+
expectedArgs += 3;
349+
} else if (node.getOpcode() <= 0x57) {
350+
// pop discards a single value
351+
expectedArgs++;
352+
} else if (node.getOpcode() <= 0x58) {
353+
// pop2 discards 2 values
354+
expectedArgs += 2;
355+
} else if (node.getOpcode() <= 0x58) {
356+
// dup* adds an extra value to the stack
357+
expectedArgs += 2;
358+
} else if (node.getOpcode() <= 0x5e) {
359+
// TODO : dup2* use words, but they may be a single or 2 values...
360+
} else if (node.getOpcode() <= 0x5f) {
361+
// swap doesn't alter stack size
362+
} else if (node.getOpcode() <= 0x73) {
363+
// *add, *sub, *mul, *div, *rem takes 2 values, and pushes back 1
364+
expectedArgs++;
365+
} else if (node.getOpcode() <= 0x77) {
366+
// *neg doen't change the stack size
367+
} else if (node.getOpcode() <= 0x83) {
368+
// *shl, *shr, *and, *or, *xor takes 2 values, and pushes back 1
369+
expectedArgs++;
370+
} else if (node.getOpcode() <= 0x93) {
371+
// iinc, x2y don't modify the stack size
372+
} else if (node.getOpcode() <= 0x98) {
373+
// *cmp* takes 2 values, and pushes back 1
374+
expectedArgs++;
375+
} else if (node.getOpcode() <= 0xb1) {
376+
// branching, goto, return... none expected here...
377+
} else if (node.getOpcode() <= 0xb2) {
378+
// getstatic
379+
expectedArgs--;
380+
} else if (node.getOpcode() <= 0xb3) {
381+
// putstatic
382+
expectedArgs++;
383+
} else if (node.getOpcode() <= 0xb4) {
384+
// getfield takes one and puts one back
385+
} else if (node.getOpcode() <= 0xb5) {
386+
// putfield takes two
387+
expectedArgs += 2;
388+
} else if (node.getOpcode() <= 0xba) {
389+
// TODO : invoke* may take arguments, not sure how to deal with this...
390+
} else if (node.getOpcode() <= 0xbb) {
391+
// new creates a new object
392+
expectedArgs--;
393+
} else {
394+
// More branching instructions, throws and things we don't expect here
395+
}
313396
}
314-
315-
// check if we have local variables
316-
if (node.getPrevious() instanceof LdcInsnNode) {
317-
// get the previos node to get the key
318-
node = node.getPrevious();
397+
398+
// We have our instruction!
399+
if (node instanceof LdcInsnNode) {
400+
return ((LdcInsnNode) node).cst.toString();
319401
}
320402

321-
return ((LdcInsnNode) node).cst.toString();
403+
return null;
322404
}
323405

324406
@Override
@@ -386,7 +468,7 @@ private void reportOverwritingFieldsAndInvalidType(@Nonnull final Map<String, In
386468
statesToRemove.remove(entry.getKey());
387469
}
388470

389-
final AbstractInsnNode instruction = entry.getValue().getInstruction();
471+
final MethodInsnNode instruction = entry.getValue().getInstruction();
390472
final FieldInsnNode field = getField(instruction, isRestoring);
391473
if (field == null) {
392474
// we are restoring or saving a key locally
@@ -402,7 +484,7 @@ private void reportOverwritingFieldsAndInvalidType(@Nonnull final Map<String, In
402484
fields.add(nameSaved);
403485
}
404486

405-
final String descriptor = ((MethodInsnNode) instruction).desc;
487+
final String descriptor = instruction.desc;
406488
String type;
407489
if (isRestoring) {
408490
type = descriptor.substring(descriptor.indexOf(')') + 1);
@@ -439,10 +521,10 @@ private void reportSaveRestoreWithDifferentTypes(@Nonnull final ClassContext con
439521

440522
// check the field type with the expected type
441523
if (!field.desc.equals(expectedtype)) {
442-
final AbstractInsnNode instruction = instanceScopeHolder.getInstruction();
524+
final MethodInsnNode instruction = instanceScopeHolder.getInstruction();
443525
context.report(INVALID_TYPE, instanceScopeHolder.getMethodNode(), instruction,
444526
classContext.getLocation(instruction),
445-
String.format(message, ((MethodInsnNode) instruction).name, expectedtype, field.name, field.desc));
527+
String.format(message, instruction.name, expectedtype, field.name, field.desc));
446528
}
447529
}
448530

@@ -507,9 +589,9 @@ private void report(@Nonnull final ClassContext context, @Nonnull final Map<Stri
507589
@Nonnull final String message) {
508590
if (!states.isEmpty()) {
509591
for (final Entry<String, InstanceStateHolder> entry : states.entrySet()) {
510-
final AbstractInsnNode instruction = entry.getValue().getInstruction();
592+
final MethodInsnNode instruction = entry.getValue().getInstruction();
511593
context.report(MISSING_SAVED_INSTANCE_STATES, entry.getValue().getMethodNode(), instruction,
512-
classContext.getLocation(instruction), String.format(message, getBundleKey(instruction)));
594+
classContext.getLocation(instruction), String.format(message, entry.getKey()));
513595
}
514596
}
515597
}

‎src/main/java/com/monits/linters/MonitsIssueRegistry.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,9 @@ public List<Issue> getIssues() {
3131
ParcelDetector.MISSING_OR_OUT_OF_ORDER,
3232
ParcelDetector.INCOMPATIBLE_READ_WRITE_TYPE,
3333
FactoryMethodDetector.USE_FACTORY_METHOD_INSTEAD_NEW_FRAGMENT,
34-
InstanceStateDetector.MISSING_SAVED_INSTANCE_STATES);
34+
InstanceStateDetector.MISSING_SAVED_INSTANCE_STATES,
35+
InstanceStateDetector.KEY_IS_NOT_CONSTANT,
36+
InstanceStateDetector.OVERWRITING_FIELDS,
37+
InstanceStateDetector.OVERWRITING_INSTANCE_STATES);
3538
}
3639
}

‎src/main/java/com/monits/linters/instancestate/holder/InstanceStateHolder.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,15 @@
1515

1616
import javax.annotation.Nonnull;
1717

18-
import org.objectweb.asm.tree.AbstractInsnNode;
18+
import org.objectweb.asm.tree.MethodInsnNode;
1919
import org.objectweb.asm.tree.MethodNode;
2020

2121
/**
2222
* Class to hold the instruction and the method of the state to be analyzed
2323
*/
2424
public class InstanceStateHolder {
2525

26-
private final AbstractInsnNode instruction;
26+
private final MethodInsnNode instruction;
2727
private final MethodNode methodNode;
2828

2929
/**
@@ -32,7 +32,7 @@ public class InstanceStateHolder {
3232
* @param instruction The instruction of the state analyzed
3333
* @param methodNode The methodNode of the state analyzed
3434
*/
35-
public InstanceStateHolder(@Nonnull final AbstractInsnNode instruction, @Nonnull final MethodNode methodNode) {
35+
public InstanceStateHolder(@Nonnull final MethodInsnNode instruction, @Nonnull final MethodNode methodNode) {
3636
this.instruction = instruction;
3737
this.methodNode = methodNode;
3838
}
@@ -41,7 +41,7 @@ public InstanceStateHolder(@Nonnull final AbstractInsnNode instruction, @Nonnull
4141
* @return the instruction
4242
*/
4343
@Nonnull
44-
public AbstractInsnNode getInstruction() {
44+
public MethodInsnNode getInstruction() {
4545
return instruction;
4646
}
4747

0 commit comments

Comments
 (0)