PVS-Studio analyzer checks TortoiseGit

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

Tortoisegit

Description from Wikipedia: TortoiseGit is a visual client of 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 Git-specific features.

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

Developers obviously care about quality. Indirectly, this is evidenced by the fact that when compiling with Visual C ++, the / W4 key is used (fourth level of warning details). Plus, in the source code I met the mention of the Cppcheck analyzer.
')
Let's see if something interesting can be found in this PVS-Studio project.

Test results

A note for TortoiseGit developers. Immediately check the project does not work. There is confusion with the connection of files stdafx.h. I will explain quite briefly.

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

Badies 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 ())' operator. gitstatuslistctrl.cpp 2642

There is a comment in the code that says that developers are also suspicious :).

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

Something is wrong with the terms

 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 Warning: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 2464, 2466. TortoiseGitMerge mergeinfo.c 2464

There is something wrong with the terms. To make it clearer, let's 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 you can clearly see that there is no point in doing checks (A <0), (B <0). These conditions are never met. The code contains some logical errors.

Forgot pointer dereference

 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 the pointer to the '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, a unit is added to the pointer, after which the result is compared with zero. This expression has no practical meaning. I think the code should be like this:

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

Although, perhaps, it is 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 the pointer to the '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 a code that wanders from project to project using Copy-Paste, 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 it's not scary. I do not think that someone is using this encoding. However, it is worth telling 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 it is not possible. Oct: 037, Dec: 31. unicodeutils.cpp 42

To make the text of the program look beautiful, 0 was added to the number 37. This is wrong. The decimal number 37 turned into an octal number 037. The octal number 03 is 7 decimal number 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 characters \ and / at the end of the line. In fact, these characters will not be deleted. Error in this location:

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

The character in the string can not be both \ 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); } 

Similar error with 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. The status is always unequal GIT_ACK_CONTINUE or not equal to GIT_ACK_COMMON.

There is no virtual destructor

The program has a class Command, which contains virtual functions:

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

Destructor forgot to declare 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 when destroying 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 when discussing a hackneyed question at the interview “Why do we need virtual destructors?”. Like, how much can you ask it.

And in vain, by the way laugh. Very good question. I will ask it. It allows you to quickly identify suspicious individuals. If a person answers the question about virtual destructors, it certainly does not mean anything special. He either read about it in a booklet or showed interest in what questions are usually asked at interviews. And he decided to prepare, having studied the answers to the model 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 dereference

This time, I did not carefully watch the warnings related to the potential possibility of exchanging the null pointer. There are a number of diagnostics V595 , but something is not interesting to watch and 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 used 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 'n' pointer is checked for NULL equality. This means that the pointer may 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); .... } 

PVS-Studio warning: 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' theoretically can have a negative value. The analyzer noticed that the datasource_to_index function can return the value -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

So, while this code may work, it is potentially dangerous. May occur beyond the boundary of the array.

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

The device context (DC) is created, but it is not used or destroyed at all. 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 variable tag_of_filtered_mode is of 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 errors

Were found and other errors and shortcomings. But they did not seem interesting to me to describe them in the article. TortoiseGit developers will be able to easily find all the shortcomings using the PVS-Studio tool.

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

Conclusion

This article turned out with a strong advertising bias. I apologize. Firstly, not every time really interesting articles about checking projects are obtained. 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 lot 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 .