Clever Geek Handbook

メモ帳++：5年後のコードチェック

写真1






今年、PVS-Studio静的アナライザーは10歳になりました。 確かに、10年前はViva64と呼ばれていたことを明確にする価値があります。 また、別の興味深い日付があります。Notepad++プロジェクトコードの前回の検証から5年が経過しました。 それ以来、PVS-Studioは大幅に改善されました。約190の新しい診断が追加され、古い診断が改善されています。 ただし、Notepad ++で大量のエラーが発生することはないはずです。 これは、123個のソースファイルのみで構成される小さなプロジェクトです。 それでも、修正に役立つコードにエラーが見つかりました。



はじめに



Notepad ++は、Windows用の無料のオープンソーステキストエディターであり、多数のプログラミングおよびマークアップ言語の構文を強調表示します。 STLを使用してC ++で記述されたScintillaコンポーネント、およびWindows APIに基づいており、GNU General Public Licenseの下で配布されています。



私の意見では、Notepad ++は優れたテキストエディターです。 私自身は、コードの記述以外のすべてに使用します。 ソースを分析するために、 PVS-Studio 6.15静的アナライザーを使用しました。 Notepad ++プロジェクトは、 2010年と2012 年に既にテストされています。 84の高警告、124の中警告および548低警告があります。 警告レベルは、見つかったエラーの有効性を示します。 したがって、84の最も信頼性の高い警告（高）のうち81は、私の意見では、コードの本当の欠陥を示しています。 問題は明らかです。



ご注意 静的アナライザーの結果の処理に加えて、インデントにスペースまたはタブを使用することを決定して、コードを改善することは有用です。 絶対にすべてのコードは次のようになります。



図1-コード内のさまざまなインデント







図1-コード内のさまざまなインデント



私にとって最も興味深いと思われたエラーのいくつかを見てみましょう。



継承の問題



写真5









V599 「FunctionParser」クラスには仮想関数が含まれていますが、仮想デストラクタは存在しません。 functionparser.cpp 39 



class FunctionParser
{
friend class FunctionParsersManager;
public:
  FunctionParser(....): ....{};

  virtual void parse(....) = 0;
  void funcParse(....);
  bool isInZones(....);
protected:
  generic_string _id;
  generic_string _displayName;
  generic_string _commentExpr;
  generic_string _functionExpr;
  std::vector<generic_string> _functionNameExprArray;
  std::vector<generic_string> _classNameExprArray;
  void getCommentZones(....);
  void getInvertZones(....);
  generic_string parseSubLevel(....);
};

std::vector<FunctionParser *> _parsers;

FunctionParsersManager::~FunctionParsersManager()
{
  for (size_t i = 0, len = _parsers.size(); i < len; ++i)
  {
    delete _parsers[i]; // <=
  }

  if (_pXmlFuncListDoc)
    delete _pXmlFuncListDoc;
}


, . FunctionParser parse(), . , FunctionZoneParser, FunctionUnitParser FunctionMixParser: 



class FunctionZoneParser : public FunctionParser
{
public:
  FunctionZoneParser(....): FunctionParser(....) {};

  void parse(....);
  
protected:
  void classParse(....);

private:
  generic_string _rangeExpr;
  generic_string _openSymbole;
  generic_string _closeSymbole;

  size_t getBodyClosePos(....);
};

class FunctionUnitParser : public FunctionParser
{
public:
  FunctionUnitParser(....): FunctionParser(....) {}

  void parse(....);
};

class FunctionMixParser : public FunctionZoneParser
{
public:
  FunctionMixParser(....): FunctionZoneParser(....), ....{};

  ~FunctionMixParser()
  {
    delete _funcUnitPaser;
  }

  void parse(....);

private:
  FunctionUnitParser* _funcUnitPaser = nullptr;
};


:



2 - FunctionParser







2 — FunctionParser



, . . , UB, , , , «delete _funcUnitPaser» .



.



V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'redraw' in derived class 'SplitterContainer' and base class 'Window'. splittercontainer.h 61 



class Window
{
  ....
  virtual void display(bool toShow = true) const
  {
    ::ShowWindow(_hSelf, toShow ? SW_SHOW : SW_HIDE);
  }

  virtual void redraw(bool forceUpdate = false) const
  {
    ::InvalidateRect(_hSelf, nullptr, TRUE);
    if (forceUpdate)
      ::UpdateWindow(_hSelf);
  }
  ....
}

class SplitterContainer : public Window
{
  ....
  virtual void display(bool toShow = true) const; // <= good

  virtual void redraw() const;                    // <= error
  ....
}


Notepad++ . SplitterContainer, Window, display() , redraw() .



:







Picture 4









V773 The function was exited without releasing the 'pXmlDocProject' pointer. A memory leak is possible. projectpanel.cpp 326 



bool ProjectPanel::openWorkSpace(const TCHAR *projectFileName)
{
  TiXmlDocument *pXmlDocProject = new TiXmlDocument(....);
  bool loadOkay = pXmlDocProject->LoadFile();
  if (!loadOkay)
    return false;        // <=

  TiXmlNode *root = pXmlDocProject->FirstChild(TEXT("Note...."));
  if (!root) 
    return false;        // <=

  TiXmlNode *childNode = root->FirstChildElement(TEXT("Pr...."));
  if (!childNode)
    return false;        // <=

  if (!::PathFileExists(projectFileName))
    return false;        // <=

  ....

  delete pXmlDocProject; // <= free pointer
  return loadOkay;
}


. pXmlDocProject , . , , , .



V773 Visibility scope of the 'pTextFind' pointer was exited without releasing the memory. A memory leak is possible. findreplacedlg.cpp 1577 



bool FindReplaceDlg::processReplace(....)
{
  ....
  TCHAR *pTextFind = new TCHAR[stringSizeFind + 1];
  TCHAR *pTextReplace = new TCHAR[stringSizeReplace + 1];
  lstrcpy(pTextFind, txt2find);
  lstrcpy(pTextReplace, txt2replace);
  ....
}


processReplace() . : pTextFind pTextReplace. , — . , :



  1. pTextFind . txt2find.
  2. pTextReplace , .


: . , .





Picture 9









V595 The 'pScint' pointer was utilized before it was verified against nullptr. Check lines: 347, 353. scintillaeditview.cpp 347 



LRESULT CALLBACK ScintillaEditView::scintillaStatic_Proc(....)
{
  ScintillaEditView *pScint = (ScintillaEditView *)(....);

  if (Message == WM_MOUSEWHEEL || Message == WM_MOUSEHWHEEL)
  {
    ....
    if (isSynpnatic || makeTouchPadCompetible)
      return (pScint->scintillaNew_Proc(....); // <=
    ....
  }
  if (pScint)
    return (pScint->scintillaNew_Proc(....));
  else
    return ::DefWindowProc(hwnd, Message, wParam, lParam);
}


pScint .



V713 The pointer _langList[i] was utilized in the logical expression before it was verified against nullptr in the same logical expression. parameters.h 1286 



Lang * getLangFromID(LangType langID) const
{
  for (int i = 0 ; i < _nbLang ; ++i)
  {
    if ((_langList[i]->_langID == langID) || (!_langList[i]))
      return _langList[i];
  }
  return nullptr;
}


. _langID, _langList[i], .



, : 



Lang * getLangFromID(LangType langID) const
{
  for (int i = 0 ; i < _nbLang ; ++i)
  {
    if ( _langList[i] && _langList[i]->_langID == langID )
      return _langList[i];
  }
  return nullptr;
}




Picture 6









V501 There are identical sub-expressions to the left and to the right of the '!=' operator: subject != subject verifysignedfile.cpp 250 



bool VerifySignedLibrary(...., const wstring& cert_subject, ....)
{
  wstring subject;
  ....
  if ( status && !cert_subject.empty() && subject != subject)
  {
    status = false;
    OutputDebugString(
      TEXT("VerifyLibrary: Invalid certificate subject\n"));
  }
  ....
}


, Notepad++ , . . , , , .



: 



subject != subject


, , : 



if ( status && !cert_subject.empty() && cert_subject != subject)
{
  ....
}


V560 A part of conditional expression is always true: 0xff. babygrid.cpp 711 



TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
  int returnvalue;
  TCHAR mbuffer[100];
  int result;
  BYTE keys[256];
  WORD dwReturnedValue;
  GetKeyboardState(keys);
  result = ToAscii(static_cast<UINT>(wParam),
    (lParam >> 16) && 0xff, keys, &dwReturnedValue, 0); // <=
  returnvalue = (TCHAR) dwReturnedValue;
  if(returnvalue < 0){returnvalue = 0;}
  wsprintf(mbuffer, TEXT("return value = %d"), returnvalue);
  if(result!=1){returnvalue = 0;}
  return (TCHAR)returnvalue;
}


