CodeReview常见错误集合(FindBugs)

FindBugs插件代码分析结果中的分类：

• Malicious Code Vulnerability 恶意代码漏洞
• Correctness 正确性
• Security 安全性问题
• Performance 性能问题
• Experimental 实验性问题
• Bad Practice 坏习惯
• Dodgy Code 小问题

常见错误列表及建议

• 【Correctness】Possible null pointer dereference of con on exception path 可能引起NullPointException的异常未做是否为空判断

• 【Security】Relative path traversal: 目录遍历漏洞
  在进行下载和上传功能时，对前端请求的文件名不做过滤和判断将会产生服务器目录和文件暴露的风险。
  解决方式：虽然前端有js判断文件名和格式，但是这对攻击者来说是无效的，后端应该对其抱有不信任的态度，严格判断文件名参数。过滤带有“../”，"..\"，"C:"，"c:"，"etc/passwd"等字符的参数。

• 【Malicious Code Vulnerability】May expose internal representation by returning(or incorporating) reference to mustable object:
  类实例生成之后，里面包含非原始数据类型，如Date类型数据。这就导致不光实体类的set方法可以改变其值，外部引用之后也可能改变Date类型，会导致不安全或者数据错误。

错误代码示例：

public class QCData {
    private Date labdate;
    public Date getLabdate() {
       return labdate;
    }

   public void setLabdate(Date labdate) {
          this.labdate = labdate;
    }
     
}

当我们运行测试类时会发现：

public class ExposeTest {
    public static void main(String[] args){
        QCData qcdata = new QCData();
        Date labdate = new Date();
        qcdata.setLabdate(labdate); 
        System.err.println(qcdata.getLabdate());
        labdate.setTime(11122222);//外部引用改变了labdate的值
        System.err.println(qcdata.getLabdate());//这里并没有再次调用实体类的setLabdate方法，但是labdate的值已经改变
    }
}
结果：
Mon Sep 25 16:52:51 CST 2017
Thu Jan 01 11:05:22 CST 1970

解决方式:

public class QCData {
    private Date labdate;
    public Date getLabdate() {
        if (labdate == null)
        {  
            return null;
        }
        return (Date)labdate.clone();
     }
  
    public void setLabdate(Date labdate) {
        if (labdate == null)
        {
            this.labdate = null;
        } else {
            this.labdate = (Date)labdate.clone();
        }
   }
}
//再次运行测试类会发现对外部引用的改变将不对实例内的值产生影响

• 【Malicious Code Vulnerability】Field isn't final but should be:静态变量需要设置成final

public class CommonTips {
    public static String TIPS_OPERATE_SUCCESS = "成功！";
    public static String TIPS_OPERATE_FAIL = "失败！";
    /**
    *修改为 public static final String TIPS_OPERATE_SUCCESS = "成功！";
    */
}

• 【Performance】Inefficient use of keySet iterator instead of entrySet iterator: 使用迭代器时，用entrySet 来代替keySet。效率会有不小的差距。因为keySet是循环获得了key。往往拿到key之后，又会去用key来获取值，这一次的查找就是浪费的。

for (String key : dataMap.keySet()) {  //迭代key
    if(key.equals("a")){
        dataMap.get(key);  //根据key获取value的时候。又是一次迭代
    }
    ...           
 }

解决方式：

for(Map.Entry<String,String> entry : dataMap.entrySet()){  //只需要一次迭代
    String key = entry.getKey(); 
    if(key.equals("a")){
        entry.getValue();
    }
}

• 【Performance】Method concatenates strings using + in a loop : 拼接字符串时使用了“+” 。
  解决方式: 用StringBuffer/StringBuilder 来替代 “+” 号
• 【Performance】Method invoke inefficient new String(String) constructor: 用低效的方式创建了String对象。
  代码示例：

String tableHtml = new String(Common.getTableHtml(columns, columnsName, voucherList));
/**
*请不要 new String();  无须在堆中新建一个String对象。
*/

• 【Experimental】Method may fail to clean up stream or resource：打开的资源未做手动关闭。很多文件流的操作中，会出现未关闭的情况。长期不关闭将占用程序的内存资源，并且文件可能无法被更改从而影响其他功能。
  代码示例：

public static void copyFile(String oldPath, String newPath) {
        try {
            int bytesum = 0;
            int byteread = 0;
            File oldfile = new File(oldPath);
            if (oldfile.exists()) {
                InputStream inStream = new FileInputStream(oldPath);
                FileOutputStream fs = new FileOutputStream(newPath);
                byte[] buffer = new byte[1444];
                while ((byteread = inStream.read(buffer)) != -1) {
                    bytesum += byteread;
                    fs.write(buffer, 0, byteread);
                }
                inStream.close();//只关闭了InputStream。未对FileOutputStream进行关闭。
            }
        } catch (Exception e) {
            System.out.println("复制单个文件操作出错 ");
            e.printStackTrace();
        }
    }

解决方式:

public static void copyFile(String oldPath, String newPath) {
        InputStream inStream = null;
        FileOutputStream fs = null;
        try {
            int bytesum = 0;
            int byteread = 0;
            File oldfile = new File(oldPath);
            if (oldfile.exists()) {
                inStream = new FileInputStream(oldPath);
                fs = new FileOutputStream(newPath);
                byte[] buffer = new byte[1444];
                while ((byteread = inStream.read(buffer)) != -1) {
                    bytesum += byteread;
                    fs.write(buffer, 0, byteread);
                }
            }
        } catch (Exception e) {
            System.out.println("复制单个文件操作出错 ");
            e.printStackTrace();
        }finally{  //在finally语句块中将资源关闭
          try {
                inStream.close();
                fs.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
          }
    }

• 【Dodgy Code】Dead store to local variable :从来没有被用到的对象都要删除。这个在部分IDE中也会有提示。
• 【Dodgy Code】Useless object created: 对象创建后没有被用到。请直接删除
• 【Dodgy Code】Exception is caught when exception is not thrown:捕获的异常未被抛出
• 【Bad Practice】The method name doesn't start with a lower case letter:方法首字母没有以小写开头
• 【Bad Practice】Checking String equality using == or !=
  : String的值比对用了==或！=
  解决方式：使用equals（）