Errors detected in the Visual C++ 2012 libraries

Static code analysis is one of the error detection methodologies. We are glad that this methodology is becoming more and more popular nowadays. Visual Studio which includes static analysis as one of its many features contributes to this process to a large extent. This feature is easy to try and start using regularly. When one understands one likes static code analysis, we are glad to offer a professional analyzer PVS-Studio for the languages C/C++/C++11.

Introduction

The Visual Studio development environment allows you to perform static code analysis. This analysis is very useful and easy-to-use. However, we should understand that Visual Studio performs a huge number of functions. It means that each of its functions taken separately cannot compare to specialized tools. The code refactoring and coloring functions are not as good as in Visual Assist. The function of integrated image editing is naturally worse than that in Adobe Photoshop or CorelDRAW. The same is true for the static code analysis function as well.

But this all is theorization. Let's move to practice and see what interesting things the PVS-Studio analyzer has managed to find in Visual Studio 2012 folders.

We didn't actually plan to check the source files included into Visual Studio. It happened by chance: many header files underwent some changes in Visual Studio 2012 because of support for the new language standard C++11. We have faced the task to make sure that the PVS-Studio analyzer can handle these header files.

Unexpectedly we noticed a few errors in the header *.h files. We decided to go on and study the files of Visual Studio 2012 in detail. In particular, the following folders:

We haven't managed to carry out a full-fledged check because we didn't have projects or make-files to build the libraries. So, we have managed to check only a very small part of the libraries' codes. Despite the incompleteness of the check, the results we've got are rather interesting.

Let's see what the PVS-Studio analyzer has found inside the libraries for Visual C++. As you can see, all these errors passed unnoticed by the analyzer integrated into Visual C++ itself.

Some of the suspicious fragments detected

We won't claim that all the fragments cited below really contain errors. We just picked up those fragments from the list generated by the PVS-Studio analyzer that seem to be the most probable to have defects.

A strange loop

This strange code was the first to be found. It prompted us to continue our investigation.

template <class T>
class ATL_NO_VTABLE CUtlProps :
  public CUtlPropsBase
{
  ....
  HRESULT GetIndexOfPropertyInSet(....)
  {
    ....
    for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
    {
      if( dwPropertyId == pUPropInfo[ul].dwPropId )
        *piCurPropId = ul;
      return S_OK;
    }

    return S_FALSE;
  }
  ....
};

V612 An unconditional 'return' within a loop. atldb.h 4829

The loop body is executed only once. There's no need to explain this error: most likely, the 'return' operator should be called when the necessary value is found. In this case the code should look like this:

for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
{
  if( dwPropertyId == pUPropInfo[ul].dwPropId )
  { 
    *piCurPropId = ul;
    return S_OK;
  } 
}

A strange projection

Sorry for this hard-to-read sample. Note the condition in the ternary operator.

// TEMPLATE FUNCTION proj
_TMPLT(_Ty) inline
  _CMPLX(_Ty) proj(const _CMPLX(_Ty)& _Left)
  {  // return complex projection
  return (_CMPLX(_Ty)(
    _CTR(_Ty)::_Isinf(real(_Left)) ||
    _CTR(_Ty)::_Isinf(real(_Left))
      ? _CTR(_Ty)::_Infv(real(_Left)) : real(_Left),
    imag(_Left) < 0 ? -(_Ty)0 : (_Ty)0));
  }

V501 There are identical sub-expressions '_Ctraits < _Ty >::_Isinf(real(_Left))' to the left and to the right of the '||' operator. xcomplex 780

The "_CTR(_Ty)::_Isinf(real(_Left))" expression is repeated twice in the condition. We cannot say for sure if there is an error here and in what way the code should be fixed. But this function is obviously worth paying attention to.

An unnecessary check