. 0xff . , ToAscii() : 



(lParam >> 16) & 0xff


V746 Type slicing. An exception should be caught by reference rather than by value. filedialog.cpp 183 



TCHAR* FileDialog::doOpenSingleFileDlg()
{
  ....
  try {
    fn = ::GetOpenFileName(&_ofn)?_fileName:NULL;
    
    if (params->getNppGUI()._openSaveDir == dir_last)
    {
      ::GetCurrentDirectory(MAX_PATH, dir);
      params->setWorkingDir(dir);
    }
  } catch(std::exception e) {                             // <=
    ::MessageBoxA(NULL, e.what(), "Exception", MB_OK);
  } catch(...) {
    ::MessageBox(NULL, TEXT("....!!!"), TEXT(""), MB_OK);
  }

  ::SetCurrentDirectory(dir); 

  return (fn);
}


. , , . , Exception , .



V519 The 'lpcs' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3116, 3117. babygrid.cpp 3117 



LRESULT CALLBACK GridProc(HWND hWnd, UINT message,
                          WPARAM wParam, LPARAM lParam)
{
  ....
  case WM_CREATE:
    lpcs = &cs;
    lpcs = (LPCREATESTRUCT)lParam;
  ....
}


. . , , .



V601 The 'false' value becomes a class object. treeview.cpp 121 



typedef std::basic_string<TCHAR> generic_string;

generic_string TreeView::getItemDisplayName(....) const
{
  if (not Item2Set)
    return false;                     // <=
  TCHAR textBuffer[MAX_PATH];
  TVITEM tvItem;
  tvItem.hItem = Item2Set;
  tvItem.mask = TVIF_TEXT;
  tvItem.pszText = textBuffer;
  tvItem.cchTextMax = MAX_PATH;
  SendMessage(...., reinterpret_cast<LPARAM>(&tvItem));
  return tvItem.pszText;
}


, - «return false».





Picture 8









, . .



V668 There is no sense in testing the 'source' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. notepad_plus.cpp 1149 



void Notepad_plus::wsTabConvert(spaceTab whichWay)
{
  ....
  char * source = new char[docLength];
  if (source == NULL)
    return;
  ....
}


? C++, new , nullptr.



. , , .



, , . , , .



V713 The pointer commentLineSymbol was utilized in the logical expression before it was verified against nullptr in the same logical expression. notepad_plus.cpp 3928 



bool Notepad_plus::doBlockComment(comment_mode currCommentMode)
{
  ....
  if ((!commentLineSymbol) ||       // <=
      (!commentLineSymbol[0]) ||
       (commentLineSymbol == NULL)) // <= WTF?
  { .... }
  ....
}


:





V601 The 'true' value is implicitly cast to the integer type. pluginsadmin.cpp 603 



INT_PTR CALLBACK PluginsAdminDlg::run_dlgProc(UINT message, ....)
{
  switch (message)
  {
    case WM_INITDIALOG :
    {
      return TRUE;
    }
    ....
    case IDC_PLUGINADM_RESEARCH_NEXT:
      searchInPlugins(true);
      return true;

    case IDC_PLUGINADM_INSTALL:
      installPlugins();
      return true;
    ....
  }
  ....
}


run_dlgProc() , true/false, TRUE/FALSE. , , : 90 . . , , , , .



V704 '!this' expression in conditional statements should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. notepad_plus.cpp 4980 



void Notepad_plus::notifyBufferChanged(Buffer * buffer, int mask)
{
  // To avoid to crash while MS-DOS style is set as default 
  // language,
  // Checking the validity of current instance is necessary.
  if (!this) return;
  ....
}


. , this . C++, .



:







, . Notepad++ . .



, . :). , , , , , .



, 1000 PVS-Studio 2 . , . , - 5-10 1000 , . Notepad++ 95 KLoc, : 0-40 1000 . , , , , , .



Notepad++ .











, : Svyatoslav Razmyslov. Checking Notepad++: five years later



?
. : PVS-Studio, 2015. , .


More articles:


All Articles