金山卫士代码批评

金山卫士开源了，参见金山卫士开源计划。 抱着学习研究的目的下了一份看看。看了一些代码，觉得被忽悠了。中国知名通用软件厂商，民族软件业的一面旗帜就这代码水平?代码显然达不到工业级的标准，只能算是实习生练手的水准。为了给有意拿这份代码当学习资料的初学者提个醒，不被误导，做出了一个艰难的决定，写博文来评论金山卫士的代码。

先说说代码中的几个突出问题

1. C++的应用不过关。该用const和static的时候不用
2. 代码封装做的不好，调用者知道被调用者很多细节，且对被调用者做了过多假设。
3. 文件和函数命名不规划。不能表达内容，且容易引起误解
4. 测试靠打印而不是assert,很难自动化验证。且测试代码未与工程代码分离。
5. 太多的if-else而不会用表驱动
6. 函数逻辑不严格，有明显漏洞。

一点一点的看

1 C++的应用不过关。该用const和static的时候不用

ppro\PrivacyProtection\rule\IRule.h
classIRule

{

public:

//MichaelPeng: Name函数可以设置为const

virtualLPCTSTR Name(void)=0;

virtualvoidEnable(BOOL bEnable){}

//MichaelPeng: Match从语义上也应为const,且参数pData也应为const

virtualBOOL Match(KAccessData*pData)=0;

//MichaelPeng: DbgPrint从语义上也应为const

virtualvoidDbgPrint(void)=0;

};

2 代码封装做的不好，调用者知道被调用者很多细节，且对被调用者做了过多假设。

ppro\PrivacyProtection\rule\KDirRelateRule.cpp

BOOL KDirRelateRule::Match(KAccessData pData)

{

BOOL bRetCode=FALSE;

std::map<CString, std::vector<CString>>::iterator iter;



for(iter=m_mapDirRelates.begin(); iter!=m_mapDirRelates.end(); iter++)

{

constCString&strDir=iter->first;

//MicahelPeng: 在向m_mapDirRelated中插入数据时都调用MakeLower转化成了小写，但在比较时却不转化，假定调用者作了转化??

if(strDir.GetLength()<=pData->nFileDirLen&&

memcmp((LPCTSTR)strDir, pData->szFilePath, strDir.GetLength()sizeof(TCHAR))==0)

{

std::vector<CString>::iterator iterRelate;

std::vector<CString>&vecStr=iter->second;



for(iterRelate=vecStr.begin(); iterRelate!=vecStr.end();++iterRelate)

{

intnMemSize=pData->nProcPathLensizeof(TCHAR);

CString&strTemp=iterRelate;

if(strTemp.GetLength()==pData->nProcPathLen&&

memcmp((LPCTSTR)strTemp, pData->szProcPath, nMemSize)==0)

{

returnTRUE;

}

}

}

}



//MichaelPeng:szProcPath应当类似于C:\windows\notepad.exe, 这里需要保证nProcPathLen和nProcDirLen都被正确设置,

//最好是KAccessData提供方法SetProcPath,在其中将szProcPath, nProcPathLen, nProcDirLen均设置了

//但没有这种函数，需要靠调用者去保证每次都将这三个变量同时正确设置j

intnProcNameLen=pData->nProcPathLen-pData->nProcDirLen;

LPCTSTR pProcStr=pData->szProcPath+pData->nProcDirLen;

//MicaelPeng: 遍历的容器都一样,没有必要声明两个iterator

std::map<CString, std::vector<CString>>::iterator iter2;

//...

}

3 文件和函数命名不规划。不能表达内容，且容易引起误解

RuleManager.cpp，你看到这个文件名第一反应这个文件 是做什么用的?管理rule的是吧，接下来看到的代码会超越你所有的常识。

//KRuleManager.cpp : Defines the entry point for the console application.

//



#include"stdafx.h"

#include"KFileExtensionRule.h"

#include"KProcFileRule.h"

#include"KFileDirRule.h"

#include"KFilePathRule.h"

#include"KFileExtRelateRule.h"

#include"KFileNameRelateRule.h"

#include"KDirRelateRule.h"

#include"KProcPathRule.h"

#include"KSignDataRule.h"

#include"KVariableString.h"

#include"KRuleConfig.h"

#include"KTree.h"

#include"KRuleImpl.h"

#include"KRuleTestImpl.h"

#include"signverify.h"

#include"KSignCache.h"

#include"KProcNameCache.h"



voidTestFileExtensionRule(KAccessDatapData)

{

KFileExtensionRule rule;



rule.AddFileExt(_T(".jpg"));

rule.AddFileExt(_T(".html"));

rule.AddFileExt(_T(".dll"));

rule.AddFileExt(_T(".html"));

rule.AddFileExt(_T(".DLL"));

rule.RemoveFileExt(_T(".dll"));



BOOL bRetCode=rule.Match(pData);

//MichaelPeng: 通过打印而非Assert来校验测试结果,不可重复和自动化

DPrintA("KFileExtensionRule::Match return:%d\n", bRetCode);

}

