金山安全卫士代码批评

标签: 金山 安全 卫士 | 发表时间:2010-12-02 09:00 | 作者:Michael Peng edware_love
出处:http://www.cnblogs.com/

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

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

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

 

一点一点的看

 

1 C++的应用不过关。该用conststatic的时候不用

 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%"))) {
            ::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);
}


 

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

 

 

 

 

作者: Michael Peng 发表于 2010-12-02 09:00 原文链接

评论: 128 查看评论 发表评论


最新新闻:
· 切客网试水酒店商务合作 探索LBS应用合作模式(2010-12-02 18:06)
· 谷歌街景服务因侵犯私人领地被判赔偿1美元(2010-12-02 18:05)
· 摩托罗拉收购4Home欲涉足智能家居(2010-12-02 18:02)
· 微软张亚勤:要有批判性思维不断挑战与争论(2010-12-02 17:57)
· Google宣布HTML5游戏大赛获胜者,《忍者跳》和《猴子要塞》胜出(2010-12-02 17:53)

编辑推荐:金山安全卫士代码批评

网站导航:博客园首页  我的园子  新闻  闪存  小组  博问  知识库

相关 [金山 安全 卫士] 推荐:

金山安全卫士代码批评

- edware_love - 博客园-首页原创精华区
金山卫士开源了,参见金山卫士开源计划. 抱着学习研究的目的下了一份看看. 中国知名通用软件厂商,民族软件业的一面旗帜就这代码水平?代码显然达不到工业级的标准,只能算是实习生练手的水准. 为了给有意拿这份代码当学习资料的初学者提个醒,不被误导,做出了一个艰难的决定,写博文来评论金山安全卫士的代码. 该用const和static的时候不用.

从360安全卫士卸载金山网盾看产品设计

- xh - 所有文章 - UCD大社区
做产品的要学会分析用户心理,诱导用户按我们的意愿办事,但还得看起来像是用户做了英明的决策. 该文非常多智慧的火花,收藏学 习. 闹得沸沸扬扬的“360流氓卸载金山网盾”事件中,别人关注的是国内IT界两个豪门之间的是非,其实这里面最有看头的,是学习一下360安全卫 士的产品经理如何熟练的通过产品交互手段操纵用户,打击竞争对手产品.

消息称小米手机将内置金山手机卫士

- kong - cnBeta.COM
消息人士今日透露,小米手机将与金山网络合作,内置金山手机卫士. 对于小米与金山网络的合作,双方均未发表评论. 有分析人士认为,由于两家公司的董事长同为雷军,双方的合作顺风顺水,在很多分析者的预料之内.

腾讯QQ和360安全卫士之战升级

- Pangolin - 月光博客
  腾讯和360的弹窗大战愈演愈烈,两家公司之间的口水仗引来众多网民关注,两大网络客户端则不停向数亿网民强势推送“最新战况”,使得这场腾讯和360的大战让数亿网民“被围观”.   根据CNNIC近期的统计数据,中国国内网民为4亿多,据腾讯的数据,目前QQ注册用户已经突破了10亿,同时在线用户人数也突破了1亿.

金山重装高手-重装系统如此安全简单

- Richard - 小建の软件园
金山重装高手更新到2.1.1.537正式版,增加常用软件保留列表,一些最新更新的补丁也集成其中,重装以后进入桌面的速度也有了提升. 说到重装系统,你还在使用系统光盘、Ghost镜像甚至命令行进行系统重装吗. 使用金山重装高手,无需光盘、也无需另外下载系统或者镜像,即可一键帮你完成所有重装过程. 金山重装高手包含几个非常实用的重要功能,包括一键系统重装、一键重要数据备份、驱动更新、软件更新及常用软件等.

金山KingCloud私有云安全平台官网正式上线

- Nowings - cnBeta.COM
国内首款基于智能终端安全基线的私有云安全平台――金山KingCloud私有云安全系统官方网站近日正式发布上线. 与传统的终端安全解决方案不同,金山KingCloud私有云创新的提出了“企业私有安全基线”概念,帮助企业打造专属的IT健康免疫系统.

宜居旧金山

- Linker Lin - 译言-每日精品译文推荐
       有多达43座山的旧金山真是一座光彩夺目的山上城市. 旧金山有凉爽的气温,时尚的城市氛围. 海狮们在码头上晒太阳,高耸入云的大桥横跨海湾. 在旧金山你会感到,这里总会有非同一般的事情发生. 正是由于这种活力和生机,使许多人愿意到旧金山来居住或游览,以便亲自体验一下北加州的神奇.        有太多事情使旧金山变得与众不同,从红色巨兽般的金门大桥,到城市海纳百川的胸怀.

金山面试CDN

- - CSDN博客互联网推荐文章
今天去金山网络面试的时候,被问到 性能优化,我说了几个,最后说到了 CDN,我说要尽量把静态的内容放置到CDN,但是为什么呢. 面试官说既然你说到CDN,你就说说它的原理. 按我个人理解来说它是遵循就近原则,给用户找到最近的服务器来提供用户的静态内容,比如CSS文件、图像等,来提高用户访问网站的响应速度.

金山将推 WPS for Linux

- dunqiu - LinuxTOY
今天金山办公软件副总裁章庆元在其微博爆料称,金山正在为 Linux 平台开发 WPS 办公软件,并放出了 WPS 的 Linux 版本在 Ubuntu 上运行的截图. 据了解,WPS 的 Linux 版本为原生程序,采用 C++ 开发,预计春节前后发布首个社区测试版本. 收藏到 del.icio.us |.

金山Office移动版4.0发布

- Ren - cnBeta.COM
金山办公软件旗下的Kingsoft Office移动版4.0版本正式发布,该产品满足Android手机或平板用户随时随地的办公需求 ,让用户的Office文档操作尽在掌控. Kingsoft Office移动版4.0不仅支持doc、docx、xls、xlsx、ppt、pptx等十种主流格式Office文档的查看和编辑,而且内置文件管理 器,能让用户自动整理手机或平板电脑上的办公文档.