Andrey2008 June 25, 2013 at 10:10

Rechecking TortoiseSVN with PVS-Studio Code Analyzer

We sent TortoiseSVN developers a free key for the PVS-Studio analyzer for a while. Until they had time to use it, I decided to quickly download the TortoiseSVN source codes and perform the analysis myself. The goal is clear. Another small article for advertising PVS-Studio.

We have already tested the TortoiseSVN project. It was a long time ago. The verification of the project coincided with the release of PVS-Studio 4.00, where the general diagnostic rules first appeared.

From time to time, we repeat checks to demonstrate the benefits of using the tool regularly. It makes no sense to check the project once or twice. In a mutable code new errors constantly appear. And then slowly and sadly corrected. Accordingly, the maximum benefit will be achieved with the daily use of PVS-Studio. Better yet, when using incremental analysis .

So, let's see what was found interesting in the project using PVS-Studio version 5.05. TortoiseSVN source codes were taken 06/19/2013 from http://tortoisesvn.googlecode.com/svn/trunk. By the way, the TortoiseSVN project is very high quality and has a huge user base of programmers. So if you find at least something, this is already a great achievement.

Strange conditions

static void ColouriseA68kDoc (....)
{
  if (((sc.state == SCE_A68K_NUMBER_DEC) && isdigit(sc.ch))
      ....
      || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch))
      || ((sc.state == SCE_A68K_MACRO_ARG) && isdigit(sc.ch))
      ....
}

Diagnostic Message: V501 There are identical sub-expressions '((sc.state == 11) && isdigit (sc.ch))' to the left and to the right of the '||' operator. lexa68k.cxx 160

There are two identical comparisons. There may be a typo.

A typo is probably present in the following code. The value of the variable 'rv' is checked twice.

struct hentry * AffixMgr::compound_check(
  ....
  if (rv && forceucase && (rv) && ....)
  ....
}

Diagnostic Message: V501 There are identical sub-expressions to the left and to the right of the '&&' operator: rv && forceucase && (rv):

affixmgr.cxx 1784
affixmgr.cxx 1879

The following code snippet:

bool IsAllASCII7 (const CString& s)
{
  for (int i = 0, count = s.GetLength(); i < count; ++i)
    if (s[i] >= 0x80)
      return false;
    return true;
}

Diagnostic message: V547 Expression 's [i]> = 0x80' is always false. The value range of char type: [-128, 127]. logdlgfilter.cpp 34

The IsAllASCII7 () function always returns 'true'. The condition 's [i]> = 0x80' is always false. The value of a variable of type 'char' cannot be greater than or equal to 0x80.

The following code snippet with an incorrect comparison:

int main(int argc, char **argv)
{
  ....
  DWORD ticks;
  ....
  if (run_timers(now, &next)) {
    ticks = next - GETTICKCOUNT();
    if (ticks < 0) ticks = 0;
  } else {
    ticks = INFINITE;
  }
  ....
}

Diagnostic message: V547 Expression 'ticks <0' is always false. Unsigned type value is never <0. winplink.c 635 The

variable 'ticks' is unsigned. This means that checking “if (ticks <0)” does not make sense. Overflow situation will not be processed.

Consider the error due to which the strncmp function does not fully compare strings.

int  AffixMgr::parse_convtable(...., const char * keyword)
{
  char * piece;
  ....
  if (strncmp(piece, keyword, sizeof(keyword)) != 0) {
  ....
}

Diagnostic Message: V579 The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. affixmgr.cxx 3654

The 'sizeof' operator computes the size of a pointer. This value has nothing to do with string length.

Suspicious string formation

Functions with a variable number of arguments are always everywhere, and as always dangerous.

class CTSVNPath
{
  ....
private:
  mutable CString m_sBackslashPath;
  mutable CString m_sLongBackslashPath;
  mutable CString m_sFwdslashPath;
  ....
};
const FileStatusCacheEntry * SVNFolderStatus::BuildCache(
  const CTSVNPath& filepath, ....)
{
  ....
  CTraceToOutputDebugString::Instance() (_T(__FUNCTION__)
    _T(": building cache for %s\n"), filepath);
  ....
}

Diagnostic Message: V510 The 'operator ()' function is not expected to receive class-type variable as second actual argument:

svnfolderstatus.cpp 150
svnfolderstatus.cpp 355
svnfolderstatus.cpp 360

The specifier "% s" indicates that the function expects a string as the actual argument. However, the variable 'filepath' is not a string at all, but a complex object consisting of many lines. I am at a loss to say what will be printed out and whether this code will fall at all.

It is dangerous to use functions such as 'printf ()' as follows: "printf (myStr);". If control qualifiers are present inside 'myStr', the program may print something that is not supposed to or may crash.

Consider the code from TortoiseSVN:

BOOL CPOFile::ParseFile(....)
{
  ....
  printf(File.getloc().name().c_str());
  ....
}

Diagnostic Message: V618 It's dangerous to call the 'printf' function in such a manner, as the line being passed could contain format specification. The example of the safe code: printf ("% s", str); pofile.cpp 158

If the file name is “myfile% s% i% s.txt”, the result will be disastrous.

Note. We have an interesting note on the dangers of using the printf () function .

Incorrect zeroing of arrays

I do not know how dangerous it is for TortoiseSVN to leave the contents of buffers without resetting them. Perhaps generally safe. However, there is code for zeroing buffers. And since it does not work, it is worth mentioning about it. Errors look like this:

static void sha_mpint(SHA_State * s, Bignum b)
{
  unsigned char lenbuf[4];
  ....
  memset(lenbuf, 0, sizeof(lenbuf));
}

Diagnostic Message: V597 The compiler could delete the 'memset' function call, which is used to flush 'lenbuf' buffer. The RtlSecureZeroMemory () function should be used to erase the private data. sshdss.c 23

Before exiting the function, the array 'lenbuf' should be cleared. Since then the array is no longer used, the optimizer will delete the call to the 'memset' function. To prevent this from happening, you need to use special functions.

Other places where the compiler will remove the call to 'memset ()':

sshdss.c 37
sshdss.c 587
sshdes.c 861
sshdes.c 874
sshdes.c 890
sshdes.c 906
sshmd5.c 252
sshrsa.c 113
sshpubk.c 153
sshpubk.c 361
sshpubk.c 1121
sshsha.c 256

Strange

BOOL InitInstance(HINSTANCE hResource, int nCmdShow)
{
  ....
  app.hwndTT; // handle to the ToolTip control
  ....
}

Diagnostic message: V607 Ownerless expression 'app.hwndTT'. tortoiseblame.cpp 1782

Most likely, in the function 'InitInstance ()', the member 'hwndTT' must be initialized with something. However, due to a typo, the code turned out to be incomplete.

64-bit errors

I do a very superficial search for errors. I am attentive, just enough so that I have enough examples for writing an article. No, I'm not byak. It's just that the authors of the project will still perform a better analysis than I can do. I look at

64-bit errors even more superficially. It is very difficult to judge, without knowing the structure of the project, this or that error may occur or not.

I will give only a couple of dangerous places:

void LoginDialog::CreateModule(void)
{
  ....
  DialogBoxParam(g_hmodThisDll, MAKEINTRESOURCE(IDD_LOGIN),
                 g_hwndMain, (DLGPROC)(LoginDialogProc),
                 (long)this);
  ....
}

Diagnostic message: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'this'. logindialog.cpp 105 The

pointer to 'this' is explicitly cast to type 'long'. Then it is implicitly expanded to type LPARAM (LONG_PTR). The important thing is that the pointer turns into a 'long' for a while. This is bad if the program is 64-bit. A pointer occupies 64 bits. The 'long' type in Win64 is still a 32-bit type. As a result, the most significant bits of the 64-bit variable will be lost.

If the object is created outside the younger 4 GB of RAM, then the program will become unpredictable. The probability of such an event is certainly not great,

Correct code: DialogBoxParam (...., (LPARAM) this);

Consider another dangerous cast:

static int cmpforsearch(void *av, void *bv)
{
  Actual_Socket b = (Actual_Socket) bv;
  unsigned long as = (unsigned long) av,
                bs = (unsigned long) b->s;
  if (as < bs)
    return -1;
  if (as > bs)
    return +1;
  return 0;
}

Diagnostic message: V205 Explicit conversion of pointer type to 32-bit integer type: (unsigned long) av:

winnet.c 139
winhandl.c 359
winhandl.c 348

Pointers are explicitly cast to the type 'unsigned long' and placed in the variables 'as' and 'bs'. Since the most significant bits of the address may be lost in this case, the comparison may not work correctly. It is generally not clear why pointers are cast to integer types here. You can simply compare the pointers.

Deprecated null pointer checks

The 'new' operator, when it cannot allocate memory, long ago did not return NULL. It throws an exception std :: bad_alloc. Of course, you can make the operator 'new' return 0, but this is now irrelevant.

Nevertheless, the following code continues to live in the programs:

int _tmain(....)
{
  ....
  pBuf = new char[maxlength];
  if (pBuf == NULL)
  {
    _tprintf(_T("Could not allocate enough memory!\n"));
    delete [] wc;
    delete [] dst;
    delete [] src;
    return ERR_ALLOC;
  }
  ....
}

Diagnostic Message: V668 There is no sense in testing the 'pBuf' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error.

subwcrev.cpp 912
repositorybrowser.cpp 2565
repositorybrowser.cpp 4225
svnstatuslistctct.cpp 5254
svnprogressdlg.cpp 2357
bugtraqassociations.cpp 116
xmessagebox.cpp 792
xmessagebox.cpp 797
hyperlink_base.cpp 166
affixmgr.cxx 272
hashmgr.cxx 363
hashmgr.cxx 611

And so it goes

About many errors that I encounter when studying code, I do not write in articles. The fact is that they do not interfere with the program. This time I decided to write about a couple of such cases. It’s just very funny to observe situations when the program does not work because it is correctly written, but because of luck.

void CBaseView::OnContextMenu(CPoint point, DiffStates state)
{
  ....
  popup.AppendMenu(MF_STRING | oWhites.HasTrailWhiteChars ?
                   MF_ENABLED : (MF_DISABLED|MF_GRAYED),
                   POPUPCOMMAND_REMOVETRAILWHITES, temp);
  ....
}

Diagnostic Message: V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower priority than the '|' operator. baseview.cpp 2246

Depending on the value of the variable 'oWhites.HasTrailWhiteChars', one of the following combinations of constants is required:

MF_STRING | MF_ENABLED
MF_STRING | MF_DISABLED | MF_GRAYED

But the code doesn't work that way. Operation Priority '|' higher than operation priority '?:'. We put the brackets for clarity:

(MF_STRING | oWhites.HasTrailWhiteChars)? MF_ENABLED: MF_DISABLED | MF_GRAYED

The code works correctly, only because the constant 'MF_STRING' is 0. It has no effect on the result. As a result, the wrong expression works correctly.

Consider another example of luck. In TortoiseSVN, the HWND type is often used as the 'unsigned' type. For this, explicit type conversions have to be performed. For example, as shown in the following functions:

HWND m_hWnd;
UINT_PTR uId;
INT_PTR CBaseView::OnToolHitTest(....) const
{
  ....
  pTI->uId = (UINT)m_hWnd;
  ....
}
UINT_PTR  idFrom;
HWND m_hWnd;
BOOL CBaseView::OnToolTipNotify(
  UINT, NMHDR *pNMHDR, LRESULT *pResult)
{
  if (pNMHDR->idFrom != (UINT)m_hWnd)
    return FALSE;
  ....
}

Or, for example, the value of a variable of type HWND is printed as if it were of type 'long'.

bool CCommonAppUtils::RunTortoiseProc(....)
{
  ....
  CString sCmdLine;
  sCmdLine.Format(L"%s /hwnd:%ld",
    (LPCTSTR)sCommandLine, AfxGetMainWnd()->GetSafeHwnd());
  ....
}

Formally, this code is incorrect. The fact is that the type 'HWND' is a pointer. This means that it cannot be turned into 32-bit integer types. And the PVS-Studio analyzer is worried about this, giving warnings.

But the interesting thing is that this code will work completely correctly!

The HWND type is used to store descriptors that are used by Windows to work with various system objects. The same types are HANDLE, HMENU, HPALETTE, HBITMAP and so on.

Although descriptors are 64-bit pointers, for greater compatibility (for example, for interoperability between 32-bit and 64-bit processes) they use only the lower 32-bit. See the Microsoft Interface Definition Language (MIDL): 64-Bit Porting Guide for more information."(USER and GDI handles are sign extended 32b values).

When placing the HWND type in 32-bit types, the developers were hardly based on these assumptions. Most likely, this is not a very accurate code that works correctly due to the luck and efforts of the Windows API developers.

Conclusion

Use static analysis regularly during development, and you will find many errors at the earliest stages. I naturally recommend that you first of all get acquainted with the PVS-Studio code analyzer . However, there are many other good code analyzers: static code analysis tools .

References

Additional links that may clarify some of the subtle points described in the article.

Knowledge base. Overwriting memory - why?
Documentation. V668 There is no sense in testing the pointer against null, as the memory was allocated using the 'new' operator.
Knowledge base. How to correctly cast a pointer to int in a 64-bit program?
Karpov Andrey, Evgeny Ryzhkov. Lessons for developing 64-bit C / C ++ applications .

Tags: