tgwadm接入coverity告警案例分享

ゝ一世哀愁。 2022-04-01 04:01 299阅读 0赞

### 文章目录 ###

*   *  静态代码分析
     *   *  Coverity
     *  静态检查告警案例
     *   *  静态检查问题总览
         *  案例1（内存泄露）
         *  案例2（内存泄露）
         *  案例3（变量未初始化）
         *  案例4（内存越界）
         *  案例5（无效的sizeof）
     *  Coverity使用最佳实践
     *  总结
     *  参考文章：

> 本文适合不了解代码静态检查的初学者或者犹豫是否将项目接入静态检查的人阅读

> tgwadm是云网关组转发模块的agent组件，其主要功能有配置下发、与报文转发进程交互等功能。最新版本由于将其重构，代码变化很大，单纯靠有限时间内的人肉测试以及codereview，可以发现的问题有限，故尝试接入公司内部代码检查平台来检查代码中的比如死锁、内存问题等常见但难以发现的问题。

## 静态代码分析 ##

静态代码分析是指无需运行被测代码，通过词法分析、语法分析、控制流、数据流分析等技术对程序代码进行扫描，找出代码隐藏的错误和缺陷，如参数不匹配，有歧义的嵌套语句，错误的递归，非法计算，可能出现的空指针引用等等。统计证明，在整个软件开发生命周期中，30% 至 70% 的代码逻辑设计和编码缺陷是可以通过静态代码分析来发现和修复的。  
在C++项目开发过程中，因为其为编译执行语言，语言规则要求较高，开发团队往往要花费大量的时间和精力发现并修改代码缺陷。所以C++ 静态代码分析工具能够帮助开发人员快速、有效的定位代码缺陷并及时纠正这些问题，从而极大地提高软件可靠性并节省开发成本。  
**静态代码分析工具的优势 ：**

*  自动执行静态代码分析，快速定位代码隐藏错误和缺陷。
 *  帮助代码设计人员更专注于分析和解决代码设计缺陷。
 *  减少在代码人工检查上花费的时间，提高软件可靠性并节省开发成本。

![在这里插入图片描述][watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70]

> 带着问题来看：静态分析找到的Bug中，有多少是能够在Code Review（代码审查）、自验证或系统测试过程被发现的？

本组项目使用的是公司内部的代码检查平台，包括Coverity、Klocwork、PyLint等检查工具，支持的工具列表如下：

![在这里插入图片描述][watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 1]

由于本组主要开发语言为C/C++，经过一段时间试用，综合对比后，个人认为最适合本组而且又最好用的就是Coverity。

### Coverity ###

Coverity公司是由一流的斯坦福大学的科学家于2002年成立的，产品核心技术是1998年至2002年在斯坦福大学计算机系统实验室开发的，用于解决一个计算机科学领域最困难的问题，在2003年发布了第一个能够帮助Linux、FreeBSD等开源项目检测大量关键缺陷的系统，Coverity是唯一位列IDC前10名软件质量工具供应商的静态分析工具厂商，被VDC评为静态源代码分析领域的领导者。

> Coverity做了近15年，才基本做到精确(基本的意思是还有大概15%的误报率，当然这个数字在业内可以说是顶尖的)。

**Coverity主要从以下四个方面来对代码进行分析**

*  编译器告警
 *  编码规范
 *  静态分析
 *  数据流分析

具体数据可以参考WeTest团队的一篇《C++代码质量扫描主流工具深度比较》文章，地址看最后的参考

> 下面看下使用Coverity扫描本项目的告警情况，用数据说话

## 静态检查告警案例 ##

### 静态检查问题总览 ###

优先级分类：

![在这里插入图片描述][watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 2]

严重缺陷分类：

![在这里插入图片描述][watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 3]

一般缺陷分类：

![在这里插入图片描述][watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 4]

