Checking WinMerge with PVS-Studio for the second time

Abstract

The article continues the idea that static code analyzers are tools to be used regularly, not once.

Introduction

The PVS-Studio analyzer allows you to detect errors in C/C++ applications. We checked the WinMerge with it some time ago. There were few errors whose description can be found in the article "Comparing the general static analysis in Visual Studio 2010 and PVS-Studio by examples of errors detected in five open source projects" [1].

A year has passed since then, and we have decided to test the new version of WinMerge with the new version of PVS-Studio. Below are the results of this second check. But what is the most important, there is the following conclusion to draw from it:

There is no sense in checking a project with a static code analysis tool only once and get satisfied with it. Analysis should be performed regularly.

These are the reasons:

Let's get back to the defects found in the code. Note that many of the errors described here refer not to the WinMerge project itself but the libraries it uses. However, it doesn't matter. We just wanted to show that the PVS-Studio analyzer is quickly developing and learning to detect more new types of bugs. The examples below prove it.

Fragments of odd code

Fragment N1

BOOL CCrystalEditView::
DoDropText (....)
{
  ...
  UINT cbData = (UINT) ::GlobalSize (hData);
  UINT cchText = cbData / sizeof(TCHAR) - 1;
  if (cchText < 0)
    return FALSE;
  ...
}

PVS-Studio's diagnostic rule: V547 Expression 'cchText < 0' is always false. Unsigned type value is never < 0. Merge ccrystaleditview.cpp 1135

The GlobalSize() function returns value 0 in case of an error. If it happens, this case will be handled incorrectly. The code is built using unsigned data types, the 'cchText' variable also being of the 'unsigned' type. It means that the "cchText < 0" condition is always false. The code can be fixed by rewriting it in the following way:

UINT cbData = (UINT) ::GlobalSize (hData);
if (cbData < sizeof(TCHAR))
  return FALSE;
UINT cchText = cbData / sizeof(TCHAR) - 1;

Fragment N2

bool isopenbrace (TCHAR c)
{
  return c == _T ('{') || c == _T ('(') ||
         c == _T ('[') || c == _T ('<');
}

bool isclosebrace (TCHAR c)
{
  return c == _T ('}') || c == _T ('}') ||
         c == _T (']') || c == _T ('>');
}

PVS-Studio's diagnostic rule: V501 There are identical sub-expressions to the left and to the right of the '||' operator: c == L'}' || c == L'}' Merge ccrystaleditview.cpp 1556

In the isclosebrace() function, the 'c' variable is compared to the '}' character twice. If you examine the isopenbrace() function's code, you'll understand that the 'c' variable should be compared with the ')' character in the second case.

Fragment N3

static HRESULT safeInvokeA(....)
{
  HRESULT h;
  ...
  // set h to FAILED
  h = -1;
  ...
}

PVS-Studio's diagnostic rule: V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 992

It's not nice and correct to assign value -1 to a variable whose type is HRESULT.

HRESULT is a 32-bit value divided into three different fields: severity code, device code and error code. To handle the HRESULT value, such specific constants are used as S_OK, E_FAIL, E_ABORT, etc., while macros like SUCCEEDED and FAILED are used to check values of the HRESULT type.

The way the value "-1" is written is incorrect. If you want to report some odd bug, you should use value 0x80004005L (Unspecified failure). This constant and others similar to it are described in "WinError.h".

A similar error can be found here:

V543 It is odd that value '-1' is assigned to the variable 'h' of HRESULT type. Merge plugins.cpp 1033

Fragment N4

int TimeSizeCompare::CompareFiles(....)
{
  UINT code = DIFFCODE::SAME;
  ...
  if (di.left.size != di.right.size)
  {
    code &= ~DIFFCODE::SAME;
    code = DIFFCODE::DIFF;
  }
  ...
}

PVS-Studio's diagnostic rule: V519 The 'code' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 80. Merge timesizecompare.cpp 80

