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

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

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

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

一点一点的看

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

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

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

virtualvoid 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: 遍历的容器都一样,没有必要声明两个iteratorstd::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();return0;}

 

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("