Как используя PVS-Studio можно улучшить Visual C++ 2017 Libraries02.05.2017 15:48

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

Кстати, предлагаю в начале самостоятельно попробовать найти ошибку, не читая статью дальше.

void CPaneContainerManager::RemoveAllPanesAndPaneDividers()
{
  ASSERT_VALID(this);
  POSITION pos = NULL;

  for (pos = m_lstControlBars.GetHeadPosition(); pos != NULL;)
  {
    POSITION posSave = pos;
    CBasePane* pWnd = DYNAMIC_DOWNCAST(
      CBasePane, m_lstControlBars.GetNext(pos));
    ASSERT_VALID(pWnd);

    if (pWnd->IsPaneVisible())
    {
      m_lstControlBars.RemoveAt(posSave);
    }
  }

  for (pos = m_lstSliders.GetHeadPosition(); pos != NULL;)
  {
    POSITION posSave = pos;
    CBasePane* pWnd = DYNAMIC_DOWNCAST(
      CBasePane, m_lstControlBars.GetNext(pos));
    ASSERT_VALID(pWnd);

    if (pWnd->IsPaneVisible())
    {
      m_lstSliders.RemoveAt(posSave);
    }
  }
}

Готов поспорить, что многие сдались и решили читать статью дальше. Это хороший пример, почему статические анализаторы важны и нужны: они не ленятся и не устают.

Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'm_lstSliders' variable should be used instead of 'm_lstControlBars'. afxpanecontainermanager.cpp 1645

Впрочем, даже прочитав предупреждение анализатора, не просто найти ошибку в коде. Поэтому я сокращу его, оставив важное для нас:

for (... m_lstControlBars ...)
{
  CBasePane* pWnd = ... m_lstControlBars ...
  m_lstControlBars.RemoveAt();
}

for (... m_lstSliders ...)
{
  CBasePane* pWnd = ... m_lstControlBars ...
  m_lstSliders.RemoveAt();
}

С вероятностью 99%, второй цикл был написан с использованием технологии Copy-Paste: был взят первый цикл, скопирован, а затем в нем заменили имя m_lstControlBars на m_lstSliders. Но в одном месте заменить имя забыли!

Ошибка здесь: CBasePane* pWnd =… m_lstControlBars…

Это была хорошая ошибка, но следующая не уступает ей по красоте. Рассмотрим, как реализованы операторы инкремента/декремента в классе CMFCScanliner:

class CMFCScanliner
{
  ....
  inline  const CMFCScanliner& operator ++ ()
  {
    m_line += m_offset;
    return *this;
  }

  inline  const CMFCScanliner& operator ++ (int)
  {
    m_line += m_offset;
    return *this;
  }

  inline  const CMFCScanliner& operator -- ()
  {
    m_line -= m_offset;
    return *this;
  }

  inline  const CMFCScanliner& operator -- (int)
  {
    m_line += m_offset;
    return *this;
  }
  ....
};

Обратите внимание на реализацию самого последнего оператора. Забыли поменять += на -=. Это классика! Это «эффект последней строки» в действии!

Нашлось три места, где могут возникать утечки. Вот одно из них:

CSpinButtonCtrl* CMFCPropertyGridProperty::CreateSpinControl(
  CRect rectSpin)
{
  ASSERT_VALID(this);
  ASSERT_VALID(m_pWndList);

  CSpinButtonCtrl* pWndSpin = new CMFCSpinButtonCtrl;

  if (!pWndSpin->Create(WS_CHILD | WS_VISIBLE | UDS_ARROWKEYS |
                        UDS_SETBUDDYINT | UDS_NOTHOUSANDS,
                        rectSpin, m_pWndList,
                        AFX_PROPLIST_ID_INPLACE))
  {
    return NULL;
  }
  ....
}

Если функция Create будет выполнена с ошибкой, то не будет удалён объект, указатель на который хранится в переменной pWndSpin.

Аналогично:

• V773 The function was exited without releasing the 'pList' pointer. A memory leak is possible. afxribboncombobox.cpp 461
• V773 The function was exited without releasing the 'pButton' pointer. A memory leak is possible. afxvslistbox.cpp 222

typedef void *PVOID;
typedef PVOID PSECURITY_DESCRIPTOR;

class CSecurityDescriptor
{
  ....
  PSECURITY_DESCRIPTOR m_pSD;
  ....
};

inline CSecurityDescriptor::~CSecurityDescriptor()
{
  delete m_pSD;        // <= Член m_pSD имеет тип void *
  free(m_pOwner);
  free(m_pGroup);
  free(m_pDACL);
  free(m_pSACL);
}

Аналогичные проблемы можно найти здесь:

• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1048
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1070
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1667
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. afxstate.cpp 265
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1240
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1250
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. doccore.cpp 1654
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dockstat.cpp 343
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 43
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 49
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. sockcore.cpp 541
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 145
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 465
• V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. mapiunicodehelp.h 168

