[Fb-contrib-commit] SF.net SVN: fb-contrib:[1614] trunk/fb-contrib
Brought to you by:
dbrosius
Menu
▾
▴
[Fb-contrib-commit] SF.net SVN: fb-contrib:[1614] trunk/fb-contrib
|
From: <dbr...@us...> - 2010-09-19 02:30:51
|
Revision: 1614
http://fb-contrib.svn.sourceforge.net/fb-contrib/?rev=1614&view=rev
Author: dbrosius
Date: 2010-09-19 02:30:45 +0000 (Sun, 19 Sep 2010)
Log Message:
-----------
Redo SNG to look for if (a != null) a = somevalue;
Modified Paths:
--------------
trunk/fb-contrib/etc/messages.xml
trunk/fb-contrib/htdocs/index.shtml
trunk/fb-contrib/samples/SNG_Sample.java
trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/SuspiciousNullGuard.java
Modified: trunk/fb-contrib/etc/messages.xml
===================================================================
--- trunk/fb-contrib/etc/messages.xml 2010-09-17 04:51:05 UTC (rev 1613)
+++ trunk/fb-contrib/etc/messages.xml 2010-09-19 02:30:45 UTC (rev 1614)
@@ -1081,10 +1081,9 @@
<Details>
<![CDATA[
<p>Looks for code that checks to see if a field or local variable is not null,
- before entering a code block either an if, or while statement, and then doesn't
- reference that field or local in the block of code that is guarded by the null
- check. It is likely that null check is being done on the wrong variable, either
- because of a copy/paste error, or a change in implementation.</p>
+ before entering a code block either an if, or while statement, and then reassigns that
+ field or local variable. It is likely that guard should have been to see if that
+ field or local variable is null, not, not null</p>
<p>It is a fast detector</p>
]]>
</Details>
@@ -2953,26 +2952,25 @@
</BugPattern>
<BugPattern type="SNG_SUSPICIOUS_NULL_FIELD_GUARD">
- <ShortDescription>Method tests a field for null as guard for code that doesn't use it</ShortDescription>
- <LongDescription>Method {1} tests a field for null as guard for code that doesn't use it</LongDescription>
+ <ShortDescription>Method tests a field for not null as guard and reassigns it</ShortDescription>
+ <LongDescription>Method {1} tests a field for not null as guard and reassigns it</LongDescription>
<Details>
<![CDATA[
<p>This method tests a field to make sure it's not null before executing a conditional block of
- code. However, it does not appear that this block of code relies on the field in question, so the
- choice of null guard seems dubious. It is possible that the code block calls a method that requires
- this field not to be null, but that would seem like an odd construct.</p>
+ code. However in the conditional block is reassigns the field. It is likely that the guard
+ should have been a check to see if the field is null, not that the field was not null.</p>
]]>
</Details>
</BugPattern>
<BugPattern type="SNG_SUSPICIOUS_NULL_LOCAL_GUARD">
- <ShortDescription>Method tests a local variable for null as guard for code that doesn't use it</ShortDescription>
- <LongDescription>Method {1} tests a local variable for null as guard for code that doesn't use it</LongDescription>
+ <ShortDescription>Method tests a local variable for not null as guard and reassigns it</ShortDescription>
+ <LongDescription>Method {1} tests a local variable for not null as guard and reassigns it</LongDescription>
<Details>
<![CDATA[
<p>This method tests a local variable to make sure it's not null before executing a conditional block of
- code. However, this block of does not access the local variable in question, so the
- choice of null guard seems wrong. Perhaps this is a copy/paste mistake.</p>
+ code. However in the conditional block is reassigns the local variable. It is likely that the guard
+ should have been a check to see if the local variable is null, not that the local variable was not null.</p>
]]>
</Details>
</BugPattern>
Modified: trunk/fb-contrib/htdocs/index.shtml
===================================================================
--- trunk/fb-contrib/htdocs/index.shtml 2010-09-17 04:51:05 UTC (rev 1613)
+++ trunk/fb-contrib/htdocs/index.shtml 2010-09-19 02:30:45 UTC (rev 1614)
@@ -82,12 +82,10 @@
must be restricted to covariant or invariant usage.
<span style="color: #0000FF;">--contributed by Bhaskar Maddala - THANKS!</span></li>
<li><b>[SNG] Suspicious Null Guard</b><br/>
- Looks for code that checks to see if a field or local variable is not null,
- before entering a code block (either an if, or while statement) and then doesn't
- reference that field or local in the block of code that is guarded by the null
- check. Instead it references another object of the same type. It is likely that null
- check is being done on the wrong variable, either because of a copy/paste error,
- or a change in implementation.</li>
+ Looks for code that checks to see if a field or local variable is not null
+ before entering a code block either an if, or while statement, and reassigns
+ that field or variable. It seems that perhaps the guard should check if the field
+ or variable is null.</li>
<li><b>[PUS] Possible Unsuspected Serialization</b><br/>
Looks for serialization of non-static inner classes. As this serializes
the enclosing class, it may unintentionally bring in more to the serialization
Modified: trunk/fb-contrib/samples/SNG_Sample.java
===================================================================
--- trunk/fb-contrib/samples/SNG_Sample.java 2010-09-17 04:51:05 UTC (rev 1613)
+++ trunk/fb-contrib/samples/SNG_Sample.java 2010-09-19 02:30:45 UTC (rev 1614)
@@ -7,104 +7,33 @@
private Object f1 = null;
private final Object f2 = null;
private final File file = null;
- private byte[] buffer = null;
+ private final byte[] buffer = null;
- public String badSNGFields()
+ public void badSNGFields()
{
if (f1 != null)
{
- return f2.toString();
+ f1 = "Foo";
}
-
- return null;
}
- public String badSNGLocals(Object l1, Object l2)
+ public void badSNGLocals(Object l1, Object l2)
{
if (l1 != null)
{
- return l2.toString();
- }
-
- return null;
+ l1 = l2;
+ }
}
- public boolean fpReturn(Object o)
- {
- return o != null;
- }
-
- public boolean fpAssign(Object o)
- {
- boolean b = o != null;
- return b;
- }
-
- public boolean fpField()
- {
- if (f1 != null)
- {
- return true;
+ public void fpNGFieldSetToNull() {
+ if (f1 != null) {
+ f1 = null;
}
-
- return false;
}
- public void fpAssert()
- {
- assert (f1 != null) && f1.equals(f2);
- }
-
- public Object fpSetNull(Object o) {
- if (o != null)
- {
- o = null;
+ public void fpNGLocalSetToNull(String s1) {
+ if (s1 != null) {
+ s1 = null;
}
-
- return o;
}
-
- public void fpSetMemberNull()
- {
- if (f1 != null)
- {
- f1 = null;
- }
- }
-
- public void fpDual(Object o1, Object o2)
- {
- if ((o1 == null) || (o2 == null))
- {
- throw new IllegalArgumentException("o1/o2 can not be null");
- }
- }
-
- public void discard()
- {
- if (file != null)
- {
- file.delete();
- }
- else if (buffer != null)
- {
- buffer = EMPTY_BYTE_ARRAY;
- }
- }
-
- public void fpCompound()
- {
- if ((file == null) || (buffer[0] == 0))
- {
- f1 = f2;
- }
- }
-
- public void fpDup()
- {
- if ((f1 == null) || file.isDirectory())
- {
- f1 = file.getPath();
- }
- }
}
Modified: trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/SuspiciousNullGuard.java
===================================================================
--- trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/SuspiciousNullGuard.java 2010-09-17 04:51:05 UTC (rev 1613)
+++ trunk/fb-contrib/src/com/mebigfatguy/fbcontrib/detect/SuspiciousNullGuard.java 2010-09-19 02:30:45 UTC (rev 1614)
@@ -19,12 +19,9 @@
package com.mebigfatguy.fbcontrib.detect;
import java.util.HashMap;
-import java.util.Iterator;
import java.util.Map;
import org.apache.bcel.classfile.Code;
-import org.apache.bcel.classfile.LocalVariable;
-import org.apache.bcel.classfile.LocalVariableTable;
import com.mebigfatguy.fbcontrib.utils.RegisterUtils;
@@ -37,16 +34,14 @@
/**
* looks for code that checks to see if a field or local variable is not null,
- * before entering a code block either an if, or while statement, and then doesn't
- * reference that field or local in the block of code that is guarded by the null
- * check. It is likely that null check is being done on the wrong variable, either
- * because of a copy/paste error, or a change in implementation.
+ * before entering a code block either an if, or while statement, and reassigns
+ * that field or variable. It seems that perhaps the guard should check if the field
+ * or variable is null.
*/
public class SuspiciousNullGuard extends BytecodeScanningDetector {
private final BugReporter bugReporter;
private OpcodeStack stack;
- private LocalVariableTable lvt;
private Map<Integer, NullGuard> nullGuards;
/**
@@ -80,14 +75,9 @@
*/
@Override
public void visitCode(Code obj) {
- try {
- stack.resetForMethodEntry(this);
- lvt = getMethod().getLocalVariableTable();
- nullGuards.clear();
- super.visitCode(obj);
- } finally {
- lvt = null;
- }
+ stack.resetForMethodEntry(this);
+ nullGuards.clear();
+ super.visitCode(obj);
}
/**
@@ -99,14 +89,7 @@
public void sawOpcode(int seen) {
try {
Integer pc = Integer.valueOf(getPC());
- NullGuard guard = nullGuards.remove(pc);
- if ((guard != null) && guard.sawSignatureOfGuard()) {
- boolean localBug = guard.getRegister() >= 0;
- bugReporter.reportBug(new BugInstance(this, localBug ? "SNG_SUSPICIOUS_NULL_LOCAL_GUARD" : "SNG_SUSPICIOUS_NULL_FIELD_GUARD", localBug ? NORMAL_PRIORITY : LOW_PRIORITY)
- .addClass(this)
- .addMethod(this)
- .addSourceLine(this, guard.getLocation()));
- }
+ nullGuards.remove(pc);
switch (seen) {
case IFNULL: {
@@ -125,140 +108,93 @@
}
}
break;
-
- case IFEQ:
- case IFNE:
- case IFLE:
- case IFGE:
- case IFGT:
- case IFLT:
- case IF_ICMPEQ:
- case IF_ICMPNE:
- case IF_ICMPGT:
- case IF_ICMPLE:
- case IF_ACMPEQ:
- case IF_ACMPNE: {
- int target = getBranchTarget();
- removeGuardsBeforePC(target);
- }
- break;
-
-
- case ALOAD:
- case ALOAD_0:
- case ALOAD_1:
- case ALOAD_2:
- case ALOAD_3: {
- if (lvt != null) {
- LocalVariable lv = lvt.getLocalVariable(RegisterUtils.getALoadReg(this, seen), getNextPC());
- if (lv != null) {
- markNullGuards(lv.getSignature());
- }
- }
- }
- break;
case ASTORE:
case ASTORE_0:
case ASTORE_1:
case ASTORE_2:
case ASTORE_3: {
- removeGuardForRegister(RegisterUtils.getAStoreReg(this, seen));
+ if (stack.getStackDepth() > 0) {
+ OpcodeStack.Item item = stack.getStackItem(0);
+ if (!item.isNull()) {
+ NullGuard guard = findNullGuardWithRegister(RegisterUtils.getAStoreReg(this, seen));
+ if (guard != null) {
+ bugReporter.reportBug(new BugInstance(this, "SNG_SUSPICIOUS_NULL_LOCAL_GUARD", NORMAL_PRIORITY)
+ .addClass(this)
+ .addMethod(this)
+ .addSourceLine(this));
+ nullGuards.remove(guard);
+ }
+ }
+ }
}
break;
- case GETFIELD: {
- markNullGuards(getSigConstantOperand());
- }
- break;
-
case PUTFIELD: {
- removeGuardForField(getXField());
- }
- break;
-
- case INVOKEVIRTUAL:
- case INVOKEINTERFACE: {
- if (nullGuards.size() > 0) {
- String clsName = getClassConstantOperand();
- String methodName = getNameConstantOperand();
- if ("java/io/PrintStream".equals(clsName) && methodName.startsWith("print")) {
- nullGuards.clear();
+ if (stack.getStackDepth() > 1) {
+ OpcodeStack.Item item = stack.getStackItem(0);
+ if (!item.isNull()) {
+ XField xf = getXFieldOperand();
+ if (xf != null) {
+ NullGuard guard = findNullGuardWithField(xf);
+ if (guard != null) {
+ bugReporter.reportBug(new BugInstance(this, "SNG_SUSPICIOUS_NULL_FIELD_GUARD", NORMAL_PRIORITY)
+ .addClass(this)
+ .addMethod(this)
+ .addSourceLine(this));
+ nullGuards.remove(guard);
+ }
+ }
}
}
}
break;
-
+
+ case IFEQ:
+ case IFNE:
+ case IFLT:
+ case IFGE:
+ case IFGT:
+ case IFLE:
+ case IF_ICMPEQ:
+ case IF_ICMPNE:
+ case IF_ICMPLT:
+ case IF_ICMPGE:
+ case IF_ICMPGT:
+ case IF_ICMPLE:
+ case IF_ACMPEQ:
+ case IF_ACMPNE:
+ case GOTO:
+ case GOTO_W:
case IFNONNULL:
- case ATHROW: {
- nullGuards.clear();
- }
+ nullGuards.clear();
break;
-
- case GOTO: {
- if (stack.getStackDepth() > 0) {
- nullGuards.clear();
- }
- }
- break;
}
} finally {
stack.sawOpcode(this, seen);
- if (stack.getStackDepth() > 0) {
- OpcodeStack.Item item = stack.getStackItem(0);
- int reg = item.getRegisterNumber();
- if (reg >= 0) {
- removeGuardForRegister(reg);
- } else {
- XField field = item.getXField();
- if (field != null) {
- removeGuardForField(field);
- }
- }
- }
}
}
- private void markNullGuards(String signature) {
- for (NullGuard ng : nullGuards.values()) {
- if (ng.getSignature().equals(signature)) {
- ng.markSignatureOfGuard();
+ private NullGuard findNullGuardWithRegister(int reg) {
+ for (NullGuard guard : nullGuards.values()) {
+ if (guard.getRegister() == reg) {
+ return guard;
}
}
+
+ return null;
}
- private void removeGuardForRegister(int reg) {
- Iterator<NullGuard> it = nullGuards.values().iterator();
- while (it.hasNext()) {
- NullGuard guard = it.next();
- if (reg == guard.getRegister()) {
- it.remove();
+ private NullGuard findNullGuardWithField(XField field) {
+ for (NullGuard guard : nullGuards.values()) {
+ if (field.equals(guard.getField())) {
+ return guard;
}
}
+
+ return null;
}
- private void removeGuardForField(XField field) {
- Iterator<NullGuard> it = nullGuards.values().iterator();
- while (it.hasNext()) {
- NullGuard guard = it.next();
- if (field != null) {
- if (field.equals(guard.getField())) {
- it.remove();
- }
- }
- }
- }
-
- private void removeGuardsBeforePC(int pc) {
- Iterator<Integer> it = nullGuards.keySet().iterator();
- while (it.hasNext()) {
- Integer nullGuardPC = it.next();
- if (pc > nullGuardPC) {
- it.remove();
- }
- }
- }
-
static class NullGuard {
int register;
XField field;
@@ -296,13 +232,5 @@
public String getSignature() {
return signature;
}
-
- public void markSignatureOfGuard() {
- sawSignature = true;
- }
-
- public boolean sawSignatureOfGuard() {
- return sawSignature;
- }
}
}
This was sent by the SourceForge.net collaborative development platform, the world's largest Open Source development site.
|
