findbugs 常见问题及解决方案

最新推荐文章于 2020-07-30 11:13:40 发布

curiousby

最新推荐文章于 2020-07-30 11:13:40 发布

阅读量2.8k

点赞数 1

分类专栏： Findbugs exception 文章标签： findbugs 常见问题解决方案

exception 同时被 2 个专栏收录

30 篇文章 0 订阅

订阅专栏

Findbugs

3 篇文章 0 订阅

订阅专栏

1 Bad practice 坏的实践

一些不好的实践，下面列举几个： HE：类定义了equals()，却没有hashCode()；或类定义了equals()，却使用Object.hashCode()；或类定义了hashCode()，却没有equals()；或类定义了hashCode()，却使用Object.equals()；类继承了equals()，却使用Object.hashCode()。
SQL：Statement 的execute方法调用了非常量的字符串；或Prepared Statement是由一个非常量的字符串产生。
DE：方法终止或不处理异常，一般情况下，异常应该被处理或报告，或被方法抛出。
Malicious code vulnerability 可能受到的恶意攻击

如果代码公开，可能受到恶意攻击的代码，下面列举几个： FI：一个类的finalize()应该是protected，而不是public的。 MS：属性是可变的数组；属性是可变的Hashtable；属性应该是package protected的。

2 Correctness 一般的正确性问题

可能导致错误的代码，下面列举几个： NP：空指针被引用；在方法的异常路径里，空指针被引用；方法没有检查参数是否null；null值产生并被引用；null值产生并在方法的异常路径被引用；传给方法一个声明为@NonNull的null参数；方法的返回值声明为@NonNull实际是null。 Nm：类定义了hashcode()方法，但实际上并未覆盖父类Object的hashCode()；类定义了tostring()方法，但实际上并未覆盖父类Object的toString()；很明显的方法和构造器混淆；方法名容易混淆。 SQL：方法尝试访问一个Prepared Statement的0索引；方法尝试访问一个ResultSet的0索引。 UwF：所有的write都把属性置成null，这样所有的读取都是null，这样这个属性是否有必要存在；或属性从没有被write。

3 Dodgy 危险的

具有潜在危险的代码，可能运行期产生错误，下面列举几个： CI：类声明为final但声明了protected的属性。 DLS：对一个本地变量赋值，但却没有读取该本地变量；本地变量赋值成null，却没有读取该本地变量。 ICAST：整型数字相乘结果转化为长整型数字，应该将整型先转化为长整型数字再相乘。 INT：没必要的整型数字比较，如X <= Integer.MAX_VALUE。 NP：对readline()的直接引用，而没有判断是否null；对方法调用的直接引用，而方法可能返回null。 REC：直接捕获Exception，而实际上可能是RuntimeException。 ST：从实例方法里直接修改类变量，即static属性。

4 Performance 性能问题

可能导致性能不佳的代码，下面列举几个： DM：方法调用了低效的Boolean的构造器，而应该用Boolean.valueOf(…)；用类似Integer.toString(1) 代替new Integer(1).toString()；方法调用了低效的float的构造器，应该用静态的valueOf方法。 SIC：如果一个内部类想在更广泛的地方被引用，它应该声明为static。 SS：如果一个实例属性不被读取，考虑声明为static。 UrF：如果一个属性从没有被read，考虑从类中去掉。 UuF：如果一个属性从没有被使用，考虑从类中去掉。

5 Multithreaded correctness 多线程的正确性多线程编程时，可能导致错误的代码，下面列举几个：

ESync：空的同步块，很难被正确使用。 MWN：错误使用notify()，可能导致IllegalMonitorStateException异常；或错误的使用wait()。 No：使用notify()而不是notifyAll()，只是唤醒一个线程而不是所有等待的线程。 SC：构造器调用了Thread.start()，当该类被继承可能会导致错误。

6 Internationalization 国际化当对字符串使用upper或lowercase方法，如果是国际的字符串，可能会不恰当的转换。

High

1.DM_DEFAULT_ENCODING