CSize CMFCReBar::CalcFixedLayout(BOOL bStretch, BOOL bHorz)
{
  ASSERT_VALID(this);
  ENSURE(::IsWindow(m_hWnd));

  // the union of the band rectangles is the total bounding rect
  int nCount = (int) DefWindowProc(RB_GETBANDCOUNT, 0, 0);
  REBARBANDINFO rbBand;
  rbBand.cbSize = m_nReBarBandInfoSize;
  int nTemp;

  // sync up hidden state of the bands
  for (nTemp = nCount; nTemp--; )
  {
    rbBand.fMask = RBBIM_CHILD|RBBIM_STYLE;
    VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp,
                         (LPARAM)&rbBand));
    CPane* pBar = DYNAMIC_DOWNCAST(
      CPane, CWnd::FromHandlePermanent(rbBand.hwndChild));
    BOOL bWindowVisible;
    if (pBar != NULL)
      bWindowVisible = pBar->IsVisible();
    else
      bWindowVisible = (::GetWindowLong(
        rbBand.hwndChild, GWL_STYLE) & WS_VISIBLE) != 0;
    BOOL bBandVisible = (rbBand.fStyle & RBBS_HIDDEN) == 0;
    if (bWindowVisible != bBandVisible)
      VERIFY(DefWindowProc(RB_SHOWBAND, nTemp, bWindowVisible));
  }

  // determine bounding rect of all visible bands
  CRect rectBound; rectBound.SetRectEmpty();
  for (nTemp = nCount; nTemp--; )
  {
    rbBand.fMask = RBBIM_STYLE;
    VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp,
                         (LPARAM)&rbBand));
    if ((rbBand.fStyle & RBBS_HIDDEN) == 0)
    {
      CRect rect;
      VERIFY(DefWindowProc(RB_GETRECT, nTemp, (LPARAM)&rect));
      rectBound |= rect;
    }
  }

  // add borders as part of bounding rect
  if (!rectBound.IsRectEmpty())
  {
    CRect rect; rect.SetRectEmpty();
    CalcInsideRect(rect, bHorz);
    rectBound.right -= rect.Width();
    rectBound.bottom -= rect.Height();
  }
  bStretch = 1;
  return CSize((bHorz && bStretch) ? 32767 : rectBound.Width(),
    (!bHorz && bStretch) ? 32767 : rectBound.Height());
}

Строка «bStretch = 1;» выглядит так, как будто её кто-то написал для отладки, а потом забыл удалить. Возможно, именно так и произошло. Прошу разработчиков проверить и разобраться с этим странным кодом.

Рассмотрим, как объявлена функция AdjustDockingLayout в классе CBasePane и CDockSite.

class CBasePane : public CWnd
{
  ....
  virtual void AdjustDockingLayout(HDWP hdwp = NULL);
  ....
};

class CDockSite : public CBasePane
{
  ....
  virtual void AdjustDockingLayout();
  ....
};

Мне кажется, что в какой-то момент в базовом классе в объявлении функции добавили аргумент hdwp, а в классе-наследнике забыли. В результате, теперь это две разные функции.

Схожие ситуации:

• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CPane' and base class 'CBasePane'. afxpane.h 96
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CDockablePane' and base class 'CPane'. afxdockablepane.h 184
• V762 It is possible a virtual function was overridden incorrectly. See second argument of function 'SizeToContent' in derived class 'CMFCLinkCtrl' and base class 'CMFCButton'. afxlinkctrl.h 50
• V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'RecalcLayout' in derived class 'CMFCTasksPane' and base class 'CPane'. afxtaskspane.h 287

class CAccessToken
{
  ....
  mutable CRevert *m_pRevert;
};

inline bool
CAccessToken::ImpersonateLoggedOnUser() const throw(...)
{
  ....
  delete m_pRevert;
  m_pRevert = _ATL_NEW CRevertToSelf;
  ....
}

Разберёмся, почему он это делает. Нас интересует член m_pRevert, который является указателем на объект типа CRevert. Класс используется полиморфно. К такому заключению можно прийти, посмотрев на эту строку кода:

m_pRevert = _ATL_NEW CRevertToSelf;

class CRevert
{
public:
  virtual bool Revert() throw() = 0;
};

class CRevertToSelf : public CRevert
{
public:
  bool Revert() throw()
  {
    return 0 != ::RevertToSelf();
  }
};

Мы не только добавляем в анализатор PVS-Studio новые диагностики, но и усовершенствуем уже имеющиеся. Например, недавно диагностика V611 научилась искать проблему освобождения памяти, когда процесс выделения и освобождения памяти находится в разных функциях. Сейчас я продемонстрирую, как это работает на практике.

template 
class CBulkRowset : public CRowset
{
  ....
  void SetRows(_In_ DBROWCOUNT nRows) throw()
  {
    if (nRows == 0)
      nRows = 10;
    if (nRows != m_nRows)
    {
      delete m_phRow;
      m_phRow = NULL;
      m_nRows = nRows;
    }
  }

