代码审查九句真言

看见了If，就想Else。

看见malloc，就去找Free。

函数调用要小心，需要看看返回值。

看到for循环，就找边界值。

看见return要注意，要去前面找资源。

看见数组把神提，问题往往在下标。

不要小看字符串，长度是个大问题。

得到函数不要急，看看变量初始化，各种路径要小心。

赋值函数最危险，变量没有初始化。

九句句真言不孤立，相互结合显神威。

真言详解
1.看见If，就想Else。

看到if语句，就要想到else语句。

如果没有else语句，就要分析是不需要，还是异常情况没有处理，如果是异常情况没有处理，可以提单。

2.看见malloc，就去找Free。

看到malloc语句分配了内存，立即停下正常走读，看malloc代码之后，是否在所有程序的返回分支中都有释放语句。

典型案例：
U32 DEV_IfSetSectionEnable(DEV_IF_T *pIfIns)
{
CHAR * pSectionName = NULL;
ULONG ulMsg[4];
/* 向配置文件发送消息DEV_IF_READ_SECTION, 进行配置下载 */
pSectionName = VOS_Malloc(MOD_DEV, MAX_INTERFACE_NAME_LEN+1);
if ( pSectionName != NULL )
{
VOS_strcpy(pSectionName,pIfIns->ifName);
ulMsg[0] = MID_DEV;
ulMsg[1] = MID_CFM;
ulMsg[2] = DEV_CFM_ENABLE_SECTION;
ulMsg[3] = (ULONG)pSectionName;
VOS_Que_Write(ulVRPQID_CFM , ulMsg, VOS_NO_WAIT, 0);
return SUCCESS;
}
return DEV_ERR_NOMEMORY;
}
该函数有了VOS_Malloc，但没有看到free。

于是在下面的代码中寻找，发现VOS_Que_Write中自动实现free功能。

代码看似没有问题，但我们发现VOS_Que_Write的返回值没有判断，如果VOS_Que_Write返回失败，free语句就没有被执行到。

这样可以确认该函数存在内存泄露隐患。

该代码最终修改：U32 DEV_IfSetSectionEnable(DEV_IF_T *pIfIns)
{
CHAR * pSectionName = NULL;
ULONG ulMsg[4];
ULONG rc;
/* 向配置文件发送消息DEV_IF_READ_SECTION, 进行配置下载*/
if(pIfIns == NULL || (VOS_strlen( pIfIns->ifName) > (MAX_INTERFACE_NAME_LEN+1)))
{
return DEV_ERR_GEN;
}
pSectionName = VOS_Malloc(MOD_DEV, MAX_INTERFACE_NAME_LEN+1);
if ( pSectionName != NULL )
{
VOS_strcpy(pSectionName,pIfIns->ifName);
ulMsg[0] = MID_DEV;
ulMsg[1] = MID_CFM;
ulMsg[2] = DEV_CFM_ENABLE_SECTION;
ulMsg[3] = (ULONG)pSectionName;
rc = VOS_Que_Write(ulVRPQID_CFM , ulMsg, VOS_NO_WAIT, 0);
if(rc != SUCCESS)
{
rc = VOS_Free(pSectionName);
VOS_DBGASSERT(rc == VOS_OK);
return DEV_ERR_GEN;
}
return SUCCESS;
}
return DEV_ERR_NOMEMORY;
}
3.函数调用要小心，需要看看返回值。

看到函数调用，要养成习惯，进入函数内部瞄一眼。

看看函数的正常值和异常值都是什么。

看看返回值需不需要判断。

看看有没有参数理解不一致的地方。

例如：
if ( VOS_strnicmp(szFullName, DEV_ATM_NAME , DEV_ATM_NAMELEN) == 0 )
{
ulIfType = DEV_GetIfTypeFromIfName( szFullName );
if ( ulIfType == -1 )
{
EXEC_OutString( ulExecID, "\r\nUnknown interface type" );
return VOS_ERR;
}
/*得到端口的索引*/
ulRet = DEV_GetIfIndexFromIfName( szFullName, &ulIfIndex);
if (SUCCESS != ulRet)
{
EXEC_OutString( ulExecID, "\r\nUnknown interface number" );
return VOS_ERR;
}
/*判断端口是否已经存在*/
pIfIns = DEV_GetIfFromIndex(ulIfIndex);
if(NULL == pIfIns)
{
rc = DEV_Cnsl_CreateIf(ulExecID, ulIfType, ulIfIndex, ulSubType);
if(SUCCESS != rc)
{
return SUCCESS;
}
pIfIns = DEV_GetIfFromIndex(ulIfIndex);
if(NULL == pIfIns)
{
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING, "pIfIns = NULL is invalid %s.%d"
, __FILE__, __LINE__);
return DEV_ERR_GEN;
}
函数使用-1作为非法值，而在DEV_GetIfTypeFromIfName函数中：
U32 DEV_GetIfTypeFromIfName(CHAR *ifName)
{
CHAR szIfType[20]; //接口的类型字符串
U32 ulIfType;
U32 strLen;
U32 i;
if(NULL ==ifName)
{
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING, "ifName = NULL in GetIfTypeFromIfName %s.%d", __FILE__, __LINE__);
return DEV_ERR_VALUE;
}
strLen = VOS_strlen(ifName);
if(0 == strLen)
{
/*字符串为空，返回错误*/
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING, "strlen = 0 %s.%d", __FILE__, __LINE__);
return DEV_ERR_VALUE;
}
VOS_Mem_Set(szIfType, 0, sizeof(szIfType));
/*---------------------------------------------------------*/
/*从字符串的尾部向前查找，直到找到第一个不是数字的字符 */
/*---------------------------------------------------------*/
for(i = strLen-1; i >= 0; i--)
{
/*字符不等于'.', '/'或数字字符时循环结束*/
if(ifName[i] != '.' && ifName[i] != '/'
&& (ifName[i] < '0' || ifName[i] > '9'))
break;
}
VOS_strncpy(szIfType, ifName, i+1);
ulIfType = DEV_IfStringToType( szIfType );
return ulIfType;
}
函数的一个错误返回值是DEV_ERR_VALUE，很显然，两边参数理解不一致。

4.看到for循环，就找边界值。

看到for循环，就要看看边界值是否合理。

如果循环变量是数组的下标，更加需要注意。

例如：U8* DEV_IfTypeToString(U32 ulIfType)
{
U32 i;
// change to user type
ulIfType = DEV_IfUserType(ulIfType);
for(i=0;ulIfType != -1 && i<DEV_USERIFTYPE_NUM
&& UserIfTypes[i].ulIfType!=-1;i++)
{
if (UserIfTypes[i].ulIfType == ulIfType)
return UserIfTypes[i].pIfTypeString;
}
return "unknown-interface";
}
函数中：
#define DEV_USERIFTYPE_NUM 9 // number of UserIfTypes, NOT DEV_IFTYPE_MAX_NUM!!!
而数组：
static IfTypeList_S UserIfTypes[] =
{
{ DEV_IFTYPE_ATM, "Atm", "ATM interface" ,3, 0, DEV_AATMIF_LOGIF_MAX_NUM},
#ifndef ISN_MODIFIED
//Modified by: liyanmin
//Data: 2001/1/3
//Description: 8850设备目前只管理ATM物理端口和ATM子接口?
{ DEV_ATM_TYPE_VPT, CLI_CMD_TEMPLET_IF_ATMVPT, "ATM VP tunnel", 4, DEV_AATMIF_LOGIF_MAX_NUM, 0 },
{ DEV_ATM_TYPE_VPRT, CLI_CMD_TEMPLET_IF_ATMVPRT, "ATM VP ring tunnel", 4, DEV_AATMIF_LOGIF_MAX_NUM, 0 },
#endif //end ISN_MODIFIED
{ DEV_ATM_TYPE_ATMSUB, "atm-sub", "ATM sub-interface for IP", 4, DEV_AATMIF_LOGIF_MAX_NUM, 0 }, #ifndef ISN_MODIFIED
{ DEV_IFTYPE_SONET, CLI_CMD_TEMPLET_IF_POS, "IP POS interface", 3, 0, 0 },
{ DEV_IFTYPE_FASTETHERNET, CLI_CMD_TEMPLET_IF_FASTETHERNET, "IP 10M/100M ethernet interface", 3, 0, DEV_AFE_VLAN_MAX_NUM },
{ DEV_IFTYPE_GIGABITETHERNET, CLI_CMD_TEMPLET_IF_GIGAETHERNET, "IP 1000M ethernet interface", 3, 0, DEV_AGE_VLAN_MAX_NUM },
{ DEV_IFTYPE_VT, "virtual-template", "IP PPP virtual templete", 1, 1, 0 },
{ DEV_IFTYPE_VE, CLI_CMD_TEMPLET_VIRTUAL_ETHERNET, "IP virtual ethernet for PPPOE", 3, 1, 0 }, //no { DEV_IFTYPE_L3IPVLAN, "vlan", "IP virtual LAN", 1, 0 },
#endif //end ISN_MODIFIED
{ -1,"","",0,0},
};
而数组定义中，去掉被注释掉的部分，数组长度只有3。

