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.

Introduction

Notepad++ — 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.

Figure 1 — various indentations in the code.

Inheritance problems

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;
}
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;
};
Figure 2 — Scheme of inheritance from the FunctionParser class
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
....
}
  • 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

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;
}
bool FindReplaceDlg::processReplace(....)
{
....
TCHAR *pTextFind = new TCHAR[stringSizeFind + 1];
TCHAR *pTextReplace = new TCHAR[stringSizeReplace + 1];
lstrcpy(pTextFind, txt2find);
lstrcpy(pTextReplace, txt2replace);
....
}
  • The pTextReplace buffer is not used further on, but the memory doesn’t get freed.

Errors with pointers

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

bool VerifySignedLibrary(...., const wstring& cert_subject, ....)
{
wstring subject;
....
if ( status && !cert_subject.empty() && subject != subject)
{
status = false;
OutputDebugString(
TEXT("VerifyLibrary: Invalid certificate subject\n"));
}
....
}
subject != subject
if ( status && !cert_subject.empty() && cert_subject != subject)
{
....
}
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;
}
(lParam >> 16) & 0xff
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);
}
LRESULT CALLBACK GridProc(HWND hWnd, UINT message,
WPARAM wParam, LPARAM lParam)
{
....
case WM_CREATE:
lpcs = &cs;
lpcs = (LPCREATESTRUCT)lParam;
....
}
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;
}

Code cleaning

void Notepad_plus::wsTabConvert(spaceTab whichWay)
{
....
char * source = new char[docLength];
if (source == NULL)
return;
....
}
bool Notepad_plus::doBlockComment(comment_mode currCommentMode)
{
....
if ((!commentLineSymbol) || // <=
(!commentLineSymbol[0]) ||
(commentLineSymbol == NULL)) // <= WTF?
{ .... }
....
}
  • 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
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;
....
}
....
}
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;
....
}
  • 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.

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C++, C#, and Java source code.