一次老项目FindBugs Correct 经历

Richard_Yi 2020年01月17日 169次浏览

前言

被派到这个任务的时候我是懵逼的,因为老项目的代码简直惨不忍睹。

image.png

我寻思着不能让我一个人恶心啊,所以就整理出来这么篇文章,给同事做下错误示范。

这里只针对检测出来的比较典型的bug。

以下文章,用🐞来标识检测出来的 bug 。

顺便说一句,我个人觉得Findbugs 这种静态代码分析工具是必备的,坚持使用不仅可以加强代码的健壮性,也可以让开发对所使用的开发语言有更深的理解。

一、Correctness 正确性

代码的正确性是我们应该第一个保证的。

1.1 问题代码①

a. 问题代码

@Data // 🐞 Medium Confidence Correctness
public class AliCodeUrlReq extends PayReq implements Serializable {
    ...
}

b. 问题描述

equals method overrides equals in superclass and may not be symmetric

This class defines an equals method that overrides an equals method in a superclass. Both equals methods methods use instanceof in the determination of whether two objects are equal. This is fraught with peril, since it is important that the equals method is symmetrical (in other words, a.equals(b) == b.equals(a)). If B is a subtype of A, and A's equals method checks that the argument is an instanceof A, and B's equals method checks that the argument is an instanceof B, it is quite likely that the equivalence relation defined by these methods is not symmetric.

子类覆盖了父类中的equals方法,而两个类中都是用了instanceof来判断两个对象是否相等,这个操作存在风险。equals方法应该具有对称性(a.equals(b) == b.equals(a)),但是当B是A的子类时,A的equals方法检查参数是A的实例,B的equals方法检查参数是B的实例,则这些方法定义的equal关系很可能不是对称的。

c. 解决方法

@EqualsAndHashCode(callSuper = true)
@Data // 🐞 
public class AliCodeUrlReq extends PayReq implements Serializable {
    ...
}

此注解会生成equals(Object other)hashCode()方法。通过callSuper=true让其生成的方法中调用父类的对应方法。

1.2 问题代码②

a. 问题代码

public static Map<String, String> paraFilter(Map<String, String> sArray) {
        Map<String, String> result = new HashMap<>(sArray.size());
        if (sArray == null || sArray.size() <= 0) { // 🐞 sArray == null
            return result;
        }
    	...
}

b. 问题描述

Nullcheck of value previously dereferenced
A value is checked here to see whether it is null, but this value can't be null because it was previously dereferenced and if it were null a null pointer exception would have occurred at the earlier dereference. Essentially, this code and the previous dereference disagree as to whether this value is allowed to be null. Either the check is redundant or the previous dereference is erroneous.

翻译:对一个值进行了判空校验,但是这个值前面已经被引用过了,不可能是null了。

c. 解决方法

    public static Map<String, String> paraFilter(Map<String, String> sArray) {
        if (CollectionUtils.isEmpty(sArray)) {
            return Collections.emptyMap();
        }
        Map<String, String> result = new HashMap<>(sArray.size());
        ....
    }

1.3 问题代码③

a. 问题代码

	List<String> PAY_TYPES = Arrays.asList(
            TransTypeEnum.Ali.getVal(),
            TransTypeEnum.WeChat.getVal(),
        	// ...
    );
    private ***Resp func(***Req req) {
        /// ...
        TransTypeEnum tempType = TransTypeEnum.of(val);
        // 是否包含这种支付类型
        if (PAY_TYPES.contains(tempType.getVal())) { 🐞 
            // ...
        }
        // ...
    }

b. 问题描述

No relationship between generic parameter and method argument

PAY_TYPESList<String>类型的,contains()方法传入的值是 TransTypeEnum类型的,List#contains(Object o)虽然编译不会报错,但是类型不同,判断结果肯定不是我们要的。

这个在我看来是蛮严重的错误了,一般来说现在的IDE在你开发的时候就会提醒你这类错误。

c. 解决方法

private ***Resp func(***Req req) {
    /// ...
    TransTypeEnum tempType = TransTypeEnum.of(val);
    // 是否包含这种支付类型
    if (PAY_TYPES.contains(tempType.getVal())) {
        // ...
    }
    // ...
}

二、Dodgy code 糟糕的代码(可能会有bug的代码)

2.1 问题代码①

