安全代码的一些小tip

历经一年多的flex+java游荡,总算又回归linux下C/C++开发,虽然这一年中曾经客串写过daemon,但是看到大段大段的C++ code,仍然有种事过境迁激动到要内牛满面的地步。好久不碰是一方面,基础差也是一方面。最近修了点Klockwork扫出的bug,看到一点有趣的东西,记下来备忘。

Klockwork扫出的问题很大一部分是buffer overrun的问题,基本上是因为不安全API引起的。比如说下面的这几个经典API就应该替换成右边对应的safe API(以下来自飞哥http://zoufeiblog.appspot.com/的经典总结):

    1. sprintf –> snprintf
    2. strcat –> strncat
    3. strcpy –> strncpy
    4. sscanf –> fgetc
    5. system –> fork + exec
    6. execvp –> execve

但是在修的过程中,发现几个很好玩的特例。

    1. sscanf
    2. snprintf
    3. new
    4. stl iterator

sscanf

看到两个特例,没有替换API的提要。一个是用format字符串限制字符长度,

char * src = "abc 123";
char key[3] = {0};
int count = 0;
sscanf(src, "%2s %d", &key, &count);

这种写法可以避免buffer overrun,但是不能保证数据的正确性。下一个特例就非常经典,对sscanf结果的错误判断很tricky

int TmDbConnectImpl::getColValInt(const char* szColName)
{
        const char* szBuf = getColVal(szColName);
        if (NULL == szBuf)
                throw "Wrong column name";

// Check the numeric string is valid
        int  iValue=0;
        char cBad;

switch(sscanf(szBuf, "%d%c", &iValue, &cBad))
        {
        case 1: // One numeric value matched, data is valid.
                break;
        case 2:
                LogPrintf("Invalid character = %c\n", cBad);
        default:
                LogPrintf("Could not convert string to int, column name:[%s], value:[%s]",
                          szColName, szBuf);
        //throw "Could not convert data to number";
       }
}

当然其实上面这段code可以用strtol来代替。

snprintf

这是个safe API没错,但在windows下面_snprintf不能保证null terminated,所以需要_snprintf(buf, sizeof(buf) – 1, "%s", value);而在linux下面snprintf又是null terminated,所以snprintf(buf, sizeof(buf), "%s", value)。

为了达到平台统一性,在linux下面是不是也写作snprintf(buf, sizeof(buf)-1, "%s", value)好了呢,可以。但是也发现一个特例,

mechstr = (char*) malloc(strlen(mechanism) + 5);
if (!mechstr)
    return 0;
sprintf(mechstr, "{ %s }", mechanism);

字符数组分配的长度刚刚好,如果用-1的写法的话,除去snprintf为null terminator保留的一个字符外,还会丢掉一个字符,那么得到的结果是不正确的。所以最后的修法是,

snprintf(mechstr, len, "{ %s }", mechanism);
mechstr[len - 1] = ”;

强制末尾置0,以防万一,就算在windows下面也没有问题。

除此之外,还看见下面一个避开这个问题的解法:sprintf((char *)(SessionKey + i*2), "%2.2hx", HA1[i]),有人能看懂这个sprintf的意图么

new

一个常识是,new完了要检查内存分配是否成功。在大学的时候老师说的是用指针为不为空来判断,但这个是不准确的。在1993年前,C++一直要求在内存分配失败时operator new要返回0。但是现在是要求抛出std::bad_alloc异常。

所以检查new是否成功的方式不是Object *p = new Object; if (p == NULL) xxx,而是

try {
        Object *p = new Object;
        p->name = "test";
}
catch (bad_alloc& ba) {xxx}

对于malloc,仍然用NULL进行判断

stl iterator

对于stl的iterator,一般的用法是for(vector::iterator p = objectVector->begin(); p != objectVector->end(); p++)。在这个例子中需要注意的是,如果在循环体内对objectVector里的Object进行了删除的话,那么p指针可能失效,导致死循环。

问题是,这样的写法for(vector::iterator p = objectVector->begin(); p end(); p++)对不对。还有一个问题是,下面的这个函数,里面的iterator用的对不对Smile 相当tricky的用法

 

int getAllParentGroupsByMember(const string &user_filter, ldap_set &groups)
{
     std::string szDn;
    std::string filter = user_filter;
   
    std::vector<std::string>::iterator iGroupQ;
    std::vector<std::string> groupQ;
   
    //reserve memory for vector to avoid iterator invalidation after inserting or deleting
    //plus, it adds efficiency
    groupQ.reserve(128);
    groupQ.clear();
   
    //iterate from the first member in the group queue
    iGroupQ = groupQ.begin();
 
    do
    {       
        vector< vector<string> > *res = 0;
        vector< vector<string> >::iterator p;
        vector<string>::iterator q;
       
        int rc = getEntityAttrs(filter, xxx, res, xx);
        if(rc > 0 && res != NULL){   
              for(p = res->begin(); p != res->end(); p++)
              {
                for(q = (*p).begin(); q != (*p).end(); q++)
                { 
                    if(*q != "" && (*q).c_str() != NULL)
                    {
                         //check for duplicate group objects in ldap set
                         //Prefast:local declaration hides declaration in an outer space
                         ldap_set::iterator pset = groups.find(*q);
                         if(pset == groups.end())
                         {
                              //enqueue new group object found in the group queue
                              groupQ.push_back(*q);   
                              //insert this unique group object in the sorted ldap set
                              groups.insert(*q);
                           }
                    }   
                }
              }
        }
       
        freeQueryResults(res);
       
        //groupQ is empty, no need to do further search, bails out
        if(groupQ.empty())
            break;
           
        //magic number 128 is chosen based on common practice.
        if(groupQ.size() > 128){
            break;
        }
       
        if(iGroupQ != groupQ.end())
        {
            getEntityDn((xxxx, iGroupQ->c_str(), szDn, xxxx);
            filter = "(&(member=" + szDn + ")" + "(objectCategory=group))";
        }
       
    }while(iGroupQ++ != groupQ.end());
   
    if(groups.empty() || groupQ.empty())
        return 0;
       
    return 1;
}

这些是修klockwork扫描结果时看到的一部分有意思的东西,有不对的地方请指正。这也只是书写安全代码很小的一部分,有一本书《Writing Secure Code》,有心修炼的可以去看看。

另:贴下非线程安全的API(同样来自飞哥的总结,refer自http://pubs.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_09.html

unsafe_api