Re-check project Notepad ++

More than a year has passed since we checked Notepad ++ with the help of PVS-Studio. It is interesting to see how the PVS-Studio analyzer has become better, and what has been fixed in Notepad ++ from previous errors.

Introduction

So, we checked the Notepad ++ project taken from the repository on January 31, 2012. The PVS-Studio analyzer version 4.54 was used for the verification.

As already mentioned, we previously checked this project. Not a lot of errors found, but still found something. In the new version of the project, some of the old errors are fixed, and some are not. This is strange. Apparently, the previous note went unnoticed by the authors of Notepad ++ and they did not use PVS-Studio to check the project. Perhaps, this note still attracts Notepad ++ authors, especially since we recently changed the trial mode and all the errors found using PVS-Studio are visible.

Corrected errors were apparently found by other testing methods or reported by users.
')
For example, the error described in the previous article concerning the filling of the _iContMap array has been fixed. It was like this:

memset(_iContMap, -1, CONT_MAP_MAX); 

Corrected version:

 memset(_iContMap, -1, CONT_MAP_MAX * sizeof(int)); 

But this mistake continues to live and live:

 bool isPointValid() { return _isPointXValid && _isPointXValid; }; 

Diagnostic message of the PVS-Studio analyzer:

V501 operator: _isPointXValid && _isPointXValid Notepad ++ parameters.h 166

We will not return to the mistakes that were described in the previous article. Consider something new that the PVS-Studio analyzer has learned to diagnose in the past tense:

As always, we want to remind you that this is by no means all the shortcomings found, and only those that seemed interesting to us to write the article. Do not forget about the new trial mode, which is the best suited for checking including opensource projects.

New bugs found

Error N1. Array bounds

 int encodings[] = { 1250, 1251, 1252, .... }; BOOL CALLBACK DefaultNewDocDlg::run_dlgProc( UINT Message, WPARAM wParam, LPARAM) { ... for (int i = 0 ; i <= sizeof(encodings)/sizeof(int) ; i++) { int cmdID = em->getIndexFromEncoding(encodings[i]); ... } 

Diagnostic message of the PVS-Studio analyzer:

V557 Array overrun is possible. The value of 'i' index could reach 46. Notepad ++ preferencedlg.cpp 984

The loop enumerates the elements of the “encodings” array. The error is in the wrong condition to stop the cycle. At the last loop iteration, the memory access occurs outside the array. You can fix the error by replacing the condition "<=" with "<". Correct code:

 for (int i = 0 ; i < sizeof(encodings)/sizeof(int) ; i++) 

Error N2. Wrong buffer size calculation

 typedef struct tagTVITEMA { ... LPSTR pszText; ... } TVITEMA, *LPTVITEMA; #define TVITEM TVITEMA HTREEITEM TreeView::addItem(...) { TVITEM tvi; ... tvi.cchTextMax = sizeof(tvi.pszText)/sizeof(tvi.pszText[0]); ... } 

Diagnostic message of the PVS-Studio analyzer:

V514 Dividing sizeof a pointer 'sizeof (tvi.pszText)' by another value. There is a possibility of logical error presence. Notepad ++ treeview.cpp 88

The buffer size is calculated using the expression “sizeof (tvi.pszText) / sizeof (tvi.pszText [0])”. This expression does not make sense. In it, the size of the pointer is divided by the size of one character. How to fix the code is difficult to say, since we are not familiar with the logic of the program.

Error N3. Wrong check that string is empty

 size_t Printer::doPrint(bool justDoIt) { ... TCHAR headerM[headerSize] = TEXT(""); ... if (headerM != '\0') ... } 

Diagnostic message of the PVS-Studio analyzer:

V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * headerM! = '\ 0'. Notepad ++ printer.cpp 380

The pointer is compared with a null value. Zero is given by the literal of the form '\ 0'. This tells us that pointer dereference has been forgotten. Correct code:

 if (*headerM != '\0') 

A similar error was made elsewhere:

V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * headerR! = '\ 0'. Notepad ++ printer.cpp 392

Error N4. A typo in the condition

 DWORD WINAPI Notepad_plus::threadTextPlayer(void *params) { ... const char *text2display = ...; ... if (text2display[i] == ' ' && text2display[i] == '.') ... } 

Diagnostic message of the PVS-Studio analyzer:

V547 Expression is always false. Probably the '||' operator should be used here. Notepad ++ notepad_plus.cpp 4967

The condition (text2display [i] == '' && text2display [i] == '.') Is never satisfied. A character cannot be a space and a dot at the same time. Apparently here we are dealing with a simple typo and the code should look like this:

 if (text2display[i] == ' ' || text2display[i] == '.') 

A similar error was made elsewhere:

V547 Expression is always false. Probably the '||' operator should be used here. Notepad ++ notepad_plus.cpp 5032

Error N5. A typo in the condition

 int Notepad_plus::getHtmlXmlEncoding(....) const { ... if (langT != L_XML && langT != L_HTML && langT == L_PHP) return -1; ... } 

Diagnostic message of the PVS-Studio analyzer:

V590 Consider inspecting this expression. The expression is misprint. Notepad ++ notepad_plus.cpp 853

The check in the code can be simplified. Then the code will look like this:

 if (langT == L_PHP) 

This is clearly not what the programmer wanted. Apparently here we are again dealing with a typo. Correct code:

 if (langT != L_XML && langT != L_HTML && langT != L_PHP) 

Error N6. Incorrect bit handling

 TCHAR GetASCII(WPARAM wParam, LPARAM lParam) { ... result=ToAscii(wParam,(lParam >> 16) && 0xff, keys,&dwReturnedValue,0); ... } 

Diagnostic message of the PVS-Studio analyzer:

V560 A part of the conditional expression is always true: 0xff. Notepad ++ babygrid.cpp 694

In the code they want to extract the third byte from the variable 'lParam'. Because of a typo, the code doesn't do that at all. The error is that the '&&' operator is used instead of '&'. Correct code:

 result=ToAscii(wParam,(lParam >> 16) & 0xff, keys,&dwReturnedValue,0); 

Error N7. Unfinished code

 #define SCE_T3_BRACE 20 static inline bool IsAnOperator(const int style) { return style == SCE_T3_OPERATOR || SCE_T3_BRACE; } 

Diagnostic message of the PVS-Studio analyzer:

V560 A part of the conditional expression is always true: 20. lextads3.cxx 700

The IsAnOperator () function always returns 'true'. Correct code:

 return style == SCE_T3_OPERATOR || style == SCE_T3_BRACE; 

Error N8. Code that will never be executed

 static void ColouriseVHDLDoc(....) { ... } else if (sc.Match('-', '-')) { sc.SetState(SCE_VHDL_COMMENT); sc.Forward(); } else if (sc.Match('-', '-')) { if (sc.Match("--!")) sc.SetState(SCE_VHDL_COMMENTLINEBANG); else sc.SetState(SCE_VHDL_COMMENT); } ... } 