数组将严重越界。

5.看见return要注意，要去前面找资源。

看见return语句，尤其是函数中间的异常返回语句。

看到这种语句，就需要折回头去看看前面有没有分配资源。

前面分配的任何资源（包括内存，端口，等等），在异常返回处需要一并释放。

例如：
在PVC_vl.c中的ULONG AllocateVl(void *pMsgEnv)函数中,
if ( pValue->rxTDscrIndex)
{
rc = TD_IncreaseTDRefer(pValue->rxTDscrIndex);
if (rc)
{
#if PVC_APS_ENABLE == YES
APS_DecrProtectConf(void);
#endif
return PVC_TRAFF_ERROR;
}
newVlEntry.rxIndex = pValue->rxTDscrIndex;
}
if ( pValue->txTDscrIndex)
{
rc = TD_IncreaseTDRefer(pValue->txTDscrIndex);
if (rc)
{
TD_DecreaseTDRefer(pValue->rxTDscrIndex);
#if PVC_APS_ENABLE == YES
APS_DecrProtectConf(void);
#endif
return PVC_TRAFF_ERROR;
}
newVlEntry.txIndex = pValue->txTDscrIndex;
}
if (newVlEntry.txIndex && newVlEntry.rxIndex)
newVlEntry.direction = BI_DIRECTIONAL;
else if (newVlEntry.rxIndex)
newVlEntry.direction = RECEIVE;
else
newVlEntry.direction = TRANSMIT;
/* check virtual link self consistency */
rc = CheckVlSelfConsistency(&newVlEntry);
if (rc)
{
#if PVC_APS_ENABLE == YES
APS_DecrProtectConf(void);
#endif
TD_DecreaseTDRefer(newVlEntry.rxIndex);
TD_DecreaseTDRefer(newVlEntry.txIndex);
return rc;
}
/* create virtual link*/
newVlEntry.port = port;
newVlEntry.vpi = vpi;
newVlEntry.vci = vci;
newVlEntry.atmOperStatus = PVC_DOWN;
newVlEntry.atmAlarmStatus = PVC_NOALARM;
newVlEntry.oamSegEndPoint = PVC_NOT_ENDPT;
newVlEntry.oamEndEndPoint = PVC_NOT_ENDPT;
newVlEntry.status = ROW_STATUS_UNKNOWN;
newVlEntry.vlFlag = PVC_NONRING;
VOS_Tm_Get(&newVlEntry.date, &newVlEntry.time, &newVlEntry.msec);
/* check upc flag value */
if ( PVC_CHECK_UPCNPCFLAG(pValue->upc) )
return PVC_BAD_VALUE;
/*p2mpleaf禁止设置UPC*/
if ( newVlEntry.atmCastType == P2MPLEAF && pValue->upc == ENABLE)
return PVC_STATE_ERROR;
else
newVlEntry.atmConnUpcNpcEnb = pValue->upc;
红色代码处分配了资源，而在兰色代码处异常返回时没有被释放。

6.看见数组把神提，问题往往在下标。

函数中一旦出现数组，就要提起精神。

数组越界可是个致命问题。

