PVS-Studio analyzer checks TortoiseGit

Most of our articles talk about errors found in various projects using the PVS-Studio analyzer. This time, the TortoiseGit project was the target for verification.

Tortoisegit

Description from Wikipedia: TortoiseGit is a visual client for the git source code management system for Microsoft Windows. Implemented as a Windows Explorer extension (shell extension). The user interface is almost identical to TortoiseSVN, with the exception of features specific to Git.

The TortoiseGit project is small. The size of the downloaded source codes is 35 megabytes. And if you drop the “ext” folder, only 9 megabytes will remain.

Developers clearly care about quality. This is indirectly evidenced by the fact that when compiling with Visual C ++, the / W4 switch is used (the fourth level of warning detail). Plus, in the source code I came across a mention of the Cppcheck analyzer .

Let's see if PVS-Studio can find something interesting in this project.

Validation Results

Note for TortoiseGit developers. Immediately verify the project will not work. There is confusion with the inclusion of stdafx.h files. I will explain very briefly.

The wrong stdafx.h files will be connected in some places. When compiling, problems are not visible, since the compiler takes data from precompiled * .pch files. But these errors manifest themselves when trying to create preprocessed * .i files. TortoiseGit developers can write to us and we will show you how to fix the project.

Nelada 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 Warning: 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. Most likely, they should have been present in the expression. Then, the code should be like this:

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

Another very similar place:

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

PVS-Studio Warning: 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 the code that says that developers also suspect something was wrong :).

And another such typo can be found here: gitstatuslistctrl.cpp 3274.

Something is wrong with the 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;
  ....
}

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

with the conditions. To make it clearer, simplify the code:

• Replace “range-> start + offset” with A;
• Replace “range-> end + offset” with B.

It turns out 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;
  ....
}

Now it is clearly seen that it makes no sense to do checks (A <0), (B <0). These conditions are never met. The code contains some kind of logical error.

Forgot to dereference a 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 warning: V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * last_dot + 1! = '\ 0'. path.c 1258

Consider the expression (last_dot + 1! = '\ 0') in more detail. In it, one is added to the pointer, after which the result is compared with zero. This expression has no practical meaning. It seems to me that the code should have been like this:

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

Although it is perhaps better to write:

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

PVS-Studio analyzer 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 warning: V528 It is odd that pointer to 'char' type is compared with the '\ 0' value. Probably meant: * src_orig == '\ 0'. utf.c 501

It should be written:

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

Octal number

There is code that roams using Copy-Paste from project to project, and I often see it. It contains an error due to which almost all programs behave incorrectly with IBM EBCDIC US-Canada encoding. I think this is not scary. I don’t think anyone is using this encoding right now. However, it's worth talking about the error. Here is the 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 Warning: V536 Be advised that the utilized constant value is represented by an octal form. Oct: 037, Dec: 31. unicodeutils.cpp 42

To make the program text look beautiful, 0 was added to number 37. This is incorrect. Decimal 37 turned into octal number 037. Octal number 03 7 equals decimal 31.

Conditions that 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 Warnings: V547 Expression is always false. Probably the '||' operator should be used here. clonedlg.cpp 413

The above code snippet should remove all the \ and / characters at the end of the line. In fact, these characters will not be deleted. Error in this place:

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

A character in a string cannot be simultaneously \ and /. The program should look like this:

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

A similar error related to the 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 Warning: V547 Expression is always true. Probably the '&&' operator should be used here. smart_protocol.c 264

Here, on the contrary, the condition is always satisfied. Status is always unequal to GIT_ACK_CONTINUE or not equal to GIT_ACK_COMMON.

Missing virtual destructor

The program has a Command class that contains virtual functions:

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

They forgot to declare the destructor virtual. Many other classes are inherited from the class:

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

Since the program works with a pointer to the base class, it leads to problems with the destruction of objects.

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

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

Note. I will make a small digression. Often people joke and scoff at discussing the hackneyed question at the interview “Why do we need virtual destructors?” Like, how much can you ask him already.

But in vain, by the way they laugh. Very good question. I will definitely ask him. It allows you to quickly identify suspicious individuals. If a person answers the question about virtual destructors, this of course does not mean anything special. He either read about it in a book or showed interest in what questions are usually asked at interviews. And he decided to prepare, having studied the answers to typical questions.

Again. The correct answer to the question does not guarantee that a person is a good programmer. It is much more important if a person cannot answer this question. How can I read books on C ++, read articles on the Internet about interviews, and skip this topic? Very strange!

Potential Null Pointer Dereferencing

This time I did not carefully watch the warnings related to the potential possibility to exchange the null pointer. There are a number of V595 diagnostics , but it's not interesting to watch or study them. I will give 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 Warning: V595 The 'n' pointer was utilized before it was verified against nullptr. Check lines: 41, 43. decorate.c 41 The

pointer 'n' is dereferenced in the expression 'n-> hash'. Below the pointer 'n' is checked for NULL. This means that the pointer may turn out to be null and this will lead to problems.

Invalid string format

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

Warning PVS-Studio: 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 superfluous.

Potentially Dangerous Array Index

TortoiseGit has the following code:

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

It is dangerous because the variable 'idx' can theoretically have a negative value. The analyzer noticed that the datasource_to_index function can return -1 in case of an error:

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

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

Thus, although this code may work, it is potentially dangerous. An overflow of the array may occur.

Resource leak

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

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

A device context (DC) is created, but is not used or destroyed in any way. A similar situation here: mymemdc.h 70

Different enum types are compared

There is confusion when comparing numbered 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 warning: V556 The values ​​of different enum types are compared: switch (ENUM_TYPE_A) {case ENUM_TYPE_B: ...}. fast-export.c 449

The tag_of_filtered_mode variable has one type, and ABORT is of another type.

Typo

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

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

Other bugs

Other errors and omissions were found. But they did not seem interesting to me to describe them in the article. TortoiseGit developers can easily find all the shortcomings using the PVS-Studio tool.

I want to remind you that the main benefit of static analysis is its regular use. Download and one-time check the code, this is pampering, and not the use of static code analysis techniques. For example, programmers watch warnings of the compiler regularly, and do not turn them on every 3 years before one of the releases.

Conclusion

This article has turned out with a strong advertising bias. I apologize. Firstly, not every time you get really interesting articles about checking projects. Secondly, I want more people to learn about the PVS-Studio analyzer. This is a great, inexpensive tool that can suit a huge audience of developers using Visual C ++. With regular use, it will save a ton of time searching for typos and other errors.

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

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. The PVS-Studio Analyzer Checks TortoiseGit .