Geekly Articles each Day
📜 ⬆️ ⬇️

Notepad ++: code verification five years later

Picture 1

This year, the PVS-Studio static analyzer is 10 years old. True, it is worth clarifying that 10 years ago it was called Viva64. And there is another interesting date: 5 years have passed since the previous verification of the code for the project Notepad ++. Since then, PVS-Studio has been greatly improved: about 190 new diagnostics have been added, and the old ones have been improved. However, expect a huge number of errors in Notepad ++ is not worth it. This is a small project consisting of only 123 source code files. However, errors were found in the code that would be useful to correct.

Introduction


Notepad ++ is a free, open source text editor for Windows, with syntax highlighting for a large number of programming languages ​​and markup. Based on the Scintilla component, written in C ++ using STL, as well as the Windows API and distributed under the GNU General Public License.

In my opinion, Notepad ++ is an excellent text editor. I myself use it for everything except writing code. To analyze the sources, I used the PVS-Studio 6.15 static analyzer. The Notepad ++ project has already been tested in 2010 and 2012 . There are now 84 High warnings, 124 Medium warnings and 548 Low warnings. Warning levels indicate the accuracy of the errors found. So, out of the 84 most reliable warnings (High), 81, in my opinion, point to real defects in the code - they should be fixed right away, without delving deeply into the logic of the program, since The problems are obvious.

Note. In addition to processing the results of the static analyzer, it will be useful to improve the code by defining: use spaces or tabs for indents. Absolutely all the code looks like this:
')

Figure 1 - Different indents in the code.


Figure 1 - Different indents in the code.

Let's look at some of the errors that seemed most interesting to me.

Inheritance problems


Picture 5



V599 The Virtual Parity Class contains virtual functions. 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. , .

Source: https://habr.com/ru/post/330394/

More articles:


All Articles