解决的典型问题及分析：  
（1） RESOURCE\_LEAK 查找程序可能存在的内存泄漏、指针泄露等问题，避免系统崩溃；  
（2） OVERRUN 检测内存越界访问等问题。它可以检测堆缓冲区和栈缓冲区的越界情况。内存越界问题很严重。  
（3） UNINIT 查找使用变量/对象时存在的未初始化的情况，使用未初始化的变量可能会导致无法预测的行为或程序崩溃；  
（4） UNINIT\_CTOR 未初始化的成员变量，风险与UNINIT类似；  
（5） CHECKED\_RETURN 检测代码忽略处理系统调用返回的错误代码等情况，忽略返回的函数错误代码并假设运算成功可能导致异常。  
（6） MISSING\_BREAK 查找 switch 语句中缺少 break 语句的情况，缺少break可能会导致不可预测的行为，增加代码理解难度，而且在后续代码维护过程中容易出错。  
（7） NEGATIVE\_RETURNS 查找滥用负整数的情况，负整数和可能为负的函数返回值在使用（例如作为数组索引、循环边界、代数表达式变量或系统调用的大小/长度参数）之前必须进行检查。滥用负整数可能导致内存损坏、进程崩溃、无限循环、整数溢出和安全缺陷。比如使用负值当做数组下标。  
（8） UNREACHABLE 检测代码中的DEAD CODE，不会跑到的代码增加后续维护成本，且由于一直无法运行，其可用性未知。  
（9） SLEEP 检测在持有锁/互斥锁时调用了sleep函数的情况。这会导致其他线程无法获得该锁，可能导致死锁或者性能下降。  
（10） NULL\_RETURNS 检测C/C++代码中指针或引用是否可能会为NULL值，检测是否会存在操作空指针操作。  
（11） CONSTANT\_EXPRESSION\_RESULT 检测常量中的运算符混淆、优先级混淆、类型大小或者复制/粘贴错误等问题。  
（12） SIZEOF\_MISMATCH 和 BAD\_SIZEOF 检测不匹配的sizeof操作或者memset时不匹配的对象大小，可能会导致memset会不足或者memset越界。

> 下面挑几个比较经典的错误拿出来讲一讲

### 案例1（内存泄露） ###

int CBinlogMgr::BinlogSetMirIPRecover(map<IPAddr, MirrorIP *> *ip_list,
        char *curr)
    {
        int i, num;
        be32 ip;
        MirrorIP *mip = new MirrorIP;
    
        ip_list->clear();
        GETINT(num);
        for(i = 0; i < num; ++ i){
            mip = new MirrorIP;
            if(! mip){
                return -1;
            }
            GETBE32(ip);
            mip->addr = ip;
            ip_list->insert(make_pair(ip, mip));
        }
    
        return 0;
    }

**Coverity案例**  
RESOURCE\_LEAK 查找程序没有尽快释放系统资源的情况。没有释放所需资源的应用程序可能面临性能降级、崩溃、拒绝服务或无法成功获取指定资源  
这段代码声明mip变量时，申请了一段内存，在后续循环中，还未释放相应内存便让mip变量指向了新申请的内存。  
**影响分析**  
内存泄露  
**修复方法**  
删除变量定义代码中，`MirrorIP *mip = new MirrorIP;`中的申请内存部分。

### 案例2（内存泄露） ###

int Config::SetSgList(ReqTVSAdminSetSgList *req,
        RspTVSAdminSetSgList *rsp)
    {
        SgRuleList *sg_list;
        SgRule *sg, *rsp_sg;
        int success, total, ret;
        ...
    
        asn_list_for_each(sg_list, sg) 
        {
            ++ total;
            ...
            
            TvsSecureGroup *tvssg = new TvsSecureGroup(sg);
            if (!tvssg)
            {
                LOG("alloc Sg rule %s failed.\n", (char *)sg->sgid);
                continue;
            }
            ret = g_stAclMgr.SetAclToUmod(tvssg);
            if (ret) 
            {
                LOG("Set Sg rule %s failed.\n", tvssg->id.c_str());
            } 
            else 
            {
                ++ success;
                g_stAclMgr.SaveAclToAdmin(tvssg->id, tvssg);
                g_stBinlogMgr.BinlogSetSg(tvssg);
            }
            ...
        }
        ...
        return 0;
    }

**Coverity案例**  
属于RESOURCE\_LEAK问题  
本代码在循环中，在`ret = g_stAclMgr.SetAclToUmod(tvssg);if (ret)`分支中，若是false分支，会将tvssg指向的内存放入一个队列，会在其他位置释放；但若进入另一个分支，则不会将tvssg指向的内存释放，在下一个循环中，tvssg指向新的内存，导致内存泄露  
**影响分析**  
内存泄露  
**修复方法**  
在异常分支增加释放内存逻辑

### 案例3（变量未初始化） ###

