金山卫士代码批评

快来打我* 2021-11-26 08:26 224阅读 0赞

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

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

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

一点一点的看

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

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

转载于:https://www.cnblogs.com/MichaelPeng/archive/2010/12/02/1893999.html

[Link 1]: http://code.ijinshan.com/