Andrey Karpov

May 18 2014

Tags:

#Cpp

The PVS-Studio Analyzer Checks TortoiseGit

May 18 2014

Author: Andrey Karpov

TortoiseGit
Analysis results
Conclusion

In most of our articles about project checks, we mention that bugs are found by the PVS-Studio static code analyzer. This time we used PVS-Studio, when checking the TortoiseGit project.

TortoiseGit

Description from Wikipedia: TortoiseGit is a Git revision control client, implemented as a Microsoft Windows shell extension. It is free software released under the GNU General Public License.

The TortoiseGit project is small – the total size of the source codes we have downloaded is 35 Mbytes. And if we don't count the "ext" folder, it leaves only 9 Mbytes.

The project developers are obviously concerned with the product's quality. It is indirectly hinted at by the fact that they use the /W4 switch (the fourth warning level) when compiling the code with Visual C++. Besides, I also noticed the Cppcheck analyzer to be mentioned in the source code.

So let's find out if PVS-Studio has managed to find anything of interest in this project.

Analysis results

A note for TortoiseGit's developers. The project can't be checked right away as there is some trouble with inclusion of stdafx.h files. Below is a brief explanation.

In certain places wrong stdafx.h files are included. You don't face any problems during compilation because the compiler takes data from the precompiled *.pch files. But these errors reveal themselves when trying to create preprocessed *.i files. TortoiseGit's developers may contact us, and we will explain how to fix this issue in the project.

Troubles with m_Rev2

class CGitStatusListCtrl :
  public CListCtrl
{
  ....
  CString m_Rev1;
  CString m_Rev2;
  ....
};

