Skip to content

Commit

Permalink
#406 don't report String.getBytes(String s) as not throwing an exception
Browse files Browse the repository at this point in the history
  • Loading branch information
@mebigfatguy
mebigfatguy committed Dec 2, 2023
1 parent 1f2a7db commit 35b763a
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 48 deletions.
104 changes: 62 additions & 42 deletions src/main/java/com/mebigfatguy/fbcontrib/detect/BogusExceptionDeclaration.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import com.mebigfatguy.fbcontrib.collect.MethodInfo;
import com.mebigfatguy.fbcontrib.collect.Statistics;
import com.mebigfatguy.fbcontrib.utils.BugType;
import com.mebigfatguy.fbcontrib.utils.FQMethod;
import com.mebigfatguy.fbcontrib.utils.OpcodeUtils;
import com.mebigfatguy.fbcontrib.utils.SignatureUtils;
import com.mebigfatguy.fbcontrib.utils.StopOpcodeParsingException;
Expand All @@ -43,15 +44,17 @@
import edu.umd.cs.findbugs.ba.ClassContext;

/**
* looks for constructors, private methods or static methods that declare that they throw specific checked exceptions, but that do not. This just causes callers
* of these methods to do extra work to handle an exception that will never be thrown. also looks for throws clauses where two exceptions declared to be thrown
* are related through inheritance.
* looks for constructors, private methods or static methods that declare that
* they throw specific checked exceptions, but that do not. This just causes
* callers of these methods to do extra work to handle an exception that will
* never be thrown. also looks for throws clauses where two exceptions declared
* to be thrown are related through inheritance.
*/
public class BogusExceptionDeclaration extends BytecodeScanningDetector {

private static final String IGNORE_INHERITED_METHODS_PROPERTY = "fb-contrib.bed.ignore_inherited";

private static final Set<String> safeClasses = UnmodifiableSet.create(
private static final Set<String> SAFE_CLASSES = UnmodifiableSet.create(
//@formatter:off
Values.SLASHED_JAVA_LANG_OBJECT,
Values.SLASHED_JAVA_LANG_STRING,
Expand All @@ -62,9 +65,15 @@ public class BogusExceptionDeclaration extends BytecodeScanningDetector {
Values.SLASHED_JAVA_LANG_SHORT,
Values.SLASHED_JAVA_LANG_BYTE,
Values.SLASHED_JAVA_LANG_BOOLEAN
//@formatter:on
//@formatter:on
);


private static final Set<FQMethod> SAFE_EXCEPTION_METHODS = UnmodifiableSet.create(
//@formatter:off
new FQMethod(Values.SLASHED_JAVA_LANG_STRING, "getBytes", "(Ljava/lang/String;)[B")
//@formatter:on
);

private static final boolean IGNORE_INHERITED_METHODS = Boolean.getBoolean(IGNORE_INHERITED_METHODS_PROPERTY);

private final BugReporter bugReporter;
Expand Down Expand Up @@ -93,8 +102,7 @@ public BogusExceptionDeclaration(BugReporter bugReporter) {
/**
* overrides the visitor to create the opcode stack
*
* @param classContext
* the context object of the currently parsed class
* @param classContext the context object of the currently parsed class
*
*/
@Override
Expand All @@ -115,10 +123,10 @@ public void visitClassContext(ClassContext classContext) {
}

/**
* implements the visitor to see if the method declares that it throws any checked exceptions.
* implements the visitor to see if the method declares that it throws any
* checked exceptions.
*
* @param obj
* the context object of the currently parsed code block
* @param obj the context object of the currently parsed code block
*/
@Override
public void visitCode(Code obj) {
Expand All @@ -127,21 +135,23 @@ public void visitCode(Code obj) {
if (method.isSynthetic()) {
return;
}

if (IGNORE_INHERITED_METHODS) {
MethodInfo mi = Statistics.getStatistics().getMethodStatistics(getClassName(), getMethodName(), getMethodSig());
if (mi != null && mi.isDerived()) {
return;
}
MethodInfo mi = Statistics.getStatistics().getMethodStatistics(getClassName(), getMethodName(),
getMethodSig());
if (mi != null && mi.isDerived()) {
return;
}
}

declaredCheckedExceptions.clear();
stack.resetForMethodEntry(this);

ExceptionTable et = method.getExceptionTable();
if (et != null) {
if (classIsFinal || classIsAnonymous || method.isStatic() || method.isPrivate() || method.isFinal()
|| ((Values.CONSTRUCTOR.equals(method.getName()) && !isAnonymousInnerCtor(method, getThisClass())))) {
|| ((Values.CONSTRUCTOR.equals(method.getName())
&& !isAnonymousInnerCtor(method, getThisClass())))) {
String[] exNames = et.getExceptionNames();
for (String exName : exNames) {
try {
Expand All @@ -157,8 +167,8 @@ public void visitCode(Code obj) {
try {
super.visitCode(obj);
if (!declaredCheckedExceptions.isEmpty()) {
BugInstance bi = new BugInstance(this, BugType.BED_BOGUS_EXCEPTION_DECLARATION.name(), NORMAL_PRIORITY).addClass(this)
.addMethod(this).addSourceLine(this, 0);
BugInstance bi = new BugInstance(this, BugType.BED_BOGUS_EXCEPTION_DECLARATION.name(),
NORMAL_PRIORITY).addClass(this).addMethod(this).addSourceLine(this, 0);
for (String ex : declaredCheckedExceptions) {
bi.addString(ex.replaceAll("/", "."));
}
Expand Down Expand Up @@ -189,8 +199,10 @@ public void visitCode(Code obj) {
}

if (!parentEx.equals(exceptionClass)) {
bugReporter.reportBug(new BugInstance(this, BugType.BED_HIERARCHICAL_EXCEPTION_DECLARATION.name(), NORMAL_PRIORITY).addClass(this)
.addMethod(this).addString(childEx.getClassName() + " derives from " + parentEx.getClassName()));
bugReporter.reportBug(new BugInstance(this,
BugType.BED_HIERARCHICAL_EXCEPTION_DECLARATION.name(), NORMAL_PRIORITY)
.addClass(this).addMethod(this)
.addString(childEx.getClassName() + " derives from " + parentEx.getClassName()));
return;
}

Expand All @@ -203,21 +215,23 @@ public void visitCode(Code obj) {
}

/**
* checks to see if this method is a constructor of an instance based inner class, the handling of the Exception table for this method is odd, -- doesn't
* checks to see if this method is a constructor of an instance based inner
* class, the handling of the Exception table for this method is odd, -- doesn't
* seem correct, in some cases. So just ignore these cases
*
* @param m
* the method to check
* @param cls
* the cls that owns the method
* @return whether this method is a ctor of an instance based anonymous inner class
* @param m the method to check
* @param cls the cls that owns the method
* @return whether this method is a ctor of an instance based anonymous inner
* class
*/
private static boolean isAnonymousInnerCtor(Method m, JavaClass cls) {
return Values.CONSTRUCTOR.equals(m.getName()) && (cls.getClassName().lastIndexOf(Values.INNER_CLASS_SEPARATOR) >= 0);
return Values.CONSTRUCTOR.equals(m.getName())
&& (cls.getClassName().lastIndexOf(Values.INNER_CLASS_SEPARATOR) >= 0);
}

/**
* implements the visitor to look for method calls that could throw the exceptions that are listed in the declaration.
* implements the visitor to look for method calls that could throw the
* exceptions that are listed in the declaration.
*/
@Override
public void sawOpcode(int seen) {
Expand All @@ -227,7 +241,8 @@ public void sawOpcode(int seen) {

if (OpcodeUtils.isStandardInvoke(seen)) {
String clsName = getClassConstantOperand();
if (!safeClasses.contains(clsName)) {
if (!SAFE_CLASSES.contains(clsName) || SAFE_EXCEPTION_METHODS
.contains(new FQMethod(clsName, getNameConstantOperand(), getSigConstantOperand()))) {
try {
JavaClass cls = Repository.lookupClass(clsName);
Method[] methods = cls.getMethods();
Expand All @@ -238,7 +253,8 @@ public void sawOpcode(int seen) {
if (m.getName().equals(methodName) && m.getSignature().equals(signature)) {

if (isAnonymousInnerCtor(m, cls)) {
// The java compiler doesn't properly attached an Exception Table to anonymous constructors, so just clear if so
// The java compiler doesn't properly attached an Exception Table to anonymous
// constructors, so just clear if so
break;
}

Expand Down Expand Up @@ -280,16 +296,19 @@ public void sawOpcode(int seen) {
}

/**
* removes this thrown exception the list of declared thrown exceptions, including all exceptions in this exception's hierarchy. If an exception class is
* found that can't be loaded, then just clear the list of declared checked exceptions and get out.
* removes this thrown exception the list of declared thrown exceptions,
* including all exceptions in this exception's hierarchy. If an exception class
* is found that can't be loaded, then just clear the list of declared checked
* exceptions and get out.
*
* @param thrownException
* the exception and it's hierarchy to remove
* @param thrownException the exception and it's hierarchy to remove
*/
private void removeThrownExceptionHierarchy(String thrownException) {
try {
if (Values.DOTTED_JAVA_LANG_EXCEPTION.equals(thrownException) || Values.DOTTED_JAVA_LANG_THROWABLE.equals(thrownException)) {
// Exception/Throwable can be thrown even tho the method isn't declared to throw Exception/Throwable in the case of templated Exceptions
if (Values.DOTTED_JAVA_LANG_EXCEPTION.equals(thrownException)
|| Values.DOTTED_JAVA_LANG_THROWABLE.equals(thrownException)) {
// Exception/Throwable can be thrown even tho the method isn't declared to throw
// Exception/Throwable in the case of templated Exceptions
clearExceptions();
} else {
removeException(thrownException);
Expand All @@ -314,10 +333,10 @@ private void removeThrownExceptionHierarchy(String thrownException) {
}

/**
* removes the declared checked exception, and if that was the last declared exception, stops opcode parsing by throwing exception
* removes the declared checked exception, and if that was the last declared
* exception, stops opcode parsing by throwing exception
*
* @param clsName
* the name of the exception to remove
* @param clsName the name of the exception to remove
*/
private void removeException(String clsName) {
declaredCheckedExceptions.remove(clsName);
Expand All @@ -327,7 +346,8 @@ private void removeException(String clsName) {
}

/**
* clears all declared checked exceptions and throws an exception to stop opcode parsing
* clears all declared checked exceptions and throws an exception to stop opcode
* parsing
*/
private void clearExceptions() {
declaredCheckedExceptions.clear();
Expand Down
18 changes: 12 additions & 6 deletions src/samples/java/ex/BED_Sample.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.io.UnsupportedEncodingException;
import java.sql.SQLException;
import java.util.Hashtable;
import java.util.Optional;
Expand Down Expand Up @@ -112,6 +113,10 @@ public static Process fpInterrupted(String command) throws IOException, Interrup
return p;
}

private void fp406(String p) throws UnsupportedEncodingException {
byte[] v = p.getBytes("UTF8");
}

static class FPAnonBase {

FPAnonBase(InputStream is) throws IOException {
Expand All @@ -123,15 +128,16 @@ public static FPAnonBase makeAnon(InputStream is) throws IOException {
};
}
}

static class BaseBED {
public void foo() throws IOException {
}
public void foo() throws IOException {
}
}

/* is caught based on the option fb-contrib.bed.ignore_inherited */
static class ChildBED extends BaseBED {
public void foo() throws IOException {
}
@Override
public void foo() throws IOException {
}
}
}

0 comments on commit 35b763a

Please sign in to comment.