Diagnostic message of the PVS-Studio analyzer:

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 130, 133. lexvhdl.cxx 130

If the first condition (sc.Match ('-', '-')) is true, then the second check will fail. This will result in the case of a sequence of "-!" will never be handled correctly. Most likely, here a part of the code is superfluous and should be written like this:

 static void ColouriseVHDLDoc(....) { ... } else if (sc.Match('-', '-')) { if (sc.Match("--!")) sc.SetState(SCE_VHDL_COMMENTLINEBANG); else sc.SetState(SCE_VHDL_COMMENT); } ... } 

Error N9. Extra code

There are code snippets that do not lead to problems, but are redundant. Here are two examples of this type:

 void Gripper::doTabReordering(POINT pt) { ... else if (_hTab == hTabOld) { /* delete item on switch between tabs */ ::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0); } else { if (_hTab == hTabOld) { /* delete item on switch between tabs */ ::SendMessage(_hTab, TCM_DELETEITEM, iItemOld, 0); } } ... } 

Diagnostic message of the PVS-Studio analyzer:

V571 Recurring check. The 'if (_hTab == hTabOld)' condition was already verified in line 478. Notepad ++ gripper.cpp 485

The second part of the code does not make any sense and can be deleted.

And here is another example, where there are extra checks. The 'mainVerStr' and 'auxVerStr' pointers are always non-zero, since these are arrays created on the stack:

 LRESULT Notepad_plus::process(....) { ... TCHAR mainVerStr[16]; TCHAR auxVerStr[16]; ... if (mainVerStr) mainVer = generic_atoi(mainVerStr); if (auxVerStr) auxVer = generic_atoi(auxVerStr); ... } 

Here you can write easier:

 mainVer = generic_atoi(mainVerStr); auxVer = generic_atoi(auxVerStr); 

The checks shown above are found repeatedly in the Notepad ++ project:

V600 Consider inspecting the condition. The 'mainVerStr' pointer is not always equal to NULL. Notepad ++ nppbigswitch.cpp 938

V600 Consider inspecting the condition. The 'auxVerStr' pointer is not always equal to NULL. Notepad ++ nppbigswitch.cpp 940

V600 Consider inspecting the condition. The 'intStr' pointer is not equal to NULL. Notepad ++ preferencedlg.cpp 1871

V600 Consider inspecting the condition. The 'intStr' pointer is not equal to NULL. Notepad ++ userdefinedialog.cpp 222

V600 Consider inspecting the condition. The 'intStr' pointer is not equal to NULL. Notepad ++ wordstyledlg.cpp 539

findings

Static code analyzers are not tools that can be used from time to time. Using them regularly you can quickly find errors, thereby reducing the cost of eliminating them tenfold.

Why look for a long time for the strange behavior of the program, due to an unclean array? The code "memset (_iContMap, -1, CONT_MAP_MAX)" is easily and quickly found by the static analyzer.

If this error is found by the PVS-Studio static analyzer, the tool was used incorrectly anyway. First, other diagnostic messages were not carefully studied. Secondly, it is necessary to use static analysis regularly. This allows you to quickly eliminate errors in the newly written code. Plus, new diagnostic rules are constantly appearing in PVS-Studio.

Once again I want to emphasize the idea that static analysis is a tool for regular use. This is like an extension of the list of warnings issued by the compiler. Do you work with disabled warnings and turn them on only occasionally when there is a mood? Not. The same applies to diagnostic warnings issued by static analysis tools.

How to do it? You can run the analysis at night on the server. You can use incremental analysis to check newly modified files. If it seems that the analysis is performed slowly and takes a lot of resources, then we suggest that you familiarize yourself with tips to improve the speed of work .