int Config::SetFldList(
        ReqTVSAdminSetFldList *req, RspTVSAdminSetFldList *rsp)
    {
        ...
        int op_type, end_ret, del_count, count, set_count;
        ...
        if (FLD_OP_UNBAN == op_type) {
            ...
            for (list_iter = m_tmpDelFldList.begin();
                list_iter != m_tmpDelFldList.end(); ++ list_iter) {
                ret = DelFldListFromUmod(*list_iter);
                if (ret) {
                    ...
                    goto FAILED;
                } 
                ++ del_count;
            }
            SetFldFlagToUmod(m_tmpDelFldList, DEL);
        }
        ...
        set_count = 0;
        ...
    
    FAILED:
        ...
        count = 0;
        for (iter = m_tmpModifyFldMap.begin();
            iter != m_tmpModifyFldMap.end() && count < set_count; ++ iter) {
            
            ...
            ++ count;
        }
        ...
    }

**Coverity案例**  
该错误也是变量未初始化错误，但为什么把该错误当做典型举出来？  
虽然，变量没有初始化cpplint也可以检查出来，但是cpplint很容易误报，而且要求很严，cpplint是直接扫描代码，只要它发现变量未初始化就会给你报错。  
但是coverity不一样，它会分析所有可能的值，然后根据代码逻辑遍历所有分支，分析是否存在使用未初始化值的情况，所以相较于cpplint更为精确。  
在这段代码里面，可能最初编写的代码并没有这个问题，随着后续对该代码的修改，在原有的逻辑上增加了更多的逻辑以及跳出清理资源逻辑后，如果编码不够严谨规范（即声明时即初始化），就会很容易产生这样的逻辑问题，即可能一个值还没有进行初始化，便跳出了，但在异常处理逻辑中还用到了该未初始化的值，导致更多的异常。  
set\_count在初始化为0之前，存在可能跳到异常逻辑，在异常逻辑中用到了该值作为循环，由于它未初始化，是一个不确定的值，导致循环异常，程序崩溃。  
**影响分析**  
set\_count在初始化为0之前，存在可能跳到异常逻辑，在异常逻辑中用到了该值作为循环，由于它未初始化，是一个不确定的值，导致循环异常，程序崩溃。  
**修复方法**  
声明set\_count时进行初始化

### 案例4（内存越界） ###

int CIfManager::SendARP(int if_idx, unsigned char* hwaddr, string &src, unsigned int dst)
    {
        struct ethhdr* eth;
        struct arphdr* arph;
        char buffer[42];
        char* arp_ptr;
        int af;
        unsigned int vip;
        in6_addr addr6;
    
        af = get_ip_af(src.c_str());
        /*build ethernet header*/
        eth = (struct ethhdr*)buffer;
        ...
        
        /*build arp header*/
        arph = (struct arphdr*)(buffer + sizeof(struct ethhdr));
        ...
        
        arp_ptr = (char*)(arph + 1);
        memcpy(arp_ptr, hwaddr, 6);
        arp_ptr += 6;
        if (AF_INET == af)
        {
            vip = inet_addr(src.c_str());
            memcpy(arp_ptr, &vip, sizeof(vip));
            arp_ptr += sizeof(vip);
        }
        else
        {
            int ret = get_ipv6_addr(src.c_str(), (struct gw_in6_addr *)&addr6);
            if (ret)
            {
                ...
                return -1;
            }
            memcpy(arp_ptr, &addr6, sizeof(addr6));
            arp_ptr += sizeof(addr6);
        }
        memset(arp_ptr, 0x0, 6);
        arp_ptr += 6;
        memcpy(arp_ptr, &dst, sizeof(dst));
        ...
        return 0;
    }

**Coverity案例**  
OVERRUN 可查找越界访问缓冲区的很多情况。不当的缓冲区访问可能损坏内存，导致进程崩溃、安全漏洞和其他严重的系统问题。OVERRUN 可查找到堆缓冲区和栈缓冲区的越界索引。  
这段代码对buffer进行操作，填充ARPheader字段。  
buffer本身长度为42字节，`char buffer[42];`  
第一处偏移`arph = (struct arphdr*)(buffer + sizeof(struct ethhdr));`，arph现在指向buffer的字节14  
第二处偏移`arp_ptr = (char*)(arph + 1);`；arp\_ptr现在指向buffer的字节22  
第三处偏移`arp_ptr += 6;`，arp\_ptr现在指向buffer的字节28  
需要进入ipv6分支，即`if (AF_INET == af)`的else分支，第四处偏移`memcpy(arp_ptr, &addr6, sizeof(addr6));arp_ptr += sizeof(addr6);`，此处已经产生了内存越界，此时arp\_ptr偏移在44  
第五处偏移

