Maxim Stefanov

Oct 27 2016

Tags:

#Cpp

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

Oct 27 2016

Author: Maxim Stefanov

About the project
The analysis results
Conclusion

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
V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. cpp_expr_lexer.cpp 1090
V729 Function body contains the 'find_rule' label that is not used by any 'goto' statements. 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.

#Cpp