void CGitStatusListCtrl::OnContextMenuList(....)
{
  ....
  if( (!this->m_Rev1.IsEmpty()) || (!this->m_Rev1.IsEmpty()) )
  ....
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions '(!this->m_Rev1.IsEmpty())' to the left and to the right of the '||' operator. gitstatuslistctrl.cpp 1560

There are two members in the class: m_Rev1 and m_Rev2. It is these members that should have been most likely used in the expression. Then the code should look as follows:

if( (!this->m_Rev1.IsEmpty()) || (!this->m_Rev2.IsEmpty()) )

Another similar fragment:

void CGitStatusListCtrl::OnNMDblclk(....)
{
  ....
  if( (!m_Rev1.IsEmpty()) ||
      (!m_Rev1.IsEmpty()))    // m_Rev1 twice???
  ....
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions '(!m_Rev1.IsEmpty())' to the left and to the right of the '||' operator. gitstatuslistctrl.cpp 2642

There is a comment in this code hinting that the programmers suspect something is wrong :).

Another similar typo can be found in gitstatuslistctrl.cpp 3274.

Something wrong with conditions

svn_error_t *
svn_mergeinfo__adjust_mergeinfo_rangelists(....)
{
  ....
  if (range->start + offset > 0 && range->end + offset > 0)
  {
    if (range->start + offset < 0)
      range->start = 0;
    else
      range->start = range->start + offset;

    if (range->end + offset < 0)
      range->end = 0;
    else
      range->end = range->end + offset;
  ....
}

PVS-Studio diagnostic message: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 2464, 2466. TortoiseGitMerge mergeinfo.c 2464

Something is wrong with conditions. To make it clearer, let's simplify the code a bit:

Replace "range->start + offset" with A;
Replace "range->end + offset" with B.

We get the following pseudocode:

if (A > 0 && B > 0)
{
  if (A < 0)
    range->start = 0;
  else
    range->start = A;
  if (B < 0)
    range->end = 0;
  else
    range->end = B;
  ....
}

It is now clearly seen that the checks (A < 0) and (B < 0) are meaningless: they will never be true. There must be some logical errors in the code.

Undereferenced pointer

void
svn_path_splitext(const char **path_root,
                  const char **path_ext,
                  const char *path,
                  apr_pool_t *pool)
{
  const char *last_dot;
  ....
  last_dot = strrchr(path, '.');
  if (last_dot && (last_dot + 1 != '\0'))
  ....
}

PVS-Studio diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *last_dot + 1 != '\0'. path.c 1258

Let's examine the (last_dot + 1 != '\0') expression in detail. Inside it, one is added to the pointer, the result then being compared to zero. This expression doesn't make sense, and I suspect the code should look like this:

if (last_dot && (*(last_dot + 1) != '\0'))

Well, it would probably be better this way:

if (last_dot && last_dot[1] != '\0')

PVS-Studio has found another similar error:

static const char *
fuzzy_escape(const char *src, apr_size_t len, apr_pool_t *pool)
{
  const char *src_orig = src;
  ....
  while (src_orig < src_end)
  {
    if (! svn_ctype_isascii(*src_orig) || src_orig == '\0')
  ....
}

PVS-Studio diagnostic message: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *src_orig == '\0'. utf.c 501

The following should be written instead:

if (! svn_ctype_isascii(*src_orig) || *src_orig == '\0')

Octal number

There is some piece of code which roams from project to project, and I often stumble across it. This code contains a bug that makes almost every program behave incorrectly with the IBM EBCDIC US-Canada charset. I don't think it is a crucial defect because this charset doesn't seem to be widely used nowadays. But I still should mention this bug. Here is this piece of code:

static CodeMap map[]=
{
  {037, _T("IBM037")}, // IBM EBCDIC US-Canada
  {437, _T("IBM437")}, // OEM United States
  {500, _T("IBM500")}, // IBM EBCDIC International
  ....
};

PVS-Studio diagnostic message: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. unicodeutils.cpp 42

To make the text look nicer, the programmer wrote number 37 with 0 on the left. Doing so is incorrect because it results in a decimal number 37 becoming an octal number 037. The octal number 037 is equivalent to decimal 31.

Conditions which are always true or false

void CCloneDlg::OnBnClickedCheckSvn()
{
  ....
  CString str;
  m_URLCombo.GetWindowText(str);

  while(str.GetLength()>=1 &&
        str[str.GetLength()-1] == _T('\\') &&
        str[str.GetLength()-1] == _T('/'))
  {
    str=str.Left(str.GetLength()-1);
  }
  ....
}

PVS-Studio diagnostic messages: V547 Expression is always false. Probably the '||' operator should be used here. clonedlg.cpp 413

The code fragment above must delete all the \ and / characters at the end of a string. But it won't happen actually because of the following error:

str[str.GetLength()-1] == _T('\\') &&
str[str.GetLength()-1] == _T('/')

A string character cannot be \ and / at the same time. The code must have looked like this:

while(str.GetLength()>=1 &&
      (str[str.GetLength()-1] == _T('\\') ||
       str[str.GetLength()-1] == _T('/')))
{
  str=str.Left(str.GetLength()-1);
}

There is another similar error related to a status check:

enum git_ack_status {
  GIT_ACK_NONE,
  GIT_ACK_CONTINUE,
  GIT_ACK_COMMON,
  GIT_ACK_READY
};

static int wait_while_ack(gitno_buffer *buf)
{
  ....
  if (pkt->type == GIT_PKT_ACK &&
      (pkt->status != GIT_ACK_CONTINUE ||
       pkt->status != GIT_ACK_COMMON)) {
  ....
}

PVS-Studio diagnostic message: V547 Expression is always true. Probably the '&&' operator should be used here. smart_protocol.c 264

The condition here is, on the contrary, always true; the status is always not equal to GIT_ACK_CONTINUE or GIT_ACK_COMMON.

Missing virtual destructor

The program has the Command class that contains virtual functions:

class Command
{
  virtual bool Execute() = 0;
  ....
};

The programmer forgot to declare the destructor virtual. A number of classes are inherited from this class:

class SVNIgnoreCommand : public Command ....
class AddCommand : public Command ....
class AutoTextTestCommand : public Command ....

Since we are working with a pointer to a base class, it causes problems when destroying objects.

BOOL CTortoiseProcApp::InitInstance()
{
  ....
  Command * cmd = server.GetCommand(....);
  ....
  delete cmd;
  ....
}

PVS-Studio diagnostic message: V599 The virtual destructor is not present, although the 'Command' class contains virtual functions. TortoiseGitProc tortoiseproc.cpp 497

Note. Now let me digress a bit. Applicants at an interview would often make jokes and laugh when answering the trite question, "What is the purpose of virtual destructors?", meaning that it is too old and trivial to ask it again and again.

They shouldn't laugh though. The question is really good, and I always ask it. It allows me to identify suspicious people quicker. If an applicant gives a correct answer about virtual destructors, it doesn't mean too much, of course. It is just that he must have either read about it in a book or investigated the standard questions usually asked at an interview and prepared for it by learning the answers.

Once again, a correct answer doesn't guarantee that the guy is a good programmer. A more important thing is when he cannot answer. How on earth can one read books on C++ and articles about job interviews on the Internet and miss this topic? Strange, isn't it?

Potential null pointer dereferencing

This time I haven't attentively examined the warnings about potential null pointer dereferencing errors. There were a few V595 diagnostics, but honestly I didn't feel like investigating them. Here you are only one example:

void free_decoration(struct decoration *n)
{
  unsigned int i;
  struct object_decoration *hash = n->hash;
  if (n == NULL || n->hash == NULL)
    return;
  ....
}

PVS-Studio diagnostic message: V595 The 'n' pointer was utilized before it was verified against nullptr. Check lines: 41, 43. decorate.c 41

The 'n' pointer is dereferenced in the 'n->hash' expression and is later checked for being null. It means that this pointer can potentially be null, so troubles may occur.

Incorrect string formatting

int CGit::GetCommitDiffList(....)
{
  ....
  cmd.Format(
    _T("git.exe diff -r -R --raw -C -M --numstat -z %s --"),
    ignore, rev1);
  ....
}

PVS-Studio diagnostic message: V576 Incorrect format. A different number of actual arguments is expected while calling 'Format' function. Expected: 2. Present: 3. git.cpp 1231

One actual argument is redundant.

Potentially dangerous array index

TortoiseGit contains the following code fragment:

static svn_error_t *
token_compare(....)
{
  ....
  int idx = datasource_to_index(file_token[i]->datasource);
  file[i] = &file_baton->files[idx];
  ....
}

What is dangerous about it is that the 'idx' variable may theoretically be negative. The analyzer has noticed that the datasource_to_index function may return -1 in case of an error:

static int
datasource_to_index(svn_diff_datasource_e datasource)
{
  switch (datasource)
  {
    ....
  }
  return -1;
}

PVS-Studio diagnostic message: V557 Array underrun is possible. The value of 'idx' index could reach -1. diff_file.c 1052

Thus, although this code works well, it is potentially dangerous as an array overrun may occur.

Resource leak

CMyMemDC(CDC* pDC, ....)
{
  ....
  CreateCompatibleDC(pDC);
  ....
}

PVS-Studio diagnostic message: V530 The return value of function 'CreateCompatibleDC' is required to be utilized. mymemdc.h 36

A device context (DC) is created but it is not used in any way and nor is it destroyed. A similar error can be found in mymemdc.h 70

Comparing different enum-types

Some mess occurs when comparing enum-types:

static enum {
  ABORT, VERBATIM, WARN, WARN_STRIP, STRIP 
} signed_tag_mode = ABORT;

static enum {
  ERROR, DROP, REWRITE
} tag_of_filtered_mode = ERROR;

static void handle_tag(const char *name, struct tag *tag)
{
  ....
  switch(tag_of_filtered_mode) {
  case ABORT:
  ....
}

PVS-Studio diagnostic message: V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B: ... }. fast-export.c 449

The variables tag_of_filtered_mode and ABORT are of different types.

Typo

static int blame_internal(git_blame *blame)
{
  ....
  blame->ent = ent;
  blame->path = blame->path;
  ....
}

PVS-Studio diagnostic message: V570 The 'blame->path' variable is assigned to itself. blame.c 319

Other errors

There were some other errors and defects as well, but I didn't find them interesting enough to be mentioned in the article. TortoiseGit's developers will easily find all the defects themselves with the help of the PVS-Studio tool.

I want to remind you that static analysis brings highest profit when being used regularly. To download the tool and check your code just once is dabbling, not the proper use of the static code analysis methodology. Why, programmers examine compiler warnings regularly, not just once in 3 years before some release, don't they?

Conclusion

The article appears to have a certain advertising flavor about it. Sorry for that. Firstly, it's not just every time that we manage to write interesting articles about project checks. Secondly, we want the PVS-Studio analyzer to be known by as many programmers as possible. This is a wonderful tool that can suit a large audience of developers working in Visual C++. When used regularly, it will help you save huge amounts of time you would otherwise waste searching for typos and other mistakes.

Download PVS-Studio here: http://www.viva64.com/en/pvs-studio/download/

#Cpp