memset(arp_ptr, 0x0, 6);
        arp_ptr += 6;
        memcpy(arp_ptr, &dst, sizeof(dst));

这里偏移了6个字节和4个字节，arp\_ptr已经偏移到54字节  
**影响分析**  
若从src获取出来的IP协议类型不是ipv4的话，便会发生内存越界错误，可能导致进程崩溃或者其他严重问题。  
**修复方法**  
将buffer大小修改为大于54字节，此处修改为60字节大小

### 案例5（无效的sizeof） ###

int Config::ReadBanFldList()
    {
        int ret;
        char *fld_buffer;
        ...
        fld_buffer = (char *)malloc((MAX_FLD_NUM_PER_SVC + 1) * HTTP_HOST_LEN);
        ...
        ret = 0;
        for (vector<string>::const_iterator it = fld_list.begin();
            it != fld_list.end(); ++ it) {
            ...
            memset(fld_buffer, 0, sizeof((MAX_FLD_NUM_PER_SVC + 1) * HTTP_HOST_LEN));
            ...
            }
        ...
    }

**Coverity案例**  
BAD\_SIZEOF 可报告在参数是可疑类别（例如对象的地址，通常应该是实际对象的大小）之一时使用 sizeof 运算符的情况。非正常大小值可能导致各种问题，例如分配不足或过量、缓冲区越界访问、部分初始化或复制以及逻辑不一致。  
fld\_buffer大小为(MAX\_FLD\_NUM\_PER\_SVC + 1) \* HTTP\_HOST\_LEN，在memset时，用的是sizeof((MAX\_FLD\_NUM\_PER\_SVC + 1) \* HTTP\_HOST\_LEN)，错误使用了sizeof  
**影响分析**  
memset实际上算是未生效，只初始化了fld\_buffer的4~8个字符  
若后续逻辑代码依赖于fld\_buffer初始化，可能会有问题  
**修复方法**  
将代码改为`memset(fld_buffer, 0, (MAX_FLD_NUM_PER_SVC + 1) * HTTP_HOST_LEN);`

## Coverity使用最佳实践 ##

这里的最佳实践是我从其他人的经验分享整理得来，也与大家分享一下。

*  1、每日凌晨自动扫描代码，若有问题自动提单
 *  2、告警处理人查看新增告警，明显问题直接修复，涉及到业务逻辑负责问题则交由相关模块负责人负责修复。
 *  3、先进行codereview再进行提交

## 总结 ##

通过前面的一些例子，发现了很多常见或疑难的错误，足以说明Coverity静态检查工具的功能强大。

确实，Coverity静态检查检查出来的问题有些由于逻辑分支条件非常难以满足，导致不会出现相应问题，不会触发bug。但一旦进入，便要花费上十倍百倍的时间来复现定位。

开发流程中使用静态检查，确实可能会增加部分开发成本，但是在后续维护以及稳定性上，有绝对的好处；另一方面，有了静态检查来专注于检查常见编码错误，codereview便可以更加专注于业务逻辑，也算是变相提高了开发效率。

> 使用好的静态检查检查工具来检查编码常见错误，解放codereview，让codereview专注于业务逻辑检查，提升效率  
> 降低维护成本，提升程序稳定性

## 参考文章： ##

1、【代码质量】C++代码质量扫描主流工具深度比较  
[https://blog.csdn.net/wetest\_tencent/article/details/51516347][https_blog.csdn.net_wetest_tencent_article_details_51516347]

[watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70]: /images/20220401/0dc868db9d9842db9829fe3b1896c801.png
[watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 1]: /images/20220401/3993295313734abd99d69a9c5851a480.png
[watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 2]: /images/20220401/4ad7275fcead4a46ae0e74bcac0bf51b.png
[watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 3]: /images/20220401/d01a3e8ea5bb46aa83c94e6a3091c041.png
[watermark_type_ZmFuZ3poZW5naGVpdGk_shadow_10_text_aHR0cHM6Ly9ibG9nLmNzZG4ubmV0L3FxXzE1NDM3NjY3_size_16_color_FFFFFF_t_70 4]: /images/20220401/19f6ef5ff78b47dc9dd8e02db1e3f680.png
[https_blog.csdn.net_wetest_tencent_article_details_51516347]: https://blog.csdn.net/wetest_tencent/article/details/51516347