a. 问题代码

	public static void delFile(String filepath) throws IOException {
		File f = new File(filepath);
		if (f.exists() && f.isDirectory()) {
			if (f.listFiles().length == 0) { 🐞  // f.listFiles() might return null value
				f.delete(); 🐞  // return true/false should be checked
            

b. 问题描述

写这段代码的大兄弟很厉害,一下两个bug。

Possible null pointer dereference due to return value of called method

调用方法listFiles() 可能会返回 null值,这种情况下,如果直接f.listFiles().length这样子调用的话可能会出现NPE

Method ignores exceptional return value

f.delete()返回的是true/false,这个返回值应该是要被校验的。比如这段代码的意图是删除文件,f.delete()返回的布尔值就应该被校验,要么return 给上层方法。让调用者判断。不然有没有删除成功都不知道。

c. 解决方法

	public static boolean delFile(String filepath) throws IOException {
		File f = new File(filepath);
		if (f.exists() && f.isDirectory()) {
			File[] delFile = f.listFiles();
			if (delFile == null) {
				return f.delete();
        	...

2.2 问题代码②

a. 问题代码

    public String genUnSignatureStr(Map<String, Object> data, String secretKey) {
        StringBuilder sb = new StringBuilder();
        Set<String> keySet = data.keySet();
        // 这里也有问题,下面两句可以合成一句话,用构造方法直接初始化即可 🐞
        List<String> keyList = new ArrayList<>();
        keyList.addAll(keySet);
        Collections.sort(keyList);//将所有请求字段名称按ASCII码从小到大排序
        for (String s : keyList) {
            if (SignatureField.equals(s)) {//Signature字段不参与运算
                continue;
            }
            Object obj = data.get(s);
            if (obj != null) {//空值不参与运算
                if (obj instanceof String) {
                    if (!"".equals(obj)) {
                        sb.append(obj).append("_");
                    }
                } else if (obj instanceof Map) {
                    genUnSignatureStr((Map<String, Object>) obj, secretKey); // 🐞
                } else if (obj instanceof List) {
                    List<?> listObj = (List<?>) obj;
                    for (Object o : listObj) {
                        // 这里有两个很明显的bug
                        // 1. return value ignored 2. ClassCastException
                        genUnSignatureStr((Map<String, Object>) o, secretKey); // 🐞🐞
                    }
                } else {
                    sb.append(String.valueOf(obj)).append("_");
                }
            }
        }
        sb.append(secretKey);
        return sb.toString();
    }

b. 问题描述

Return value of method without side effect is ignored

返回值被忽略了,并且调用方法内部没有产生其他的影响,即没有对上层方法的变量值进行任何修改。

c. 解决方法

public String genUnSignatureStr(Map<String, Object> data, String secretKey) {
        StringBuilder sb = new StringBuilder();
        Set<String> keySet = data.keySet();
        List<String> keyList = new ArrayList<>(keySet);
        //将所有请求字段名称按ASCII码从小到大排序
        Collections.sort(keyList);
        for (String s : keyList) {
            //Signature字段不参与运算
            if (SignatureField.equals(s)) {
                continue;
            }
            Object obj = data.get(s);
            //空值不参与运算
            if (obj != null) {
                if (obj instanceof String) {
                    if (!"".equals(obj)) {
                        sb.append(obj).append("_");
                    }
                } else if (obj instanceof Map) {
                    String temp = genUnSignatureStr((Map<String, Object>) obj, secretKey);
                    sb.append(temp).append("_");
                } else {
                    sb.append(String.valueOf(obj)).append("_");
                }
            }
        }
        sb.append(secretKey);
        return sb.toString();
    }

2.3 问题代码③

a. 问题代码

public Map<String, String> close(...) {
        Map<String, String> result = new HashMap<>();
		//...
        result = xxxService.closeOrder(...); // 🐞
		...
}

b. 问题描述

Dead store to local variable
新创建了一个对象,但是这个内部变量并没有被使用。result后面又用swiftpassService#closeOrder()又初始化了一次。前面的new HashMap<>()对象就白白创建了。

c. 解决方法

public Map<String, String> close(...) {
        Map<String, String> result;
		//...
        result = xxxService.closeOrder(...); 
		...
}

2.4 问题代码④

a. 问题代码

        PayResp payResp;
        if (Objects.equals(channel, PayChannelEnum.Native.getValue())) {
            payResp = new PayNativeManager().scanPay(req, record);
        } else {
            return this.genScanpayResult(req, "xxx");
        }
        if (payResp == null) { // 🐞 payResp must not be null here
            return this.genScanpayResult(req, "xxx");
        }
        return payResp;

b. 问题描述

Redundant nullcheck of value known to be non-null
对肯定不会为null的对象进行空校验。

c. 解决方法

        AliScanPayResp payResp;
        if (Objects.equals(channel, PayChannelEnum.Native.getValue())) {
            payResp = new PayNativeManager().scanPay(req, record);
        } else {
            return this.genScanpayResult(req, "xxx");
        }
        return payResp;

三、Performance 性能

3.1 问题代码①

a. 问题代码

 private String doGetSignStr(Document xmlDoc, String[] signXPaths) {
        String signStr = "";
        for (String xpath : signXPaths) {
            if (xmlDoc.selectSingleNode(xpath) == null) {
                continue;
            }
            String temp = xmlDoc.selectSingleNode(xpath).getText() == null ? "" : xmlDoc.selectSingleNode(xpath).getText();
            signStr = signStr + temp + "_"; // 🐞
        }
        if (!"".equals(signStr)) {// 去掉最后一个"_"
            signStr = signStr.substring(0, signStr.length() - 1);
        }
        return signStr;
    }

b. 问题描述

for 循环中 使用 "+" 连接字符串
经过编译器优化之后,编译后的代码,在for循环中,每次都是new了一个StringBuilder,然后再把String转成StringBuilder,再进行append

频繁的新建对象要耗费很多时间,还会造成内存资源的浪费。

c. 解决方法

private String doGetSignStr(Document xmlDoc, String[] signXPaths) {
        StringBuilder signStrBuilder = new StringBuilder();
        for (String xpath : signXPaths) {
            if (xmlDoc.selectSingleNode(xpath) == null) {
                continue;
            }
            String temp = xmlDoc.selectSingleNode(xpath).getText() == null ? "" : xmlDoc.selectSingleNode(xpath).getText();
            signStrBuilder.append(temp).append("_");
        }
        String signStr = signStrBuilder.toString();
        if (!"".equals(signStr)) {
            // 去掉最后一个"_"
            signStr = signStr.substring(0, signStr.length() - 1);
        }
        return signStr;
    }

3.2 问题代码②

a. 问题代码

public <T> Map<String, T> hgetallAndDeserializationForMap(String hkey) {
        Map<String, T> resultMap = new HashMap<>();
        Map<byte[], byte[]> add = jedisDao.hgetall(hkey.getBytes(Charset.forName(Protocol.CHARSET)));
        Set<byte[]> keys = add.keySet(); // 🐞
        for (byte[] bytes : keys) {
            resultMap.put(new String(bytes, Charset.forName(Protocol.CHARSET)),
                    KryoUtils.readClassAndObject(add.get(bytes)));
        }
        return resultMap;
    }

b. 问题描述

Inefficient use of keySet iterator instead of entrySet iterator
keySet方式遍历Map的性能不如entrySet性能好。

c. 解决方法

public <T> Map<String, T> hgetallAndDeserializationForMap(String hkey) {
        Map<String, T> resultMap = new HashMap<>();
        Map<byte[], byte[]> add = jedisDao.hgetall(hkey.getBytes(Charset.forName(Protocol.CHARSET)));
        Set<Map.Entry<byte[], byte[]>> entrySet = add.entrySet();
        for (Map.Entry<byte[], byte[]> entry : entrySet) {
            resultMap.put(new String(entry.getKey(), Charset.forName(Protocol.CHARSET)),
                    KryoUtils.readClassAndObject(entry.getValue()));
        }
        return resultMap;
    }

四、Multithreaded correctness

4.1 问题代码①

a. 问题代码

private static Singleton instance = null;  // 🐞
	
public static Singleton getSingleton() {
    if (instance == null) {                         //Single Checked
        synchronized (Singleton.class) {
            if (instance == null) {                 //Double Checked
                instance = new Singleton();
            }
        }
    }
    return instance ;
}

b. 问题描述

Possible double check of field
这实际上是单例模式中的双重校验锁的知识点。

这里使用的双重检验锁 来确保多线程情况下只创建一个 instance 对象。

  1. 给 instance 分配内存
  2. 调用 Singleton 的构造函数来初始化成员变量
  3. 将instance对象指向分配的内存空间(执行完这步 instance 就为非 null 了)

但是在 JVM 的即时编译器中存在指令重排序的优化。也就是说上面的第二步和第三步的顺序是不能保证的,最终的执行顺序可能是 1-2-3 也可能是 1-3-2。如果是后者,则在 3 执行完毕、2 未执行之前,被线程二抢占了,这时 instance 已经是非 null 了(但却没有初始化),所以线程二会直接返回 instance,然后使用,然后顺理成章地报错。

c. 解决方法

private volatile static Singleton instance; 

使用 volatile 的主要原因是其另一个特性:禁止指令重排序优化。也就是说,在 volatile 变量的赋值操作后面会有一个内存屏障(生成的汇编代码上),读操作不会被重排序到内存屏障之前。比如上面的例子,取操作必须在执行完 1-2-3 之后或者 1-3-2 之后,不存在执行到 1-3 然后取到值的情况。从「先行发生原则」的角度理解的话,就是对于一个 volatile 变量的写操作都先行发生于后面对这个变量的读操作(这里的“后面”是时间上的先后顺序)。

实际上单例有更好的做法。延伸阅读:http://wuchong.me/blog/2014/08/28/how-to-correctly-write-singleton-pattern/

结语

本篇文章暂时整理到这里,由于个人遇到的错误类型有限,并且部分问题是我觉得不典型、没有意义放上来的。所以这里放一下我在网上找到的感觉比较全的FindBugs错误描述

技术手册:http://findbugs.sourceforge.net/manual/index.html

更多请参见官网:http://findbugs.sourceforge.net/bugDescriptions.html