例如：ULONG DEV_ExecNetSwitchShow(ULONG ulUserID)
{
NM_VALUE_T sValue;
U32 frmIndex;
U32 slotIndex;
U32 netIndex1, netIndex2;
U32 netstatus1 = DEV_OBJ_STATUS_INACTIVE;
U32 netstatus2 = DEV_OBJ_STATUS_INACTIVE;
DEV_FRAME_T* pFrame;
DEV_SLOT_T* pSlotIns;
DEV_SLOT_T* pNetIns1;
U32 rc;
U32 i, j; // Loop Variable
U8 pOutputStr[80]; // output info buffer
U32 ulNet[DEV_SLOT_ATMUNIT_MAX_NUM];
U8* pOutputStatus;
U8* pOutputType;
U8* pOutputApcNet[DEV_ASLT_SUB_MAX_NUM - 1];
U8 xidx, yidx, tmpPort;
U32 ulNetVer;
U32 ulSeq;
U32 ulLen;
void* pData; // pointer to msg received
U8 szBuf[256];
DEV_NET_SWPTMAP_T sNetSwptMap;
// check netboard
netIndex1 = DEV_CONV_SLT2SLTINDEX(NET_CARD1);
netIndex2 = DEV_CONV_SLT2SLTINDEX(NET_CARD2);
pNetIns1 = DEV_GetSlotFromIndex(netIndex1);
pNetIns2 = DEV_GetSlotFromIndex(netIndex2);
if (pNetIns1 == NULL && pNetIns2 == NULL)
{
EXEC_OutString(ulUserID, "\r\nnet board not found");
return SUCCESS;
}
if (pNetIns1 != NULL)
{
rc = DEV_SltGetSlotStatus(netIndex1, &sValue);
if (rc)
{
//error output
return DEV_ERR_GEN;
}
if (sValue.v_num != DEV_OBJ_STATUS_INACTIVE)
netstatus1 = DEV_OBJ_STATUS_ACTIVE;
}
if(pNetIns2 != NULL)
{
rc = DEV_SltGetSlotStatus(netIndex2, &sValue);
if (rc)
{
//error output
return DEV_ERR_GEN;
}
if (sValue.v_num != DEV_OBJ_STATUS_INACTIVE)
netstatus2 = DEV_OBJ_STATUS_ACTIVE;
}
if (netstatus1 == DEV_OBJ_STATUS_INACTIVE && netstatus2 == DEV_OBJ_STATUS_INACTIVE) {
EXEC_OutString(ulUserID, "\r\nall the net board are inacitve");
return SUCCESS;
}
// default frame number is 0
frmIndex = DEV_CONV_FRM2FRMINDEX(DEV_DEFAULT_FRAME);
pFrame = DEV_GetFrmFromIndex(frmIndex);
if (pFrame == NULL) // the frame doesn't exist!
{
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING, "pFrame = NULL %s.%d"
, __FILE__, __LINE__);
return DEV_ERR_GEN;
}
EXEC_OutString(ulUserID, "\r\nSlot Type State Apc0 Apc1 Apc2 Apc3");
EXEC_OutString(ulUserID, "\r\n---- ---- ----- ---- ---- ---- ----");
for (i = 0; i < DEV_AFRM_SLT_MAX_NUM; i++)
{
slotIndex = pFrame->slotArrayIndex[i];
if(slotIndex == DEV_OBJINDEX_NOEXIST)
{
continue;
}
// get slot status
rc = DEV_SltGetSlotStatus(slotIndex, &sValue);
if (rc)
{
//error output
return DEV_ERR_GEN;
}
pOutputStatus = slotstatus[sValue.v_num];
if (sValue.v_num != DEV_OBJ_STATUS_ACTIVE)
{
//slot is inactive
VOS_sprintf(pOutputStr, "\r\n%-4d %-4s %-5s",
i, // slot num
"", // slot type
pOutputStatus); // slot status
EXEC_OutString(ulUserID, pOutputStr);
continue;
}
rc = DEV_SltGetSlotType(slotIndex, &sValue);
pOutputType = slottype[sValue.v_num];
pSlotIns = DEV_GetSlotFromIndex(slotIndex);
if(pSlotIns == NULL)
{
return DEV_ERR_GEN;
}
if (pSlotIns->slotType == DEV_SLOTTYPE_NET)
{
continue;
}
else
{
DEV_SltGetApcNetBoardUsed(slotIndex, ulNet);
for (j = 0; j < DEV_SLOT_ATMUNIT_MAX_NUM; j++)
pOutputApcNet[j] = slotUsedNetBd[ulNet[j]];
}
红色代码中：
#define DEV_ASLT_SUB_MAX_NUM 2 //一个slot支持最大subslot数
红色数组定义的长度是1
兰色代码中：
#define DEV_SLOT_ATMUNIT_MAX_NUM 4 //每槽位最大ATM单元数
使用的下标到了4，数组严重越界。

7.不要小看字符串，长度是个大问题。

字符串往往是代码审查中不被重视的问题。

而字符串由于各个模块对其他模块的不了解，经常
出现随便定义一个字符串长度的现象。

例如：
U32 DEV_GetIfTypeFromIfName(CHAR *ifName)
{
CHAR szIfType[20]; //接口的类型字符串
U32 ulIfType;
U32 strLen;
U32 i;
if(NULL ==ifName)
{
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING
, "ifName = NULL in GetIfTypeFromIfName %s.%d", __FILE__, __LINE__);
return DEV_ERR_VALUE;
}
strLen = VOS_strlen(ifName);
if(0 == strLen)
{
/*字符串为空，返回错误*/
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING
, "strlen = 0 %s.%d", __FILE__, __LINE__);
return DEV_ERR_VALUE;
}
VOS_Mem_Set(szIfType, 0, sizeof(szIfType));
/*---------------------------------------------------------*/
/*从字符串的尾部向前查找，直到找到第一个不是数字的字符 */
/*---------------------------------------------------------*/
for(i = strLen-1; i >= 0; i--)
{
/*字符不等于'.', '/'或数字字符时循环结束*/
if(ifName[i] != '.' && ifName[i] != '/'
&& (ifName[i] < '0' || ifName[i] > '9'))
break;
}
VOS_strncpy(szIfType, ifName, i+1);
ulIfType = DEV_IfStringToType( szIfType );
return ulIfType;
}
入口参数ifName在调用方定义的长度是48，而szIfType的长度是20，因此，红色代码：
VOS_strncpy(szIfType, ifName, i+1);有越界可能。

