看某位高人对金山卫士源码的分析

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

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

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

一点一点的看

1 C++的应用不过关。该用const和static的时候不用
ppro/PrivacyProtection/rule/IRule.h
class IRule
{
public:
// MichaelPeng: Name函数可以设置为const

virtual LPCTSTR Name(void) =
0;
virtual
void Enable(BOOL bEnable){}
// MichaelPeng: Match从语义上也应为const,且参数pData也应为const

virtual BOOL Match(KAccessData* pData) =
0;
// MichaelPeng: DbgPrint从语义上也应为const

virtual
void DbgPrint(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++)
{
const CString& 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)
{
int nMemSize = pData->nProcPathLen *
sizeof(TCHAR);
CString& strTemp =
*iterRelate;
if (strTemp.GetLength() == pData->nProcPathLen &&
memcmp((LPCTSTR)strTemp, pData->szProcPath, nMemSize) ==
0)
{
return TRUE;
}
}
}
}

// MichaelPeng:szProcPath应当类似于C:/windows/notepad.exe, 这里需要保证nProcPathLen和nProcDirLen都被正确设置,
// 最好是KAccessData提供方法SetProcPath,在其中将szProcPath, nProcPathLen, nProcDirLen均设置了
// 但没有这种函数，需要靠调用者去保证每次都将这三个变量同时正确设置j

int nProcNameLen = 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"
void TestFileExtensionRule(KAccessData* pData)
{
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);
}
void TestTree(void)
{
KTree<int> tree;

tree.SetValue(0);
tree.AddLeft(1);
tree.AddRight(2);
// MichaelPeng: 这里测了啥？没有抛异常就OK了???
}

void TestRule(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(int argc, _TCHAR* argv[])
{
CSignVerify::Instance().Initialize();
//TestRule();
//TestRuleImpl();
//TestRuleImplMultiThread();
//TestRuleTestImpl();
//TestSign();
//TestSignCacheAndProcNameCache();

//TestUserConfig();
// MichaelPeng: 测试代码未与工程代码清晰分离
TestLoadRule();
return
0;
}

 

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

5 太多的if-else而不会用表驱动
ppro/PrivacyProtection/rule/KSystemEnvirVar.h

class KSystemEnvirVar
{
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);
} else
if (0
== _tcsicmp(szVariable, _T("%Internet%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_INTERNET, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Programs%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PROGRAMS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Controls%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_CONTROLS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Printers%"))) {
http://hi.baidu.com/lewutian
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PRINTERS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Personal%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PERSONAL, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Favorites%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_FAVORITES, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Startup%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_STARTUP, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Recent%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_RECENT, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Sendto%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_SENDTO, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Bitbucket%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_BITBUCKET, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%StartMenu%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_STARTMENU, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Mydocuments%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYDOCUMENTS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Mymusic%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYMUSIC, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Myvideo%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_MYVIDEO, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Desktopdirectory%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_DESKTOPDIRECTORY, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Drives%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_DRIVES, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Network%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_NETWORK, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Nethood%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_NETHOOD, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Fonts%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_FONTS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Templates%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_TEMPLATES, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%CommonStartMenu%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_STARTMENU, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%CommonPrograms%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_PROGRAMS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%CommonStartup%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_STARTUP, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%LocalAppdate%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_LOCAL_APPDATA, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%CommonAppdata%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_COMMON_APPDATA, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%Windows%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_WINDOWS, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%System32%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_SYSTEM, 0);
} else
if (0
== _tcsicmp(szVariable, _T("%ProgramFilesComm%"))) {
::SHGetSpecialFolderPath(NULL, szFolderPath, CSIDL_PROGRAM_FILES_COMMON, 0);
} else
if (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;
}
}

return szFolderPath;
}

protected:

};

 

 

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

ppro/PrivacyProtection/rule/KVariableString.h

BOOL KVariableString::RelpaceVariable(CString& strString)
{
BOOL bReturn = TRUE;
int nStartPos;
int nEndPos;

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
return FALSE;
}

// 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更合适
int KFunction::GetDirCount(LPCTSTR szPath)
{
LPCTSTR pszChr = _tcsrchr(szPath, _T('//'));

if (!pszChr) return
-1;

return pszChr - szPath +
1;
}

// MicahelPeng: count应为length
int KFunction::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
int KFunction::GetFileExtCount(LPCTSTR szPath)
{
LPCTSTR pszChr = _tcsrchr(szPath, _T('.'));

if (!pszChr) return
-1;

return _tcslen(pszChr);
}

 

 

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