voidTestTree(void)

{

KTree<int>tree;



tree.SetValue(0);

tree.AddLeft(1);

tree.AddRight(2);

//MichaelPeng: 这里测了啥？没有抛异常就OK了???

}



voidTestRule(void)

{

.......

//MichaelPeng: 这么多KAccessData设置的雷同代码，为何不放到一个数组中用表驱动实现

{

KAccessData data;

//MichaelPeng: 拷贝字符串和设置长度可放到KAccessData的成员函数中一次完成

//代码冗余,暴露给外界太多信息,且这里也未设置nProdDirLen,与BOOL KDirRelateRule::Match(KAccessData pData)的要求矛盾



_tcscpy(data.szProcPath, _T("d:\a\b.exe"));

_tcscpy(data.szFilePath, _T("c:\a\b\b.doc"));

data.nProcPathLen=_tcslen(data.szProcPath);

data.nFilePathLen=_tcslen(data.szFilePath);



TestKDirRelateRule(&data);

}



{

KAccessData data;

_tcscpy(data.szProcPath, _T("c:\a\b\e.exe"));

_tcscpy(data.szFilePath, _T("c:\a\b\b.doc"));

data.nProcPathLen=_tcslen(data.szProcPath);

data.nFilePathLen=_tcslen(data.szFilePath);



TestKProcPathRule(&data);

}



{

KAccessData data;

_tcscpy(data.szProcPath, _T("c:\WINDOWS\system32\notepad.exe"));

_tcscpy(data.szFilePath, _T("c:\a\b\b.doc"));

data.nProcPathLen=_tcslen(data.szProcPath);

data.nFilePathLen=_tcslen(data.szFilePath);



TestKSignDataRule(&data);

}



{

KAccessData data;

_tcscpy(data.szProcPath, _T("c:\Program Files\Common Files\Kingsoft\kiscommon\kisuisp.exe"));

_tcscpy(data.szFilePath, _T("c:\a\b\b.doc"));

data.nProcPathLen=_tcslen(data.szProcPath);

data.nFilePathLen=_tcslen(data.szFilePath);



TestKSignDataRule(&data);

}



TestKVariableString();



TestCreateRules();



TestTree();

}

int_tmain(intargc, _TCHAR*argv[])

{

CSignVerify::Instance().Initialize();

//TestRule();

//TestRuleImpl();

//TestRuleImplMultiThread();

//TestRuleTestImpl();

//TestSign();

//TestSignCacheAndProcNameCache();



//TestUserConfig();

//MichaelPeng: 测试代码未与工程代码清晰分离

TestLoadRule();

return0;

}

4 测试靠打印而不是assert,很难自动化验证。且测试代码未与工程代码分离。

见上例

5 太多的if-else而不会用表驱动

ppro\PrivacyProtection\rule\KSystemEnvirVar.h

classKSystemEnvirVar