1.1 Found reliance on default encoding in com.cmcc.aoi.httprequest.service.HttpRequest.sendGet(String, String): new java.io.InputStreamReader(InputStream)

Found a call to a method which will perform a byte to String (or String to byte) conversion, and will assume that the default platform encoding is suitable. This will cause the application behaviour to vary between platforms. Use an alternative API and specify a charset name or Charset object explicitly.

new BufferedReader(new InputStreamReader(connection.getInputStream()));

修改为： InputStreamReader fileData = new InputStreamReader(file ,"utf-8");

1.2 Found reliance on default encoding in com.cmcc.aoi.httprequest.service.HttpRequest.sendPost(String, JSONObject): new java.io.PrintWriter(OutputStream)

out = new PrintWriter(conn.getOutputStream());

修改为： out = new PrintWriter(new OutputStreamWriter(conn.getOutputStream(), "utf-8"));

1.3 Found reliance on default encoding in com.cmcc.aoi.selfhelp.action.DeliverWebRequestAction.calculateUserCount(HttpServletRequest): String.getBytes()

fileName = new String(req.getParameter("fileName").getBytes(), "UTF-8");

修改为

fileName = new String(req.getParameter("fileName").getBytes("UTF-8"), "UTF-8");

1.4Found reliance on default encoding in com.cmcc.aoi.selfhelp.action.servlet.AoeRegAction.report(HttpServletRequest, HttpServletResponse): java.io.ByteArrayOutputStream.toString()

logger.info("RECV STR: " + baos.toString());

修改为

logger.info("RECV STR: " + baos.toString("utf-8"));

1.5 Found reliance on default encoding in com.cmcc.aoi.selfhelp.action.servlet.AoeUploadLogAction.report(HttpServletRequest, HttpServletResponse): new java.io.FileWriter(File)

new FileWriter(f).append(baos.toString("UTF-8")).close();

修改为

BufferedWriter out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(f, true)));

out.write(baos.toString("UTF-8"));

out.close();

BufferedWriter bw= new BufferedWriter( new OutputStreamWriter(new FileOutputStream(filePath, true), "utf-8"));

1.6 Found reliance on default encoding in new com.cmcc.aoi.util.TokenZipFileUtil(String): new java.io.FileReader(String)

FileReader in = new FileReader(file);

改为

BufferedReader reader = new BufferedReader(new InputStreamReader(new FileInputStream(file), "UTF-8"));

2.MS_SHOULD_BE_FINAL

com.cmcc.aoi.selfhelp.action.DeliverWebRequestAction.logger isn't final but should be

This static field public but not final, and could be changed by malicious code or by accident from another package. The field could be made final to avoid this vulnerability.

protected static Logger logger = LoggerFactory.getLogger(DeliverWebRequestAction.class);

修改为 protected static final Logger logger = LoggerFactory.getLogger(DeliverWebRequestAction.class);

3.DLS_DEAD_LOCAL_STORE

Dead store to s in com.cmcc.aoi.selfhelp.action.DeliverWebRequestAction.textSend(WebSendTextForm, HttpServletRequest)

This instruction assigns a value to a local variable, but the value is not read or used in any subsequent instruction. Often, this indicates an error, because the value computed is never used.

Note that Sun's javac compiler often generates dead stores for final local variables. Because FindBugs is a bytecode-based tool, there is no easy way to eliminate these false positives.

ShopMappingDeliver shopMappingDeliver = null;

删除即可

4.ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD

Write to static field com.cmcc.aoi.selfhelp.action.MultipleMediaAoeAction.linkRoot from instance method com.cmcc.aoi.selfhelp.action.MultipleMediaAoeAction.afterPropertiesSet()

This instance method writes to a static field. This is tricky to get correct if multiple instances are being manipulated, and generally bad practice.

linkRoot = sysConfigService.getDomainName() + "/";

修改改为：

publicstatic String getLinkRoot() {

returnlinkRoot;

}

publicstaticvoid setLinkRoot(String linkRoot) {

MultipleMediaAoeAction.linkRoot = linkRoot;

}

MultipleMediaAoeAction.setLinkRoot(sysConfigService.getDomainName() + "/");

5. J2EE_STORE_OF_NON_SERIALIZABLE_OBJECT_INTO_SESSION

Store of non serializable com.cmcc.aoi.selfhelp.action.UploadFileAction$FileUploadStatus into HttpSession in new com.cmcc.aoi.selfhelp.action.UploadFileAction$MyProgressListener(UploadFileAction, HttpServletRequest)

This code seems to be storing a non-serializable object into an HttpSession. If this session is passivated or migrated, an error will result.

修改为 FileUploadStatus implements Serializable

6. RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE

Redundant nullcheck of rtr, which is known to be non-null in com.cmcc.aoi.selfhelp.action.servlet.AoeReportApplistAction.device(HttpServletRequest, HttpServletResponse)

This method contains a redundant check of a known non-null value against the constant null.

if (rtr != null) {

Writer writer;

try {

writer = response.getWriter();

if (rtr != null) {

try {

String s = JSONUtil.objToJson(rtr);

if (LOGGER.isDebugEnabled()) {

LOGGER.debug("SEND STR: " + s);

}

writer.write(s);

writer.flush();

} catch (IOException e) {

LOGGER.warn("", e);

if (writer != null) {

try {

writer.write(JSONUtil.objToJson(rtr));

} catch (IOException e1) {

LOGGER.warn("", e1);

}

} else {

response.getWriter().write("{\"errorCode\":401}");

}

} catch (IOException e2) {

LOGGER.warn("", e2);

}

修改为

if (rtr != null) {

Writer writer;

try {

writer = response.getWriter();

try {

String s = JSONUtil.objToJson(rtr);

if (LOGGER.isDebugEnabled()) {

LOGGER.debug("SEND STR: " + s);

}

writer.write(s);

writer.flush();

} catch (IOException e) {

LOGGER.warn("", e);

if (writer != null) {

try {

writer.write(JSONUtil.objToJson(rtr));

} catch (IOException e1) {

LOGGER.warn("", e1);

}

} catch (IOException e2) {

LOGGER.warn("", e2);

}

} else {

response.getWriter().write("{\"errorCode\":401}");

}

7. RU_INVOKE_RUN

com.cmcc.aoi.selfhelp.action.servlet.UploadTokensAction$TokenFileThread.run() explicitly invokes run on a thread (did you mean to start it instead?)

This method explicitly invokes run() on an object. In general, classes implement the Runnable interface because they are going to have their run() method invoked in a new thread, in which case Thread.start() is the right method to call.

ti.run();

修改为：

ti.start();

try {

ti.join();

} catch (InterruptedException e) {

e.printStackTrace();

}

8. NM_SAME_SIMPLE_NAME_AS_SUPERCLASS

The class name com.cmcc.aoi.selfhelp.dao.BaseDao shadows the simple name of the superclass org.slave4j.orm.hibernate.BaseDao

This class has a simple name that is identical to that of its superclass, except that its superclass is in a different package (e.g., alpha.Foo extends beta.Foo). This can be exceptionally confusing, create lots of situations in which you have to look at import statements to resolve references and creates many opportunities to accidentally define methods that do not override methods in their superclasses

com.cmcc.aoi.selfhelp.dao.BaseDao

修改为

com.cmcc.aoi.selfhelp.dao.BasisDao

9. SE_BAD_FIELD_INNER_CLASS

com.cmcc.aoi.selfhelp.action.UploadFileAction$FileUploadStatus is serializable but also an inner class of a non-serializable class

This Serializable class is an inner class of a non-serializable class. Thus, attempts to serialize it will also attempt to associate instance of the outer class with which it is associated, leading to a runtime error.

If possible, making the inner class a static inner class should solve the problem. Making the outer class serializable might also work, but that would mean serializing an instance of the inner class would always also serialize the instance of the outer class, which it often not what you really want.

修改外部类

UploadFileAction extends BaseAction implements Serializable

10. DM_BOXED_PRIMITIVE_FOR_PARSING

Boxing/unboxing to parse a primitive com.cmcc.aoi.selfhelp.dao.StatAppEveryHourDao.findWeekList(String)

A boxed primitive is created from a String, just to extract the unboxed primitive value. It is more efficient to just call the static parseXXX method.

statAppEveryHour.setNewnumber(Integer.valueOf(String.valueOf(objects[2]))); statAppEveryHour.setAccnumber(Integer.valueOf(String.valueOf(objects[3])));

修改为

statAppEveryHour.setStattime(sdf.parse(String.valueOf(objects[1])));

statAppEveryHour

.setNewnumber(Integer.parseInt(String.valueOf(objects[2]) != null

&& !"".equals(String.valueOf(objects[2]))

? String.valueOf(objects[2]) : "0"));

statAppEveryHour

.setAccnumber(Integer.parseInt(String.valueOf(objects[3]) != null

&& !"".equals(String.valueOf(objects[3]))

? String.valueOf(objects[3]) : "0"));

Normal

1.SBSC_USE_STRINGBUFFER_CONCATENATION

com.cmcc.aoi.httprequest.service.HttpRequest.sendGet(String, String) concatenates strings using + in a loop

The method seems to be building a String using concatenation in a loop. In each iteration, the String is converted to a StringBuffer/StringBuilder, appended to, and converted back to a String. This can lead to a cost quadratic in the number of iterations, as the growing string is recopied in each iteration.

Better performance can be obtained by using a StringBuffer (or StringBuilder in Java 1.5) explicitly.

For example:

// This is bad

String s = "";

for (int i = 0; i < field.length; ++i) {

s = s + field[i];

}

// This is better

StringBuffer buf = new StringBuffer();

for (int i = 0; i < field.length; ++i) {

buf.append(field[i]);

}

String s = buf.toString();

2. WMI_WRONG_MAP_ITERATOR

for (String key : map.keySet()) {

System.out.println(key + "--->" + map.get(key));

}

改为

for ( Map.Entry<String, List<String>> entry : map.entrySet()) {

System.out.println(entry.getKey() + "--->" + entry.getValue());

}

3. EI_EXPOSE_REP

com.cmcc.aoi.selfhelp.entity.Activation.getValidUntil() may expose internal representation by returning Activation.validUntil

Returning a reference to a mutable object value stored in one of the object's fields exposes the internal representation of the object. If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Returning a new copy of the object is better approach in many situations.

public Date getValidUntil() {

returnvalidUntil;

}

修改为

public Date getValidUntil() {

if(validUntil == null) {

returnnull;

}

return (Date) validUntil.clone();

}

4. EI_EXPOSE_REP2

com.cmcc.aoi.selfhelp.entity.Activation.setValidUntil(Date) may expose internal representation by storing an externally mutable object into Activation.validUntil

This code stores a reference to an externally mutable object into the internal representation of the object. If instances are accessed by untrusted code, and unchecked changes to the mutable object would compromise security or other important properties, you will need to do something different. Storing a copy of the object is better approach in many situations.

publicvoid setValidUntil(Date validUntil) {

this.validUntil = validUntil;

}

修改为

publicvoid setValidUntil(Date validUntil) {

if(validUntil == null) {

this.validUntil = null;

}else {

this.validUntil = (Date) validUntil.clone();

}

5. BC_VACUOUS_INSTANCEOF

instanceof will always return true for all non-null values in com.cmcc.aoi.selfhelp.entity.AppType.compareTo(AppType), since all com.cmcc.aoi.selfhelp.entity.AppType are instances of com.cmcc.aoi.selfhelp.entity.AppType

This instanceof test will always return true (unless the value being tested is null). Although this is safe, make sure it isn't an indication of some misunderstanding or some other logic error. If you really want to test the value for being null, perhaps it would be clearer to do better to do a null test rather than an instanceof test.

6. MS_MUTABLE_ARRAY

com.cmcc.aoi.selfhelp.entity.DeviceType.CURRENTUSEDDEVICES is a mutable array

A final static field references an array and can be accessed by malicious code or by accident from another package. This code can freely modify the contents of the array.

public static final int[] CURRENTUSEDDEVICES = new int []{Device.iOS.ordinal()，Device.Android.ordinal()，Device.WP.ordinal()}；

修改为

Public > protected

7. EQ_COMPARETO_USE_OBJECT_EQUALS

com.cmcc.aoi.selfhelp.entity.AppType defines compareTo(AppType) and uses Object.equals()

This class defines a compareTo(...) method but inherits its equals() method from java.lang.Object. Generally, the value of compareTo should return zero if and only if equals returns true. If this is violated, weird and unpredictable failures will occur in classes such as PriorityQueue. In Java 5 the PriorityQueue.remove method uses the compareTo method, while in Java 6 it uses the equals method.

From the JavaDoc for the compareTo method in the Comparable interface:

It is strongly recommended, but not strictly required that (x.compareTo(y)==0) == (x.equals(y)). Generally speaking, any class that implements the Comparable interface and violates this condition should clearly indicate this fact. The recommended language is "Note: this class has a natural ordering that is inconsistent with equals."

修改

添加 hashcode() 和 equals() 代码即可

8. BC_VACUOUS_INSTANCEOF

@Override

publicint compareTo(AppType o) {

if (oinstanceof AppType) {

AppType p = (AppType) o;

returnthis.typeId > p.typeId ? 1 : this.typeId == p.typeId ? 0 : -1;

}

return 1;

}

修改为

@Override

publicint compareTo(AppType o) {

if (null != o) {

AppType p = (AppType) o ;

returnthis.typeId > p.typeId ? 1 : this.typeId == p.typeId ? 0 : -1;

}

return 1;

}

9. ME_ENUM_FIELD_SETTER

com.cmcc.aoi.selfhelp.dto.ActivationSituation.setSituation(String) unconditionally sets the field situation

This public method declared in public enum unconditionally sets enum field, thus this field can be changed by malicious code or by accident from another package. Though mutable enum fields may be used for lazy initialization, it's a bad practice to expose them to the outer world. Consider removing this method or declaring it package-private.

publicvoid setCode(String code) {

this.code = code;

}

修改

删除该无用代码

10. IM_BAD_CHECK_FOR_ODD

Check for oddness that won't work for negative numbers in com.cmcc.aoi.selfhelp.dto.WebSendTextForm.toDeliverWebRequest()

The code uses x % 2 == 1 to check to see if a value is odd, but this won't work for negative numbers (e.g., (-5) % 2 == -1). If this code is intending to check for oddness, consider using x & 1 == 1, or x % 2 != 0.

DeliverFactory

.createTextOpenApp(this.msgtype, "", this.content,

this.isRingAndVibrate % 2 == 1,

isRingAndVibrate / 2 >= 1, this.activity)

.toJsonString());

修改为

DeliverFactory

.createTextOpenApp(this.msgtype, "", this.content,

this.isRingAndVibrate % 2 != 0,

isRingAndVibrate / 2 >= 1, this.activity)

.toJsonString());

11. MS_EXPOSE_REP

Public static com.cmcc.aoi.selfhelp.dict.DeviceSupported.getSupportedDevs() may expose internal representation by returning DeviceSupported.DEVS

A public static method returns a reference to an array that is part of the static state of the class. Any code that calls this method can freely modify the underlying array. One fix is to return a copy of the array.

public static Device[] getSupportedDevs() {

return DEVS;

}

修改为

publicstatic Device[] getSupportedDevs() {

return DeviceSupported.DEVS.clone();

}

12.URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD

Unread public/protected field: com.cmcc.aoi.selfhelp.dict.OperatorDict.countryCode

This field is never read. The field is public or protected, so perhaps it is intended to be used with classes not seen as part of the analysis. If not, consider removing it from the class.

publicintcode;

public String enName;

public String cnName;

public String countryCode;

public OperatorDict() {

}

/**

* @param code

* 运营商代码,一般是5位

* @param enName

* 英文名

* @param countryCode

* 国家英文代码

* @param cnName

* 中文名

public OperatorDict(intcode, String enName, String countryCode, String cnName) {

this.code = code;

this.enName = enName;

this.countryCode = countryCode;

this.cnName = cnName == null ? Integer.toString(code) : cnName;

}

修改为

Public -》 private

13. ES_COMPARING_STRINGS_WITH_EQ

Comparison of String objects using == or != in com.cmcc.aoi.selfhelp.entity.Provider.compareTo(Object)

This code compares java.lang.String objects for reference equality using the == or != operators. Unless both strings are either constants in a source file, or have been interned using the String.intern() method, the same string value may be represented by two different String objects. Consider using the equals(Object) method instead.

return this.spid.compareTo(p.spid) > 0 ? 1 : this.spid == p.spid ? 0 : -1;

修改为

this.spid.compareTo(p.spid) > 0 ? 1 : this.spid.equals(p.spid) ? 0 : -1;

14.DB_DUPLICATE_BRANCHES

com.cmcc.aoi.selfhelp.dao.ShStatTerminalDao.getListQuery(String, int, Date, Date, boolean, int) uses the same code for two branches

This method uses the same code to implement two branches of a conditional branch. Check to ensure that this isn't a coding mistake.

if (bool) {

query.setInteger(i++, nodeType);

} else {

query.setInteger(i++, nodeType);

}

修改为

query.setInteger(i++, nodeType);

15. SE_COMPARATOR_SHOULD_BE_SERIALIZABLE

com.cmcc.aoi.selfhelp.task.entity.StatAppHabitComparator implements Comparator but not Serializable

This class implements the Comparator interface. You should consider whether or not it should also implement the Serializable interface. If a comparator is used to construct an ordered collection such as a TreeMap, then the TreeMap will be serializable only if the comparator is also serializable. As most comparators have little or no state, making them serializable is generally easy and good defensive programming.

修改为

implements Serializable

16. UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD

Unwritten public or protected field: com.cmcc.aoi.selfhelp.task.entity.StatDevice.keyname

No writes were seen to this public/protected field. All reads of it will return the default value. Check for errors (should it have been initialized?), or remove it if it is useless.

Public String keyname;

修改为

Private String keyname;

16. REC_CATCH_EXCEPTION

Exception is caught when Exception is not thrown in com.cmcc.aoi.selfhelp.task.fileparser.TokenIncrease.parseLine(String[])

This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.

A better approach is to either explicitly catch the specific exceptions that are thrown, or to explicitly catch RuntimeException exception, rethrow it, and then catch all non-Runtime Exceptions, as shown below:

try {

...

} catch (RuntimeException e) {

throw e;

} catch (Exception e) {

... deal with all non-runtime exceptions ...

}

17. RV_RETURN_VALUE_IGNORED_BAD_PRACTICE

Exceptional return value of java.io.File.mkdirs() ignored in com.cmcc.aoi.util.FileUtil.moveFile(File, String)

This method returns a value that is not checked. The return value should be checked since it can indicate an unusual or unexpected function execution. For example, the File.delete() method returns false if the file could not be successfully deleted (rather than throwing an Exception). If you don't check the result, you won't notice if the method invocation signals unexpected behavior by returning an atypical return value.

tmp.mkdirs()

修改为

booleanmkdirs = tmp.mkdirs();

logger.debug("debug",mkdirs);

18. NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE

19. ICAST_IDIV_CAST_TO_DOUBLE

Integral division result cast to double or float in com.cmcc.aoi.selfhelp.service.BaseAnalysisService.getInterval(Date, Date, int)

This code casts the result of an integral division (e.g., int or long division) operation to double or float. Doing division on integers truncates the result to the integer value closest to zero. The fact that the result was cast to double suggests that this precision should have been retained. What was probably meant was to cast one or both of the operands to double before performing the division. Here is an example:

int x = 2;

int y = 5;

// Wrong: yields result 0.0

double value1 = x / y;

// Right: yields result 0.4

double value2 = x / (double) y;