# Checking Notepad\+\+: five years later

This year PVS\-Studio static analyzer turned 10\. Although, we should clarify the point that 10 years ago it was called Viva64\. Another interesting point: it's been 5 years since the previous check of the Notepad\+\+ project\. During this period of time the analyzer was significantly improved: about 190 new diagnostics were added and the old ones got refined\. However, we cannot expect to see a large number of errors in Notepad\+\+\. It is quite a small project that has only 123 files with the source code\. Nevertheless, there are still errors that are worth fixing\.

![0511_NotepadPP3/image1.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image1.png)

## Introduction

[Notepad\+\+](https://notepad-plus-plus.org/) \- a free open source text editor for Windows with syntax highlighting for a large number of programming languages and markup\. It is based on the Scintilla component, written in C\+\+ using STL and Windows API and is distributed under GNU General Public License\.

To my mind, Notepad\+\+ is a great text editor\. I myself use it for everything besides writing the code\. To analyze the source code I used [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) 6\.15\. Notepad\+\+ project was already checked in [2010](https://pvs-studio.com/en/blog/posts/cpp/0083/) and [2012](https://pvs-studio.com/en/blog/posts/cpp/0131/)\. Now we found 84 warnings of the High level, 124 warnings of the Medium level and 548 Low level warnings\. The levels denote the degree of certainty in the detected errors\. So, out of 84 most certain warnings \(High level\), 81 point to real issues in the code \- they need to be fixed right away without digging deep in the logic of the program, as the flaws are really obvious\. 

Note\. Besides reviewing the results of the static analyzer, it would be useful to improve the code by deciding: to use spaces or tabs for indentation\. Absolutely whole code looks like this:

![0511_NotepadPP3/image2.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image2.png)

Figure 1 \- various indentations in the code\.

Let's take a look at some errors that seemed the most interesting to me\.

## Inheritance problems

![0511_NotepadPP3/image3.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image3.png)

[V599](https://pvs-studio.com/en/docs/warnings/v599/) The virtual destructor is not present, although the 'FunctionParser' class contains virtual functions\. functionparser\.cpp 39

```cpp
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;
}
```

The analyzer found a serious error that led to incomplete destruction of objects\. The base class _FunctionParser _has a virtual function _parse\(\)_, but it doesn't have a virtual destructor\. In the inheritance hierarchy of this class there are such classes as _FunctionZoneParser_, _FunctionUnitParser_ and _FunctionMixParser_:

```cpp
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;
};
```

I made an inheritance scheme for these classes: 

![0511_NotepadPP3/image4.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image4.png)

Figure 2 \- Scheme of inheritance from the FunctionParser class

Thus, the created objects will not be completely destroyed\. This will result in undefined behavior\. We cannot say for sure how the program will work after the UB, but in practice in this case we will have a memory leak as a minimum, as the code "delete \_funcUnitPaser" won't be executed\.

Let's consider the following error:

[V762](https://pvs-studio.com/en/docs/warnings/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

```cpp
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
  ....
}
```

There were several issues with function overloading in Notepad\+\+\. In the class _SplitterContainer, _inherited from the Window class, _the display\(\) _method is overloaded correctly, but a programmer made a mistake when overloading the _redraw\(\)_ method\.

Several more incorrect fragments:

* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'UserDefineDialog' and base class 'StaticDialog'\. userdefinedialog\.h 332
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'FindReplaceDlg' and base class 'StaticDialog'\. findreplacedlg\.h 245
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'GoToLineDlg' and base class 'StaticDialog'\. gotolinedlg\.h 45
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'FindCharsInRangeDlg' and base class 'StaticDialog'\. findcharsinrange\.h 52
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'ColumnEditorDlg' and base class 'StaticDialog'\. columneditor\.h 45
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'WordStyleDlg' and base class 'StaticDialog'\. wordstyledlg\.h 77
* V762 It is possible a virtual function was overridden incorrectly\. See first argument of function 'redraw' in derived class 'WordStyleDlg' and base class 'Window'\. wordstyledlg\.h 99
* V762 It is possible a virtual function was overridden incorrectly\. See third argument of function 'create' in derived class 'PluginsAdminDlg' and base class 'StaticDialog'\. pluginsadmin\.h 107

## Memory leak

![0511_NotepadPP3/image5.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image5.png)

[V773](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'pXmlDocProject' pointer\. A memory leak is possible\. projectpanel\.cpp 326

```cpp
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;
}
```

This function is also an interesting example of a memory leak\. Dynamic memory is allocated for the pointer _pXmlDocProject,_ but it is freed only when the function is executed till the end\. Which is, most likely, a flaw, leading to memory leaks\.

[V773](https://pvs-studio.com/en/docs/warnings/v773/) Visibility scope of the 'pTextFind' pointer was exited without releasing the memory\. A memory leak is possible\. findreplacedlg\.cpp 1577

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

The function _processReplace\(\)_ is called upon every replacement of a substring in a document\. The memory is allocated for two buffers: _pTextFind_ and _pTextReplace_\. The search string is copied into one buffer, to the other \- a replacement string\. There are several errors here which may cause a memory leak:

1. The buffer pTextFind doesn't get cleared and is not used in the function at all\. A search buffer _txt2find_ is used for the replacement\.
1.  The pTextReplace buffer is not used further on, but the memory doesn't get freed\.

Conclusion: every text\-replace operation leads to the leak of several bytes\. The bigger is the search string and the more matches, the more memory leaks\.

## Errors with pointers

![0511_NotepadPP3/image6.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image6.png)

[V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'pScint' pointer was utilized before it was verified against nullptr\. Check lines: 347, 353\. scintillaeditview\.cpp 347

```cpp
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);
}
```

In one fragment a programmer missed a check of the _pScint_ pointer for validity\. 

[V713](https://pvs-studio.com/en/docs/warnings/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

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

The author of the code made a mistake when writing a conditional statement\. First he addresses the field _\_langID_, using a pointer _\_langList\[i\]_, and then compares this pointer with null\.

Most likely, the correct code should be like this:

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

## Miscellaneous errors

![0511_NotepadPP3/image7.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image7.png)

[V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '\!\=' operator: subject \!\= subject verifysignedfile\.cpp 250

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

I remember that in Notepad\+\+ there was found a vulnerability allowing replacing the editor components with modified ones\. There were integrity checks added\. I am not quite sure, if this code was written to fix the vulnerability, but judging by the function name, we can say that it serves for an important check\.

The check 

```cpp
subject != subject
```

looks extremely suspicious, and most likely, it should be like this:

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

[V560](https://pvs-studio.com/en/docs/warnings/v560/) A part of conditional expression is always true: 0xff\. babygrid\.cpp 711

```cpp
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;
}
```

Statements that are always true or always false look very suspicious\. The constant _0xff_ is always true\. Perhaps, there was a typo in the operator and the parameter of the function _ToAscii\(\)_ should be like this:

```cpp
(lParam >> 16) & 0xff
```

[V746](https://pvs-studio.com/en/docs/warnings/v746/) Type slicing\. An exception should be caught by reference rather than by value\. filedialog\.cpp 183

```cpp
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);
}
```

It is better to catch exceptions by reference\. The problem of such code is that a new object will be created, which will lead to the loss of the information about the exception during the catching\. Everything that was stored in the classes inherited from _Exception,_ will be lost\.

[V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'lpcs' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 3116, 3117\. babygrid\.cpp 3117

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

An old value was instantly overridden with a new one\. It looks like an error\. If everything works correctly now, then we should leave only the second string with the assignment and remove the first one\.

[V601](https://pvs-studio.com/en/docs/warnings/v601/) The 'false' value becomes a class object\. treeview\.cpp 121

```cpp
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;
}
```

The return value of the function is a string, but someone decided to make "return false" instead of an empty string\.

## Code cleaning

![0511_NotepadPP3/image8.png](https://import.viva64.com/docx/blog/0511_NotepadPP3/image8.png)

There is no point in doing refactoring in sake of refactoring, there are way more interesting and useful tasks in any project\. What we should do is get rid of useless code\.

[V668](https://pvs-studio.com/en/docs/warnings/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

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

Why is this check necessary here in general? According to the modern C\+\+ standard, the _new_ operator throws an exception upon the lack of memory, it doesn't return _nullptr\._

This function is called upon the replacement of all the tab symbols with spaces in the whole document\. Having taken a large text document, I saw that the lack of memory really leads to the program crash\.

If the check is corrected, then the operation of symbol correction will be canceled and it will be possible to use the editor further on\. All of these fragments need correction, besides that they are so many, that I had to make [separate list of them in a file](http://cppfiles.com/NotepadPP3_V668.txt)\.

[V713](https://pvs-studio.com/en/docs/warnings/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

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

There were ten such strange and useless checks:

* 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
* V713 The pointer commentStart was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 3931
* V713 The pointer commentEnd was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 3931
* V713 The pointer commentStart was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 4228
* V713 The pointer commentEnd was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 4228
* V713 The pointer commentLineSymbol was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 4229
* V713 The pointer commentStart was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 6554
* V713 The pointer commentEnd was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 6554
* V713 The pointer commentLineSymbol was utilized in the logical expression before it was verified against nullptr in the same logical expression\. notepad\_plus\.cpp 6555

[V601](https://pvs-studio.com/en/docs/warnings/v601/) The 'true' value is implicitly cast to the integer type\. pluginsadmin\.cpp 603

```cpp
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;
    ....
  }
  ....
}
```

The function _run\_dlgProc\(\)_ is returning a value not of a logical type, even more so, the code returns either_ true/false,_ or_ TRUE/FALSE_\. First I wanted to write that at least all the indents are of the same kind, but it's not so: there is still a mixture of tabs and spaces in one of the 90 lines of the function\. All the other lines have tabs\. Yes, it's not critical, but the code looks to me as an observer, quite sloppy\.

[V704](https://pvs-studio.com/en/docs/warnings/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

```cpp
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;
  ....
}
```

I would also classify such checks as useless code as well\. As you can see in the comment, there used to be a problem with the dereference of the null _this_\. According to the modern standard of the C\+\+ language, such check is unnecessary\.

Here is a list of all such fragments:

* V704 'this && type \=\= DOCUMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 505
* V704 'this && type \=\= ELEMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 506
* V704 'this && type \=\= COMMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 507
* V704 'this && type \=\= UNKNOWN' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 508
* V704 'this && type \=\= TEXT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 509
* V704 'this && type \=\= DECLARATION' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxmla\.h 510
* V704 'this && type \=\= DOCUMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 505
* V704 'this && type \=\= ELEMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 506
* V704 'this && type \=\= COMMENT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 507
* V704 'this && type \=\= UNKNOWN' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 508
* V704 'this && type \=\= TEXT' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 509
* V704 'this && type \=\= DECLARATION' expression should be avoided: 'this' pointer can never be NULL on newer compilers\. tinyxml\.h 510
* V704 'this' expression in conditional statements should be avoided \- this expression is always true on newer compilers, because 'this' pointer can never be NULL\. nppbigswitch\.cpp 119

## Conclusion

There were other errors found, where were not covered in the article\. If desired, authors of Notepad\+\+ can check the project themselves and examine the warnings\. We are ready to provide a temporary license for this\.

Of course, a simple user won't see such issues\. RAM modules are quite large and cheap now\. Nevertheless, the project is still developing and the quality of the code, as well as the convenience of its support may be greatly improved by fixing the detected errors and removing the layers of old code\.

My evaluations are that PVS\-Studio analyzer detected 2 real errors per 1000 lines of code\. Of course, these aren't all errors\. I think there would actually be 5\-10 bugs per 1000 lines of code, which is a fairly low density of errors\. The size of Notepad\+\+ is 95 KLoc, which means that typical density of errors for projects of this kind is: 0\-40 errors per 1000 lines of code\. However, the source of these data about the average error density is quite old, I think that the code quality became much better\.

I would like to thank the authors of Notepad\+\+ for their work on this useful tool and wish them all the success further on\.