8.得到函数不要急，看看变量初始化，各种路径要小心。

在拿到函数之后，要习惯性的看看所有的局部变量是在函数一开始就被初始化了如果有
变量没有被初始化，就要小心了。

如果发现变量是在if，for，while，等语句中被初始化
的，问题可能就来了。

U32 DEV_FrmSetClkSrcMsgProc(void *pMsg, U32 ulMsgLen, void *pObject)
{
DEV_MSG_ENV_T *pDevEnv;
DEV_FRAME_T *pFrame;
U32 rc, rc1, rc2, ulSetIfClk;
U32 ulCurClkBd;
U32 ulClkType, ulClkIntf;
U32 ClkIndexM, ClkIndexS; // master and slave clock board index
U32 ulGrade;
U32 ulFlag; // number of clock boards
DEV_FRM_CLK_T* pClk;
NM_VALUE_T sValue1, sValue2;
SYSTRACE(0,0,0,0);
pFrame = (DEV_FRAME_T *)pObject;
pDevEnv = (DEV_MSG_ENV_T *)pMsg;
ulGrade = *(U32*)(pDevEnv+1);
pClk = &pFrame->frameData.switchFrmData.clkSrc[ulGrade];
ulClkType = pClk->type;
ulClkIntf = pClk->ifIndex;
ClkIndexM = pFrame->frameData.switchFrmData.clkSlotIndex;
ulCurClkBd = DEV_GET_SLT_NUMBER(ClkIndexM);
//释放原定时器
if(pFrame->frameFsm.timer != NULL)
{
TMR_DeleteTimer(pFrame->frameFsm.timer);
pFrame->frameFsm.timer = NULL;
}
//申请定时器
rc = TMR_CreateTimer(&(pFrame->frameFsm.timer), DEV_FrmTimerCb, pFrame, NULL);
if(rc != SUCCESS)
{
COUT_OUTPUT_DIAG(MOD_DEV, COUT_LEVEL_WARNING,
"rc = %d %s.%d",rc, __FILE__, __LINE__);
//向源发出错应答
rc = DEV_SendCommCfm((DEV_MSG_ENV_T *)pMsg, rc);
SYSTRACE(1,0,0,0);
return rc;
}
// set intf on a LPU if it is CLCK_SRC_SDH
if ( ulClkType == CLCK_SRC_SDH )
{
// send this first, and then send next one(DEV_N_SendSlotSetClkSrcMsg)
// after cfm recv-ed
rc = DEV_SendSlotSetSdhClkMsg(DEV_IPC_MPU, GetBdId(),
g_ulMpuSeqNo++ , ulClkIntf, DEV_FRM_SETCLKSRCCFM);
ulSetIfClk = 1; // remember, should go on
}
else
{
ulSetIfClk = 0;
ClkIndexS = DEV_CONV_SLT2SLTINDEX(((ulCurClkBd = NET_CARD1)?NET_CARD2:NET_CARD1));
rc1 = DEV_SltGetSlotStatus(ClkIndexM, &sValue1);
rc2 = DEV_SltGetSlotStatus(ClkIndexS, &sValue2);
if (rc1 == SUCCESS || sValue1.v_num == DEV_OBJ_STATUS_ACTIVE)
{
// Send reqest to clock bd, only this one
rc = DEV_N_SendSlotSetClkSrcMsg(DEV_IPC_MPU, GetBdId(), g_ulMpuSeqNo++,
ClkIndexM, ulClkIntf, ulClkType, DEV_FRM_SETCLKSRCCFM);
ulFlag = 0;
}
else
{
SYSTRACE(1,0,0,0);
return DEV_ERR_GEN;
}
if (rc2 == SUCCESS || sValue2.v_num == DEV_OBJ_STATUS_ACTIVE)
{
// there are two clk boards in the frame, and setclksrc message will
// be sent to slave clk board later in comfirm message process routine.
ulFlag = 1;
}
}
if (rc == WAITING_CONFIRM || rc == SUCCESS)
{
//state transfers
pFrame->frameFsm.sequenceNo = g_ulMpuSeqNo++;
pFrame->frameFsm.state = FRM_STE_WAITCFM;
pFrame->frameFsm.msgType = DEV_FRM_SETCLKSRCCFM;
pFrame->frameFsm.frmInfo.frmData[0] = ulSetIfClk; // remember it here
pFrame->frameFsm.frmInfo.frmData[1] = ulGrade; // remember it for 2nd mesg & cfm mesg pFrame->frameFsm.frmInfo.frmData[2] = ulFlag; // remember it for 2 clk boards
//save request msg, for send resp at last.
VOS_Mem_Copy(&pFrame->frameFsm.srcInfo.sMsgEnv,pMsg, sizeof(DEV_MSG_ENV_T));
//启动定时器
TMR_StartTimer(pFrame->frameFsm.timer, DEV_FRM_QUERY_TIME, TMR_ONE_SHOT);
rc = SUCCESS;
}
如果代码走红色的分支，则在兰色的代码处使用的红色的变量ulFlag就没有被初始化。

9.赋值函数最危险，变量没有初始化。

在C代码中，经常使用将变量的指针作为参数，在被调用函数中对变量进行赋值的做法。

这种程序的写法是标准的C语言的用法，无可厚非。

但这种做法隐藏着极大的危险。

例如：
在设备管理模块中,DEV_SubRegMsgProc函数中
if(pSubObj->subPortNumber > 1)
{
for(i = 0; i < pSubObj->subPortNumber; i++)
{
pPhyIfIns = DEV_GetIfFromIndex(pSubObj->phyIfTable[i]);
if(NULL != pPhyIfIns)
{
//删除接口时需要删除相应的端口命令模式:包括物理端口/逻辑端口
//根据接口类型确定命令模板
rc = DEV_SelectCmdTemplet(pPhyIfIns->ifType, &szTempletName);
//删除命令模式
rc = CLI_UnInstallCmdMode(szTempletName, pPhyIfIns->ifName);
DEV_IfDeleteIf(pSubObj->phyIfTable[i]);
}
}
}
其中红色代码的函数对绿色的变量进行赋值,如果红色函数返回失败,则绿色变量就没有被初始化,兰色代码会使用没有初始化的指针.。