template<typename BaseType, bool t_bMFCDLL = false>
class CSimpleStringT
{
  ....
  void Append(_In_reads_(nLength) PCXSTR pszSrc,
              _In_ int nLength)
  {
    ....
    UINT nOldLength = GetLength();
    if (nOldLength < 0)
    {
      // protects from underflow
      nOldLength = 0;
    }
  ....
};

V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. atlsimpstr.h 420

There is no error here. Judging by the code, the string length cannot become negative. The CSimpleStringT class contains the corresponding checks. The nOldLength variable having the unsigned type doesn't affect anything - the string length is positive anyway. This is just unnecessary code.

Incorrect string forming

template <class T>
class CHtmlEditCtrlBase 
{
  ....
  HRESULT SetDefaultComposeSettings(
    LPCSTR szFontName=NULL, .....) const
  {
    CString strBuffer;
    ....
    strBuffer.Format(_T("%d,%d,%d,%d,%s,%s,%s"),
                     bBold ? 1 : 0,
                     bItalic ? 1 : 0,
                     bUnderline ? 1 : 0,
                     nFontSize,
                     szFontColor,
                     szBgColor,
                     szFontName);
    ....
  }
};

V576 Incorrect format. Consider checking the eighth actual argument of the 'Format' function. The pointer to string of wchar_t type symbols is expected. afxhtml.h 826

This code forms an incorrect message in UNICODE programs. The 'Format()' function expects the eighth argument to have the LPCTSTR type, but the 'szFontName' variable will always have the LPCSTR type.

Port with a negative number

typedef WORD ATL_URL_PORT;
class CUrl
{
  ATL_URL_PORT m_nPortNumber;
  ....
  inline BOOL Parse(_In_z_ LPCTSTR lpszUrl)
  {
    ....
    //get the port number
    m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf);
    if (m_nPortNumber < 0)
      goto error;
    ....
};

V547 Expression 'm_nPortNumber < 0' is always false. Unsigned type value is never < 0. atlutil.h 2775

The check that the port number is below zero doesn't work. The 'm_nPortNumber' variable has the unsigned type ' WORD'. The 'WORD' type is 'unsigned short'.

Undefined behavior

The Visual C++ header files contain the following macro.

#define DXVABitMask(__n) (~((~0) << __n))

Wherever it is used, undefined behavior occurs. Of course, the Visual C++ developers know better if this construct is safe or not. Perhaps they assume that Visual C++ will always handle negative number shifts in the same way. Formally, a negative number shift causes undefined behavior. This subject is discussed in detail in the article "Wade not in unknown waters. Part three".

Incorrect operation in 64-bit mode

This pattern of 64-bit errors is discussed in detail in the series of lessons we have written on 64-bit C/C++ software development. To understand the point of the error, please see lesson 12.

class CWnd : public CCmdTarget
{
  ....
  virtual void WinHelp(DWORD_PTR dwData,
                       UINT nCmd = HELP_CONTEXT);
  ....
};

class CFrameWnd : public CWnd
{
  ....
};

class CFrameWndEx : public CFrameWnd
{
  ....
  virtual void WinHelp(DWORD dwData,
                       UINT nCmd = HELP_CONTEXT);
  ....
};

V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'CFrameWndEx' and base class 'CFrameWnd'. afxframewndex.h 154

The 'WinHelp' function is declared in the 'CFrameWndEx' class incorrectly. The first argument should have the 'DWORD_PTR' type. The same error can be found in some other classes:

A pointer is used first and then compared to NULL

We have found quite a lot of such fragments. It's rather tiresome to check whether or not each particular case is dangerous - the libraries' authors will be better at this. We will only cite a couple of samples.

BOOL CDockablePane::PreTranslateMessage(MSG* pMsg)
{
  ....
  CBaseTabbedPane* pParentBar = GetParentTabbedPane();
  CPaneFrameWnd* pParentMiniFrame =
    pParentBar->GetParentMiniFrame();
  if (pParentBar != NULL &&
      (pParentBar->IsTracked() ||
       pParentMiniFrame != NULL &&
       pParentMiniFrame->IsCaptured()
      )
     )
  ....
}

V595 The 'pParentBar' pointer was utilized before it was verified against nullptr. Check lines: 2840, 2841. afxdockablepane.cpp 2840

Look, at first the 'pParentBar' pointer is used to call the GetParentMiniFrame() function. Then the programmer suddenly suspects this pointer might equal NULL and makes a check for that.

AFX_CS_STATUS CDockingManager::DeterminePaneAndStatus(....)
{
  ....
  CDockablePane* pDockingBar =
    DYNAMIC_DOWNCAST(CDockablePane, *ppTargetBar);

  if (!pDockingBar->IsFloating() &&
      (pDockingBar->GetCurrentAlignment() &
       dwEnabledAlignment) == 0)
  {
    return CS_NOTHING;
  }
  if (pDockingBar != NULL)
  {
    return pDockingBar->GetDockingStatus(
      pt, nSensitivity);
  }
  ....
}

V595 The 'pDockingBar' pointer was utilized before it was verified against nullptr. Check lines: 582, 587. afxdockingmanager.cpp 582

At first the 'pDockingBar' pointer is actively used and then is suddenly compared to NULL.

And one more example:

void CFrameImpl::AddDefaultButtonsToCustomizePane(....)
{
  ....
  for (POSITION posCurr = lstOrigButtons.GetHeadPosition();
       posCurr != NULL; i++)
  {
    CMFCToolBarButton* pButtonCurr =
      (CMFCToolBarButton*)lstOrigButtons.GetNext(posCurr);

    UINT uiID = pButtonCurr->m_nID;

    if ((pButtonCurr == NULL) ||
        (pButtonCurr->m_nStyle & TBBS_SEPARATOR) ||
        (....)
    {
      continue;
    }
  ....
}

V595 The 'pButtonCurr' pointer was utilized before it was verified against nullptr. Check lines: 1412, 1414. afxframeimpl.cpp 1412

The programmer feels sure to address the 'm_nID' class member. But then we see in the condition that the 'pButtonCurr' pointer is checked for being a null pointer.

Using a destroyed object

CString m_strBrowseFolderTitle;

void CMFCEditBrowseCtrl::OnBrowse()
{
  ....
  LPCTSTR lpszTitle = m_strBrowseFolderTitle != _T("") ?
    m_strBrowseFolderTitle : (LPCTSTR)NULL;
  ....
}

V623 Consider inspecting the '?:' operator. A temporary object is being created and subsequently destroyed. afxeditbrowsectrl.cpp 308

The ternary operator cannot return values of different types. That's why an object of the CString type will be implicitly created out of "(LPCTSTR)NULL". Then out of this empty string a pointer to its buffer will be implicitly taken. The trouble is that the temporary object of the CString type will be destroyed. As a result, the 'lpszTitle' pointer's value will become non-valid and you won't be able to handle it. Here you can find a detailed description of this error pattern.

Incorrect time handling

UINT CMFCPopupMenuBar::m_uiPopupTimerDelay = (UINT) -1;
....
void CMFCPopupMenuBar::OnChangeHot(int iHot)
{
  ....
  SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE,
    max(0, m_uiPopupTimerDelay - 1),
    NULL);
  ....
}

V547 Expression '(0) > (m_uiPopupTimerDelay - 1)' is always false. Unsigned type value is never < 0. afxpopupmenubar.cpp 968

Value '-1' is used as a special flag. The programmers used the 'max' macros in an attempt to protect the code against negative values in the m_uiPopupTimerDelay variable. It won't work because the variable has the unsigned type. It is always above or equal to zero. The correct code should look something like this:

SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE,
  m_uiPopupTimerDelay == (UINT)-1 ? 0 : m_uiPopupTimerDelay - 1,
  NULL);

The same error can be found here:

A meaningless string

BOOL CMFCTasksPaneTask::SetACCData(CWnd* pParent, CAccessibilityData& data)
{
  ....
  data.m_nAccHit = 1;
  data.m_strAccDefAction = _T("Press");
  data.m_rectAccLocation = m_rect;
  pParent->ClientToScreen(&data.m_rectAccLocation);
  data.m_ptAccHit;
  return TRUE;
}

V607 Ownerless expression 'data.m_ptAccHit'. afxtaskspane.cpp 96

What is "data.m_ptAccHit;" ? Maybe the programmer wanted to assign some value to the variable but forgot to?

Additional 0 might be missing

BOOL CMFCTasksPane::GetMRUFileName(....)
{
  ....
  const int MAX_NAME_LEN = 512;

  TCHAR lpcszBuffer [MAX_NAME_LEN + 1];
  memset(lpcszBuffer, 0, MAX_NAME_LEN * sizeof(TCHAR));

  if (GetFileTitle((*pRecentFileList)[nIndex],
                   lpcszBuffer, MAX_NAME_LEN) == 0)
  {
    strName = lpcszBuffer;
    return TRUE;
  }
  ....
}

V512 A call of the 'memset' function will lead to underflow of the buffer 'lpcszBuffer'. afxtaskspane.cpp 2626

I suspect that this code may return a string that won't end with a terminal null. Most likely, the last array item should have been cleared too:

memset(lpcszBuffer, 0, (MAX_NAME_LEN + 1) * sizeof(TCHAR));

Strange 'if'

void CMFCVisualManagerOfficeXP::OnDrawBarGripper(....)
{
  ....
    if (bHorz)
    {
      rectFill.DeflateRect(4, 0);
    }
    else
    {
      rectFill.DeflateRect(4, 0);
    }
  ....
}

V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanagerofficexp.cpp 264

A dangerous class single_link_registry

If you use the 'single_link_registry' class, your application might unexpectedly terminate even if you handle all the exceptions correctly. Let's have a look at the destructor of the 'single_link_registry' class:

virtual ~single_link_registry()
{
  // It is an error to delete link registry with links
  // still present
  if (count() != 0)
  {
    throw invalid_operation(
      "Deleting link registry before removing all the links");
  }
}

V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. agents.h 759

This destructor may throw an exception. This is a bad idea. If an exception is thrown in a program, objects are starting to be destroyed through calling the destructor. If an error occurs in the 'single_link_registry' class's destructor, one more exception will be generated which is not processed in the destructor. As a result, the C++ library will immediately crash by calling the terminate() function.

The same poor destructors:

One more strange loop

void CPreviewView::OnPreviewClose()
{
  ....
  while (m_pToolBar && m_pToolBar->m_pInPlaceOwner)
  {
    COleIPFrameWnd *pInPlaceFrame =
      DYNAMIC_DOWNCAST(COleIPFrameWnd, pParent);
    if (!pInPlaceFrame)
      break;

    CDocument *pViewDoc = GetDocument();
    if (!pViewDoc)
      break;
    // in place items must have a server document.
    COleServerDoc *pDoc =
      DYNAMIC_DOWNCAST(COleServerDoc, pViewDoc);
    if (!pDoc)
      break;
    // destroy our toolbar
    m_pToolBar->DestroyWindow();
    m_pToolBar = NULL;
    pInPlaceFrame->SetPreviewMode(FALSE);
    // restore toolbars
    pDoc->OnDocWindowActivate(TRUE);
    break;
  }
  ....
}

V612 An unconditional 'break' within a loop. viewprev.cpp 476

The loop doesn't contain any 'continue' operator. There is 'break' at the end of the loop. This is very strange. The loop always iterates only once. This is either an error or 'while' should be replaced with 'if'.

A strange constant

There are other non-crucial remarks concerning the code which are not interesting to enumerate. Let's only cite just one example for you to understand what we mean.

The afxdrawmanager.cpp has a constant for the Pi number defined for some reason:

const double AFX_PI = 3.1415926;

V624 The constant 3.1415926 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. afxdrawmanager.cpp 22

This is not an error, of course, and the constant is accurate enough. But we don't understand why not use the M_PI constant which is defined much more accurately:

#define M_PI 3.14159265358979323846

Addressing the Visual C++ developers

Unfortunately, we don't have a project and make-files to build the libraries included into Visual C++. That's why our analysis is rather shallow. We have just found something and reported about it. Don't think there are no other fragments that need reviewing :).

We are sure that you'll find it much more convenient to use PVS-Studio to check the libraries. If you need, we are ready to give you all the necessary recommendations and help to integrate the tool into the make-files. It will be also easier for you to decide whether certain fragments are errors or not.

Conclusions

You see, Visual Studio 2012 has a static analysis unit for C/C++ code. But it doesn't mean this is enough. This is only the first step. It's just an easy and cheap opportunity to try using the new technology for code quality enhancing. And when you like it - you're welcome to contact us and purchase PVS-Studio. This tool fights defects much more intensively. It is designed to do this. We make money on it, which means we're developing it very actively.

We have found errors in the Visual C++ libraries, although they have their own static analysis there. We have found errors in the Clang compiler, although it has its own static analysis. Purchase our tool and we will regularly find errors in your project. Our analyzer integrates very smoothly into Visual Studio 2005, 2008, 2010, 2012 and is capable of searching for errors in the background.

You can download and try PVS-Studio here: http://www.viva64.com/en/pvs-studio/.