  HRESULT BindFinished() throw()
  {
    m_nCurrentRows = 0;
    m_nCurrentRow  = 0;
    m_hr = S_OK;

    if (m_phRow == NULL)
    {
      m_phRow = _ATL_NEW HROW[m_nRows];
      if (m_phRow == NULL)
        return E_OUTOFMEMORY;
    }

    return S_OK;
  }
  ....
  HROW*   m_phRow;
  ....
}

Память выделяется в функции BindFinished с помощью оператора new []:

m_phRow = _ATL_NEW HROW[m_nRows];

delete m_phRow;

Теперь поговорим об одном очень подозрительном вызове функции memset. Однако, прежде чем смотреть некорректный код, взглянем на код, где всё хорошо.

Правильный код:

void CToolTipCtrl::FillInToolInfo(TOOLINFO& ti, ....) const
{
  memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
  ti.cbSize = sizeof(AFX_OLDTOOLINFO);
  ....
}

В приведённом выше коде всё логично. Вычисляется размер структуры AFX_OLDTOOLINFO. Далее этот размер структуры используется для вызова функции memset и этот же размер записывается внутрь структуры.

Теперь рассмотрим аномальный код:

BOOL CControlBar::PreTranslateMessage(MSG* pMsg)
{
  ....
  TOOLINFO ti; memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
  ti.cbSize = sizeof(TOOLINFO);
  ....
}

Структура имеет тип TOOLINFO. Именно размер структуры TOOLINFO и будет записан в неё: ti.cbSize = sizeof (TOOLINFO); .

Однако, обнуляется структура не целиком, а только частично, так как количество обнуляемых байт вычисляется с помощью выражения sizeof (AFX_OLDTOOLINFO).

В итоге, часть полей структуры останется неинициализированными.

Есть ещё один случай, когда memset заполняет не всю структуру.

GUID m_Id;
void zInternalStart()
{
  ....
  // Zero the activity id in case we end up logging the stop.
  ZeroMemory(&m_Id, sizeof(&m_Id));
  ....
}

Классическая ошибка, когда вместо размера структуры вычисляется размер указателя. В результате, обнулятся только первые 4 или 8 байт, в зависимости от того, компилируется 32-битное или 64-битное приложение. Структура GUID же занимает 16 байт (128 бит).

Правильный вариант:

ZeroMemory(&m_Id, sizeof(m_Id));

Суть ошибки заключается в том, что разыменование указателя происходит до его проверки.

Рассмотрим фрагмент кода, выявленный с помощью этой диагностики.

HRESULT CBasePane::get_accHelp(VARIANT varChild, BSTR *pszHelp)
{
  if ((varChild.vt == VT_I4) && (varChild.lVal == CHILDID_SELF))
  {
    *pszHelp = SysAllocString(L"ControlPane");
    return S_OK;
  }

  if (((varChild.vt != VT_I4) && (varChild.lVal != CHILDID_SELF))
      || (NULL == pszHelp))
  {
    return E_INVALIDARG;
  }
  ....
}

Если функцию вызвать вот так:

VARIANT foo;
foo.vt = VT_I4;
foo.lVal = CHILDID_SELF;
get_accHelp(foo, NULL);

Анализатор рассуждает следующим образом. Указатель разыменовывается. Однако, ниже есть проверка на равенство этого указателя NULL. Раз есть такая проверка, то видимо указатель может быть нулевым. Если он будет нулевым, произойдет беда. Ага, надо предупредить!

Как я уже сказал, это весьма распространённая ошибка. Библиотеки Visual C++ не стали исключением. Вот ещё 17 мест, которые ждут рефакторинга: vs2017_V595.txt.

И рассмотрим завершающую ошибку, связанную с путаницей между константами FALSE и S_FALSE.

BOOL CMFCRibbonPanel::OnSetAccData (long lVal)
{
  ....
  if (nIndex < 0 || nIndex >= arElements.GetSize())
  {
    return FALSE;
  }

  if (GetParentWnd()->GetSafeHwnd() == NULL)
  {
    return S_FALSE;
  }

  ASSERT_VALID(arElements[nIndex]);
  return arElements[nIndex]->SetACCData(GetParentWnd(), m_AccData);
}

Функция возвращает тип BOOL. Если не удалось получить HWND у родительского окна, программист хотел вернуть из функции значения FALSE. Но опечатался и написал S_FALSE, что всё радикально меняет.

Вот как объявлена константа S_FALSE:

#define S_FALSE ((HRESULT)1L)

Написать «return S_FALSE;» это тоже самое, что написать «return TRUE;». Epic fail.

Ошибка не одинока, у неё есть друзья:

• V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5623
• V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5627
• V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. ctlnownd.cpp 349
• V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. olecli2.cpp 548

Примечание

Заключение

© Habrahabr.ru