{

public:

//MichaelPeng: 应为静态函数

CString GetValue(LPCTSTR szVariable)

{

TCHAR szFolderPath[MAX_PATH+1]={0};

//MichaelPeng: if else太多,应做成表驱动

if(0==_tcsicmp(szVariable, _T("%Desktop%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_DESKTOP,0);

}elseif(0==_tcsicmp(szVariable, _T("%Internet%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_INTERNET,0);

}elseif(0==_tcsicmp(szVariable, _T("%Programs%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PROGRAMS,0);

}elseif(0==_tcsicmp(szVariable, _T("%Controls%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_CONTROLS,0);

}elseif(0==_tcsicmp(szVariable, _T("%Printers%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PRINTERS,0);

}elseif(0==_tcsicmp(szVariable, _T("%Personal%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PERSONAL,0);

}elseif(0==_tcsicmp(szVariable, _T("%Favorites%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_FAVORITES,0);

}elseif(0==_tcsicmp(szVariable, _T("%Startup%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_STARTUP,0);

}elseif(0==_tcsicmp(szVariable, _T("%Recent%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_RECENT,0);

}elseif(0==_tcsicmp(szVariable, _T("%Sendto%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_SENDTO,0);

}elseif(0==_tcsicmp(szVariable, _T("%Bitbucket%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_BITBUCKET,0);

}elseif(0==_tcsicmp(szVariable, _T("%StartMenu%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_STARTMENU,0);

}elseif(0==_tcsicmp(szVariable, _T("%Mydocuments%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYDOCUMENTS,0);

}elseif(0==_tcsicmp(szVariable, _T("%Mymusic%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYMUSIC,0);

}elseif(0==_tcsicmp(szVariable, _T("%Myvideo%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYVIDEO,0);

}elseif(0==_tcsicmp(szVariable, _T("%Desktopdirectory%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_DESKTOPDIRECTORY,0);

}elseif(0==_tcsicmp(szVariable, _T("%Drives%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_DRIVES,0);

}elseif(0==_tcsicmp(szVariable, _T("%Network%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_NETWORK,0);

}elseif(0==_tcsicmp(szVariable, _T("%Nethood%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_NETHOOD,0);

}elseif(0==_tcsicmp(szVariable, _T("%Fonts%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_FONTS,0);

}elseif(0==_tcsicmp(szVariable, _T("%Templates%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_TEMPLATES,0);

}elseif(0==_tcsicmp(szVariable, _T("%CommonStartMenu%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_STARTMENU,0);

}elseif(0==_tcsicmp(szVariable, _T("%CommonPrograms%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_PROGRAMS,0);

}elseif(0==_tcsicmp(szVariable, _T("%CommonStartup%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_STARTUP,0);

}elseif(0==_tcsicmp(szVariable, _T("%LocalAppdate%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_LOCAL_APPDATA,0);

}elseif(0==_tcsicmp(szVariable, _T("%CommonAppdata%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_APPDATA,0);

}elseif(0==_tcsicmp(szVariable, _T("%Windows%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_WINDOWS,0);

}elseif(0==_tcsicmp(szVariable, _T("%System32%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_SYSTEM,0);

}elseif(0==_tcsicmp(szVariable, _T("%ProgramFilesComm%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PROGRAM_FILES_COMMON,0);

}elseif(0==_tcsicmp(szVariable, _T("%CommonDocuments%"))) {

::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_DOCUMENTS,0);

}

else

{

CString strVariable(szVariable);

strVariable.Remove(_T('%'));



TCHAR*szTemp=_tgetenv(strVariable);

if(szTemp)

{

if(_tcsstr(szTemp, L"~"))

{

::GetLongPathNameW(szTemp, szFolderPath, MAX_PATH);

}

else

{

_tcsncpy(szFolderPath, szTemp, MAX_PATH);

}

szFolderPath[MAX_PATH]=0;

}

}



returnszFolderPath;

}



protected:



};

6 函数逻辑不严格，有明显漏洞

ppro\PrivacyProtection\rule\KVariableString.h

BOOL KVariableString::RelpaceVariable(CString&strString)

{

BOOL bReturn=TRUE;

intnStartPos;

intnEndPos;



CString strTemp(strString);

CString strVariable;

CString strValue;



for(;;)

{

nStartPos=strTemp.Find(_T('%'),0);

if(nStartPos!=-1)

nEndPos=strTemp.Find(_T('%'), nStartPos+1);



if(nStartPos==-1||nEndPos==-1)

break;



strVariable=strTemp.Mid(nStartPos, nEndPos-nStartPos+1);

strValue=GetVariable(strVariable);

if(strValue.IsEmpty())

break;



//MichaelPeng: 对于%abc%xxx%def%abc%ghi%,会出现错误的替换,正确的是替换掉%abc%, %def%, %ghi%,但这段代码会替换掉两个%abc%

strTemp.Replace(strVariable, strValue);

}



if(strTemp.Find(_T('%'))!=-1)

{

#ifdef OUTPUT_INIT_WARNING

#ifdef log_file

CString strTempVar(strString);

strTempVar.Replace(_T("%"), _T("%%"));

DPrintW(L"KVariableString::RelpaceVariable fail, variablename:%s\n", strTempVar);

CString strMsg;

strMsg.Format(_T("查找环境变量失败:%s"), strString);

::MessageBox(NULL, strMsg, _T("隐私保护器"), MB_OK);

#endif

#endif

returnFALSE;

}



//MicaelPeng: 这个替换功能不能从函数名体现出来

do

{

nStartPos=strTemp.Find(_T("\\"));

if(nStartPos==-1)

break;



strTemp.Replace(_T("\\"), _T("\"));



}while(true);



strString=strTemp;


ppro\PrivacyProtection\rule\KFunction.cpp


//MichaelPeng: DirCount是什么意思?路径数目?DirLength更合适

intKFunction::GetDirCount(LPCTSTR szPath)

{

LPCTSTR pszChr=_tcsrchr(szPath, _T('\'));



if(!pszChr)return-1;



returnpszChr-szPath+1;

}



//MicahelPeng: count应为length

intKFunction::GetFileNameCount(LPCTSTR szPath)

{

LPCTSTR pszChr=_tcsrchr(szPath, _T('\'));



if(!pszChr)return-1;



return_tcslen(++pszChr);

}



//MichaelPeng: count应为length

//若文件没有后缀而路径有后缀，如C:\a.b.c\dfgh 则结果应为0或-1，但函数返回7

intKFunction::GetFileExtCount(LPCTSTR szPath)

{

LPCTSTR pszChr=_tcsrchr(szPath, _T('.'));



if(!pszChr)return-1;



return_tcslen(pszChr);

}

才看了一小部分代码就发现了这么多的问题。这还不是我看到的全部问题，只是挑了几个比较典型的。如果是园子里初学者练手的项目这种质量是没问题的。但号称专业级的安全保护模块就是这种级别的代码水准，不禁让人对其专业性产生疑虑。

原文链接: https://www.cnblogs.com/MichaelPeng/archive/2010/12/02/1893999.html

欢迎关注

微信关注下方公众号，第一时间获取干货硬货；公众号内回复【pdf】免费获取数百本计算机经典书籍