Linux version of PVS-Studio couldn't help checking CodeLite

Linux version of PVS-Studio couldn't help
checking CodeLite
Author: Maxim Stefanov
Date: 27.10.2016
As is already known to our readers, PVS-Studio static analyzer is exploring a new development direction
- the Linux platform; as you may have noticed from the previous articles, it is doing well. This article
shows how easily you can check a project with the help of the Linux version of the analyzer, because the
simpler PVS-Studio for Linux is, the more supporters it will have. This time our choice was the CodeLite
project. CodeLite was compiled and tested in Linux. Let's see what results we got.
About the project
CodeLite is a free, open source, cross platform C,C++,PHP, and Node.js IDE, which uses the wxWidgets
toolkit. To comply with the spirit of open source software, CodeLite is compiled and debugged
exclusively with free tools (MinGW and GDB).
CodeLite features: project management, code completion (ctags + clang), code refactoring, syntax
highlighting, integration into Subversion and Git, Cscope integration, UnitTest++ integration, an
interactive debugger built over GDB, and a powerful source code editor (based on Scintilla).
Codelite is distributed under the GNU General Public License v2 or later. It is free. Codelite, being well
developed and debugged, can be used as a development platform.
CodeLite's modern versions also support projects on PHP and Node.js.
The source code of CodeLite is available on GitHub
The analysis results
To do the check I used PVS-Studio for Linux. Let me briefly tell you about the workflow.
Before starting my work, I read the instructions on running and using PVS-Studio for Linux. The analyzer
can be used in two ways: integrated into a build system (considered the best way) or used as a utility
pvs-studio-analyzer. To do the check quickly and start analyzing the errors, I decided to use the second
method.

So, here we go.
First, I downloaded the source code of the project.
Then I created a simple config file - PVS-Studio.cfg - where I wrote the following:
exclude-path = /usr/include/
lic-file = /path/to/PVS-Studio.lic
output-file = /path/to/PVS-Studio.log
Since CodeLite is a cmake project, I used the cmake utility for building with the flag that is necessary for
further work with the analyzer.
$ mkdir codelite/build
$ cd build
$ cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On ../
After the project was successfully built, I started the analysis:
$ pvs-studio-analyzer analyze --cfg /path/to/PVS-Studio.cfg -j4
As a result, I got the file PVS-Studio.log through the specified path in PVS-Studio.cfg. To get useful
information from it, I used the plog-converter utility that is a part of PVS-Studio distribution kit.
To view the analyzer report, I ran the plog-converter in the following way:
$ plog-converter -a GA:1,2 -t tasklist -o /path/to/codelite.tasks
/path/to/PVS-Studio.log
After this command, I got a codelite.tasks in the specified directory, which I opened with Qt Creator.
Pointer handling
Warning V595 The 'pResult' pointer was utilized before it was verified against nullptr. Check lines: 522,
526. SqliteDatabaseLayer.cpp 522
bool CodeBlocksImporter::isSupportedWorkspace()
{
....
wxXmlNode* root = codeBlocksProject.GetRoot();
wxString nodeName = root->GetName(); // <=
if(root && // <=
(nodeName == wxT("CodeBlocks_workspace_file") ||
nodeName == wxT("CodeBlocks_project_file")))
return true;
}
return false;
}
In the code given above, the analyzer detected a bug related to a potential dereferencing of a root
pointer. It is possible that a pointer will never be null, and the programmer is sure about this, but then
why is he verifying against null again? This only leads to confusion when reading the code. In my
opinion, it is a real bug, and the code should be changed.
Similar analyzer warnings:
 V595 The 'pResult' pointer was utilized before it was verified against nullptr. Check lines: 522,
526. SqliteDatabaseLayer.cpp 522

 V595 The 'ms_instance' pointer was utilized before it was verified against nullptr. Check lines:
24, 25. php_parser_thread.cpp 24
Warning V512 A call of the 'memset' function will lead to underflow of the buffer
'EndTimestampListHandles'. md5.cpp 243
class MD5
{
....
// assumes char is 1 word long
typedef unsigned char uint1;
// next, the private data:
....
uint1 buffer[64]; // input buffer
....
static void memset(uint1 *start, uint1 val, uint4 length);
....
};
void MD5::finalize ()
{
....
// Zeroize sensitive information
memset (buffer, 0, sizeof(*buffer)); // <=
finalized=1;
}
Here the bug is connected with an incorrect value of the third argument, being passed to the memset
function. The sizeof(*buffer) operator returns not the actual size of the buffer, but the size of the first
element, which is an error. For this particular example, only 1 byte will be passed to memset instead of
64.
Note. Pay attention that here, the programmer uses a "custom" memset function. How does the
analyzer know that it is used incorrectly? The name of this and some other functions are so
fundamentally similar that they are used in the same way. That's why for this, and for some other
functions, the analyzer doesn't take notice of which namespace or in which class they are declared, the
most important thing is that the number and the type of arguments match. As we see, such actions help
find errors.
Warning V668 There is no sense in testing the 'buffer' pointer against null, as the memory was allocated
using the 'new' operator. The exception will be generated in the case of memory allocation error.
ShapeDataObject.cpp 65
wxString wxSFShapeDataObject::SerializeSelectedShapes(....)
{
....
char *buffer = new char [outstream.GetSize()];
if(buffer) //<=
{
memset(buffer, 0, outstream.GetSize());
outstream.CopyTo(buffer, outstream.GetSize()-1);
wxString output(buffer, wxConvUTF8);
delete [] buffer;
return output;

}
else
return wxT(....);
}
Here we have a pointless pointer verification. According to the C++ language standards, while allocating
memory via new, it doesn't make sense to verify the pointer against null, because there may be an
exception std::bad_alloc() thrown in case the memory will fail to allocate. In such cases, you should use
a try... catch block to handle these critical situations. If you wish to avoid using exceptions, then there is
new that doesn't throw exceptions. For example:
char *buffer = new char (std::nothrow) [outstream.GetSize()];
Of course, using try..catch or std::nothrow aren't examples of graceful solutions and are provided here
only as variants of quick and rough fixes.
There some other similar situations found (only some of the messages are provided here, they are 19 in
total):
 V668 There is no sense in testing the 'pResultSet' pointer against null, as the memory was
allocated using the 'new' operator. The exception will be generated in the case of a memory
allocation error. SqliteDatabaseLayer.cpp 199
 V668 There is no sense in testing the 'pReturnStatement' pointer against null, as the memory
was allocated using the 'new' operator. The exception will be generated in the case of a memory
allocation error. SqliteDatabaseLayer.cpp 223
 V668 There is no sense in testing the 'm_proc' pointer against null, as the memory was allocated
using the 'new' operator. The exception will be generated in the case of a memory allocation
error. async_executable_cmd.cpp 182
 and so on...
This inattention...
Warning V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a
mistake. Check lines: 175, 176. PhpLexerAPI.h 176
struct WXDLLIMPEXP_CL phpLexerUserData {
....
int m_commentStartLine;
int m_commentEndLine;
....
void ClearComment()
{
m_comment.clear();
m_commentEndLine = wxNOT_FOUND; //<=
m_commentEndLine = wxNOT_FOUND;
}
};
An obvious Copy-Paste error. In the class phpLexerUserData there is a variable commentStartLine
besides the variable commentEndLine. So, in fact, the ClearComment method should be like this:
void ClearComment()
{
m_comment.clear();
m_commentStartLine = wxNOT_FOUND;
m_commentEndLine = wxNOT_FOUND;
}

The same error was found in several more places:
 V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a
mistake. Check lines: 171, 172. CxxLexerAPI.h 172
 V519 The 'm_commentEndLine' variable is assigned values twice successively. Perhaps this is a
mistake. Check lines: 143, 144. JSLexerAPI.h 144
Warning V547 Expression 'type.Lower() == "Array"' is always false. NodeJSOuptutParser.h 61
struct NodeJSHandle {
wxString type;
....
bool IsString() const {return type.Lower() == "string";}
bool IsArray() const {return type.Lower() == "Array"; } //<=
};
The IsArray method will always return false because of a small typo. To fix it, we should just replace
"Array" with "array" and everything will work in the way it should.
Warning V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical
error presence. Check lines: 383, 386. MainFrame.cpp 383
void MainFrame::OnSignal(wxCommandEvent& e)
{
if(m_process) {
int sigid = e.GetId();
if(sigid == ID_SIGHUP)
wxKill(m_process->GetPid(), wxSIGHUP);
else if(sigid == ID_SIGINT)
wxKill(m_process->GetPid(), wxSIGINT);
else if(sigid == ID_SIGKILL)
wxKill(m_process->GetPid(), wxSIGKILL);
else if(sigid == ID_SIGKILL) // <=
wxKill(m_process->GetPid(), wxSIGTERM);
}
}
I will dare to suggest that the programmer decided to speed up writing this method by copying the
previous string, but forgot to change the constant. The increase in productivity is great, of course, but
we shouldn't forget to be attentive. The correct version is:
void MainFrame::OnSignal(wxCommandEvent& e)
{
....
else if(sigid == ID_SIGKILL)
wxKill(m_process->GetPid(), wxSIGKILL);
else if(sigid == ID_SIGTERM)
wxKill(m_process->GetPid(), wxSIGTERM);
}
}
One more analyzer warning:

 V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical
error presence. Check lines: 212, 222. new_quick_watch_dlg.cpp 212
Warning V530 The return value of function 'empty' is required to be utilized. actor_network.cpp 56
StringTokenizer::StringTokenizer(const wxString& str,
const wxString& strDelimiter,
const bool &bAllowEmptyTokens /* false */)
{
....
wxString token;
while( nEnd != -1 )
{
if( nEnd != nStart)
token = str.substr(nStart, nEnd-nStart);
else
token.empty(); // <=
if(!token.empty())
m_tokensArr.push_back(token);
....
}
}
The empty() function doesn't change the object, it only returns the Boolean result. In other words, the
else branch is not doing anything. Instead of token.empty() the programmer should have written
token.Empty(), which zeroes the string, or perhaps something else.
Oops! Something was forgotten
Warning V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements.
include_finder.cpp 716
....
#define YY_DECL int yylex YY_PROTO(( void ))
....
YY_DECL
{
....
yy_find_action:
yy_current_state = *--yy_state_ptr;
yy_lp = yy_accept[yy_current_state];
/* we branch to this label when backing up */
find_rule: //<=
for ( ; ; ) /* until we find what rule we matched */
....
}
Here, the error is that among a numerous quantity of code lines, there is a find_rule label, that none of
the goto operators refer to. This could happen because of code refactoring, or perhaps something else.
For now this lonely label carries no semantic load, it just gives a hint that something was forgotten
somewhere.
Such a warning was found in several other places:

 V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements.
comment_parser.cpp 672
cpp_expr_lexer.cpp 1090
cpp_lexer.cpp 1138
Warnings V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp 402
void wxRibbonMetroArtProvider::DrawTab(
wxDC& dc,
wxWindow* WXUNUSED(wnd),
const wxRibbonPageTabInfo& tab)
{
....
if (tab.active)
dc.SetPen(m_tab_border_pen);
else
// TODO: introduce hover border pen colour
dc.SetPen(m_tab_border_pen); // <=
....
}
In the code fragment given above, the programmer started working on some idea, but then put a note
and stopped. It's not hard to guess that there should not be a repeating code string in else-branch.
However, it is probably a temporary decision, judging by the comment.
Similar analyzer warnings:
 V523 The 'then' statement is equivalent to the 'else' statement. art_metro.cpp 402
 V523 The 'then' statement is equivalent to the 'else' statement. php_workspace_view.cpp 948
Warning V560 A part of the conditional expression is always false: 0. entry.c 397
extern void openTagFile (void)
{
....
boolean fileExists;
setDefaultTagFileName ();
TagFile.name = eStrdup (Option.tagFileName);
fileExists = doesFileExist (TagFile.name);
/* allways override old files */
if (fileExists && /*! isTagFile (TagFile.name)*/ 0) //<=
error (FATAL,
""%s" doesn't look like a tag file; ....",
TagFile.name);
if (Option.etags)
{
....
}
Here we see that the condition (fileExists && /*! isTagFile (TagFile.name)*/ 0) is always false because
of 0. Perhaps it was meant to be like this, but most likely this is an error. It could have gotten in the code

when the programmer was doing some debugging and changed the condition, but then after finishing
the work, he forgot to change the condition back.
Superfluous comparison
Warning V728 An excessive check can be simplified. The '||' operator is surrounded by opposite
expressions '!found' and 'found'. editor_config.cpp 120
bool EditorConfig::Load()
{
....
if(userSettingsLoaded) {
if(!found || (found && version != this->m_version)) { //<=
if(DoLoadDefaultSettings() == false) {
return false;
}
}
}
....
}
There is no error here, but such a condition is hard to read. It can be shortened to:
if(!found && version != this->m_version)
Warning V571 Recurring check. The 'isInStatement' condition was already verified in line 2292.
ASBeautifier.cpp 2293
void ASBeautifier::parseCurrentLine(const string& line)
{
....
if(isInStatement && !inStatementIndentStack->empty()) {
if(prevNonSpaceCh == '=' &&
isInStatement && !inStatementIndentStack->empty()) //<=
inStatementIndentStack->back() = 0;
}
}
....
}
The same subexpression is written in two checks that are executed one after another. Perhaps this error
got here through copy-paste, perhaps this condition needs to be edited, but in any case it is worth
reviewing.
Conclusion
The CodeLite project has approximately 600 thousand lines of code written in C and C++. Of course,
there were also some errors made due to inattention and pointer handling, as happens in most projects.
In total, the analyzer issued 360 first and second level warnings. About 40 of them are those that need
to be reviewed and, most likely, fixed.
To avoid errors accumulating in your code, it's important to regular use static code analyzers. As the
results showed, a great variant of an analyzer would be PVS-Studio.
If you want to check your project, or any project which is of interest to you, with the help of PVS-Studio
for Linux, it can be downloaded here.

Linux version of PVS-Studio couldn't help checking CodeLite

Recommended

More Related Content

What's hot (20)

Viewers also liked (20)

Similar to Linux version of PVS-Studio couldn't help checking CodeLite (18)

Recently uploaded (20)

Linux version of PVS-Studio couldn't help checking CodeLite