This code may be both correct and incorrect: since I'm not familiar with the structure of the WinMerge project, I cannot know for sure.

Variants are possible:

Fragment N5

BOOL CEditDropTargetImpl::
OnDrop (....)
{
  bool bDataSupported = false;

  m_pOwner->HideDropIndicator ();

  if ((!m_pOwner) ||
      (!(m_pOwner->QueryEditable ())) ||
      (m_pOwner->GetDisableDragAndDrop ()))
  ...
}

PVS-Studio's diagnostic rule: V595 The 'm_pOwner' pointer was utilized before it was verified against nullptr. Check lines: 1033, 1035. Merge ccrystaleditview.cpp 1033

As you can see from the "if ((!m_pOwner) ....)" condition, the 'm_pOwner' pointer can be equal to zero. But before the check is performed, this pointer is already being used in the 'm_pOwner->HideDropIndicator()' statement. Thus, a segmentation fault occurs instead of normal null pointer processing.

Fragment N6

BCMenu *BCMenu::FindMenuOption(int nId, UINT& nLoc)
{
  ...
  nLoc = -1;
  ...
}

BOOL BCMenu::ModifyODMenuW(....)
{
  UINT nLoc;
  ...
  BCMenu *psubmenu = FindMenuOption(nID,nLoc);
  ...
  if (psubmenu && nLoc>=0)
    mdata = psubmenu->m_MenuList[nLoc];
  ...
}

PVS-Studio's diagnostic rule: V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1232

In particular conditions, the FindMenuOption() function returns value -1 in the 'nLoc' variable. Since the 'nLoc' variable is unsigned, the function will actually return 0xFFFFFFFFu.

Now consider the code of the ModifyODMenuW() function. The "nLoc>=0" condition is always true. It means that the situation when the FindMenuOption() function returns -1 will be processed incorrectly.

Identical errors:

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1263

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1285

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1309

V547 Expression 'loc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 1561

V547 Expression 'nLoc >= 0' is always true. Unsigned type value is always >= 0. Merge bcmenu.cpp 2409

Fragment N7

The program contains the CompareOptions class that has virtual methods but doesn't have a virtual destructor. Other classes, like DiffutilsOptions, inherit from it. So, absence of a virtual destructor is an error, though it might not lead to a catastrophe.

PVS-Studio's diagnostic rule: V599 The virtual destructor is not present, although the 'CompareOptions' class contains virtual functions. Merge diffcontext.cpp 90

It's unreasonable to cite the corresponding code fragments here because they are large.

Note that the PVS-Studio analyzer's diagnostics is rather exact and the tool doesn't swear at each and all classes which lack a virtual destructor. If you want to understand how the analyzer diagnoses this type of errors, see its description: V599. The virtual destructor is not present, although the 'Foo' class contains virtual functions.

Fragment N8

static void StoreDiffData(....)
{
  ...
  GetLog()->Write
  (
    CLogFile::LCOMPAREDATA,
    _T("name=<%s>, leftdir=<%s>, rightdir=<%s>, code=%d"),
    di.left.filename.c_str(),
    di.left.path.c_str(),
    di.right.path.c_str(), di.diffcode
  );
  pCtxt->m_pCompareStats->AddItem(di.diffcode.diffcode);
  ...
}

PVS-Studio's diagnostic rule: V510 The 'Write' function is not expected to receive class-type variable as sixth actual argument. Merge dirscan.cpp 565

The 'di.diffcode' variable is a structure of the DIFFCODE type. Most likely, the correct code was meant to be the following:

CLogFile::LCOMPAREDATA, _T(...., di.diffcode.diffcode);

Fragment N9

static DIFFITEM *AddToList(....,
 const DirItem * lent, const DirItem * rent,
 ....)
{
  ...
  if (lent)
  {
    ...
  }
  else
  {
    di->left.filename = rent->filename;
  }

  if (rent)
  {
  ...
}

PVS-Studio's diagnostic rule: V595 The 'rent' pointer was utilized before it was verified against nullptr. Check lines: 608, 611. Merge dirscan.cpp 608

The 'rent' pointer is used without checking if it's not equal to zero. Perhaps such a case will never occur in practice. But still, the check "if (rent)" hints that it is possible in theory.

Fragment N10

String FileFilterHelper::ParseExtensions(....) const
{
  String strParsed;
  String strPattern;
  ...
  strParsed = _T("^");
  strPattern = string_makelower(strPattern);
  strParsed = strPattern;
  ...
}

PVS-Studio's diagnostic rule: V519 The 'strParsed' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 339, 342. Merge filefilterhelper.cpp 342

The 'strParsed' variable is assigned different values twice in a row. This code either has an error or an extra assignment. A similar case has been discussed a bit earlier.

Fragment N11

void CLogFile::EnableLogging(BOOL bEnable)
{
  ...
  Write(_T("Path: %s\n*******\n"), m_strLogPath);
  ...
}

PVS-Studio's diagnostic rule: V510 The 'Write' function is not expected to receive class-type variable as second actual argument. Merge logfile.cpp 85

The 'm_strLogPath' variable has the std::wstring type. It means that the log will contain trash. This is the correct code:

Write(_T("Path: %s\n*******\n"), m_strLogPath.c_str());

Fragment N12

void CMergeDoc::Computelinediff(
  CCrystalTextView * pView1, CCrystalTextView * pView2, 
  ....)
{
  ...
  if (pView1->GetTextBufferEol(line) !=
      pView1->GetTextBufferEol(line))
  ...
}

PVS-Studio's diagnostic rule: V501 There are identical sub-expressions 'pView1->GetTextBufferEol(line)' to the left and to the right of the '!=' operator. Merge mergedoclinediffs.cpp 216

The 'pView1' variable is used twice. This code, most likely, contains a misprint, so the correct code is the following:

if (pView1->GetTextBufferEol(line) !=
    pView2->GetTextBufferEol(line))

Fragment N13

void CSplashWnd::OnPaint()
{
  ...
  String text = LoadResString(IDS_SPLASH_DEVELOPERS);

  // avoid dereference of empty strings and
  // the NULL termiated character
  if (text.length() >= 0)
  {
  ...
}

PVS-Studio's diagnostic rule: V547 Expression 'text.length() >= 0' is always true. Unsigned type value is always >= 0. Merge splash.cpp 262

The check "text.length() >= 0" is meaningless. The 'String' type is 'std::wstring'. The 'std::wstring::length()' function always returns a value above or equal to 0.

Fragment N14

void CPreferencesDlg::AddPage(CPropertyPage* pPage, ....)
{
  ...
  m_tcPages.SetItemData(hti, (DWORD)pPage);
  ...
}

PVS-Studio's diagnostic rule: V205 Explicit conversion of pointer type to 32-bit integer type: (DWORD) pPage Merge preferencesdlg.cpp 200

Theoretically (but hardly in practice), an object pointed to by 'pPage' can be located outside the first low-order Gbytes in the 64-bit application. It implies a potential danger, as the pointer is explicitly cast to the 32-bit type 'DWORD'. This is how this code should look to be safe:

m_tcPages.SetItemData(hti, (DWORD_PTR)pPage);

Conclusion

We have found some other odd fragments in the code. But I cannot tell for sure if they contain errors. What is the most important, the PVS-Studio analyzer's progress is evident.

If you want to download a full-fledged trial of the analyzer, please follow this link: http://www.viva64.com/en/pvs-studio-download/. The new trial model will help you to benefit from the analyzer without purchasing it.

If you have questions regarding this article or the analyzer, please read the post "FAQ for those who have read our articles" [3]. You are also welcome to ask any questions by writing a letter directly to me and my colleagues using the feedback page.

References: