Обнаружены ошибки в библиотеках Visual C++ 2012

Андрей Карпов
Статей: 316


Одной из методик выявления ошибок в программах является статический анализ кода. Мы рады, что это методология становится всё более популярной. Во многом этому способствует Visual Studio, в которой статический анализ является одной из многих функциональных возможностей. Эту функциональность легко попробовать и начать регулярно использовать. Когда человек понимает, что ему нравится статический анализ кода, мы рады предложить ему профессиональный анализатор PVS-Studio для языков Си/Си++/Си++11.

Введение

Среда разработки Visual Studio позволяет осуществлять статический анализ кода. Этот анализ крайне полезен и прост в использовании. Однако следует понимать, что Visual Studio выполняет огромное количество функций. Это значит, что отдельно взятая функция всегда проигрывает специализированным инструментам. Функции рефакторинга и раскраски кода проигрывают по сравнению с Visual Assist. Функция для встроенного редактирования изображений естественно хуже, чем Adobe Photoshop или CorelDRAW. Аналогично дело обстоит и с функцией статического анализа кода.

Не будем теоретизировать. Перейдём к практике. Посмотрим, что удалось заметить интересного анализатору PVS-Studio в папках Visual Studio 2012.

На самом деле, мы не планировали проверять исходные файлы, входящие в состав Visual Studio. Всё вышло случайно. В Visual Studio 2012 в связи с поддержкой нового стандарта языка Си++11, многие заголовочные файлы подверглись изменениям. Встала задача убедиться, что анализатор PVS-Studio способен обработать входящие в него заголовочные файлы.

Неожиданно для себя, в заголовочных *.h файлах мы заметили несколько ошибок. Мы решили поподробнее изучить файлы, входящие в Visual Studio 2012. А именно такие папки как:

  • Program Files (x86)\Microsoft Visual Studio 11.0\VC\include
  • Program Files (x86)\Microsoft Visual Studio 11.0\VC\crt
  • Program Files (x86)\Microsoft Visual Studio 11.0\VC\atlmfc

Полноценной проверки не получилось, так как у нас нет проектов или make-файлов для сборки библиотек. Так что, удалось проверить только скромную часть кода библиотек. Не смотря на незавершенность проверки, результаты весьма интересны.

Давайте познакомимся, что нашел анализатор PVS-Studio внутри библиотек для Visual C++. Как видно, все эти ошибки просочились мимо анализатора, встроенного в сам Visual C++.

Некоторые из найденных подозрительных мест

Не будем утверждать, что все приведённые ниже фрагменты кода содержат ошибки. Мы просто выбрали из списка предложенного анализатором PVS-Studio места, которые кажутся наиболее подозрительными.

Странный цикл

Этот подозрительный код был найден первым. Он и послужил толчком к продолжению исследования.

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

Тело цикла выполняется только один раз. Ошибка в пояснениях не нуждается. Скорее всего, оператор 'return' должен быть вызван, когда найдено искомое значение. В этом случае код должен выглядеть так:

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

Подозрительная проекция

Прошу прощения за тяжёлый для прочтения пример. Обратите внимание на условие в тернарном операторе.

// 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

В условии два раза повторяется выражение "_CTR(_Ty)::_Isinf(real(_Left))". Затруднительно сказать, есть ли здесь ошибка и как следует исправить код. Однако эта функция явно заслуживает внимания.

Ненужная проверка

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

Ошибки здесь нет. Если судить по коду, то длина строки отрицательной стать не может. В классе CSimpleStringT имеются соответствующие проверки. То, что переменная nOldLength имеет беззнаковый тип, ничего не портит. Все равно длина всегда положительна. Это просто лишний код.

Неправильное формирование строки

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

Этот код формирует неправильное сообщение в UNICODE программах. Функция 'Format()' ожидает, что восьмой аргумент будет иметь тип LPCTSTR. Но переменная 'szFontName' всегда имеет тип LPCSTR.

Порт с отрицательным номером

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

Проверка, что номер порта меньше нуля, не сработает. Переменная 'm_nPortNumber' имеет беззнаковый тип ' WORD'. Тип 'WORD' это 'unsigned short'.

Неопределённое поведение

В заголовочных файлах Visual C++ есть следующий макрос.

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

Везде где он используется, возникает неопределённо поведение. Конечно, разработчикам Visual C++ виднее, безопасна эта конструкция или нет. Возможно, есть уверенность, что Visual C++ будет всегда одинаково обрабатывать сдвиг отрицательных чисел. Формально, сдвиг отрицательного числа приводит к Undefined behavior. Подробнее эта тематика освещена в статье "Не зная брода, не лезь в воду. Часть третья".

Некорректная работа в 64-битном режиме

Данный паттерн 64-битной ошибки подробно рассмотрен в составленных нами уроках по разработке 64-битных приложений на языке Си/Си++. Чтобы понять, в чем состоит ошибка, предлагаем ознакомиться с уроком под номером 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

Функция 'WinHelp' объявлена в классе 'CFrameWndEx' неправильно. Тип первого аргумента должен быть 'DWORD_PTR'. Аналогичную ошибку можно видеть и в некоторых других классах:

  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'CMDIFrameWndEx' and base class 'CFrameWnd'. afxmdiframewndex.h 237
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'CMDIFrameWndEx' and base class 'CMDIFrameWnd'. afxmdiframewndex.h 237
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'COleIPFrameWndEx' and base class 'CFrameWnd'. afxoleipframewndex.h 130
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'COleIPFrameWndEx' and base class 'COleIPFrameWnd'. afxoleipframewndex.h 130
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'COleDocIPFrameWndEx' and base class 'CFrameWnd'. afxoledocipframewndex.h 129
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'COleDocIPFrameWndEx' and base class 'COleIPFrameWnd'. afxoledocipframewndex.h 129
  • V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'COleDocIPFrameWndEx' and base class 'COleDocIPFrameWnd'. afxoledocipframewndex.h 129

Указатель в начале используется, а затем сравнивается с NULL

Таких мест было найдено достаточно много. Анализировать, опасен каждый конкретный случай или нет, весьма утомительно. Намного лучше с этим справятся создатели библиотек. Мы приведём только парочку примеров.

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

Смотрите, в начале указатель 'pParentBar' используется для вызова функции GetParentMiniFrame(). Затем вдруг появляется подозрение, что этот указатель может быть равен NULL и выполняется соответствующая проверка.

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

В начале указатель 'pDockingBar' активно используется, а потом вдруг сравнивается с NULL.

И ещё один пример напоследок:

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

Смело обращаемся к члену класса 'm_nID'. А затем из условия видно, что указатель 'pButtonCurr' может быть равен 0.

Использование разрушенного объекта

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

Тернарный оператор не может возвращать значения разных типов. Поэтому, из "(LPCTSTR)NULL" будет неявно создан объект типа CString. Затем из этой пустой строки будет неявно взят указатель на её буфер. Беда в том, что временный объект типа CString будет разрушен. В результате значения указателя 'lpszTitle' становится не валидным и работать с ним нельзя. Здесь можно почитать более подробное описание данного паттерна ошибки.

Неправильная работа с временем

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

Значение '-1' используется как специальный флаг. Используя макрос 'max' программисты хотели защититься от отрицательных значений в переменной m_uiPopupTimerDelay. Это не получится, так как переменная имеет беззнаковый тип. Она всегда больше или равна нулю. Корректный код должен выглядеть приблизительно так:

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

Аналогичная ошибка находится здесь:

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

Бессмысленная строка

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

Что такое "data.m_ptAccHit;" ? Быть может здесь забыли присвоить этой переменной какое-то значение?

Возможно нужен ещё один 0?

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

Есть подозрение, что этот код может вернуть строку, которая не будет завершаться терминальным нулём. Скорее всего, следовало обнулить и последний элемент массива:

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

Странный '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

Опасный класс single_link_registry

Если вы используете класс 'single_link_registry' то ваше приложение может неожиданно завершиться, даже если вы корректно обрабатываете все исключения. Посмотрим на деструктор класса 'single_link_registry':

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

Этот деструктор может бросить исключение. Это плохая идея. Если в программе возникает исключение, начинается разрушение объектов путем вызова деструкторов. Если в деструкторе 'single_link_registry' произойдет ошибка, то возникнет ещё еще одно исключение. Оно в деструкторе не обрабатывается. В результате библиотека C++ немедленно аварийно завершает программу, вызывая функцию terminate().

Аналогичные плохие деструкторы:

  • V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. concrt.h 4747
  • V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. agents.h 934
  • V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. taskcollection.cpp 880

Ещё один странный цикл

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

В цикле нет ни одного оператора 'continue'. В конце цикла стоит 'break'. Это очень странно. Цикл всегда выполняется только один раз. Здесь или ошибка или лучше заменить 'while' на 'if'.

Странная константа

Есть и другие несущественные замечания к коду, которые перечислять не интересно. Приведём только один пример, чтобы было понятно, о чём идёт речь.

В файле afxdrawmanager.cpp зачем то заведена константа для числа Пи:

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

Это конечно не ошибка и точности константы достаточно. Но не понятно, почему-бы не использовать константу M_PI, которая задана гораздо точнее:

#define M_PI 3.14159265358979323846

Обращение к разработчикам Visual C++

К сожалению, у нас нет проекта и make-файлов для сборки библиотек, входящих в состав Visual C++. Поэтому наш анализ весьма поверхностен. Мы просто что-то нашли и написали про это. Не стоит думать, что нет других подозрительных мест :).

Уверены, что вам будет гораздо удобнее воспользоваться PVS-Studio для проверки библиотек. Если потребуется, мы готовы подсказать и помочь с интеграцией в make-файлы. Также вам будет легче понять, что является ошибкой, а что нет.

Выводы

Смотрите, в Visual Studio 2012 есть статический анализ Си/Си++ кода. Однако это не означает, что этого достаточно. Это только первый шаг. Это только возможность легко и дёшево попробовать новую технологию для повышения качества кода. А когда понравится - приходите к нам и приобретайте PVS-Studio. Этот инструмент гораздо интенсивнее борется с дефектами. Он под это заточен. На нём мы зарабатываем деньги, а значит, очень активно его развиваем.

Мы нашли ошибки в библиотеках Visual C++, хотя там есть статический анализ. Мы нашли ошибки в компиляторе Clang, хотя в нём есть статический анализ. Приобретайте нас и мы будем регулярно находить ошибки в вашем проекте. Мы отлично интернируемся в Visual Studio 2005, 2008, 2010, 2012 и умеем искать ошибки в фоновом режиме.

Скачать и попробовать PVS-Studio можно здесь: http://www.viva64.com/ru/pvs-studio/.



Используйте PVS-Studio для поиска ошибок в C, C++ и C# коде

Предлагаем попробовать проверить код вашего проекта с помощью анализатора кода PVS-Studio. Одна найденная в нём ошибка скажет вам о пользе методологии статического анализа кода больше, чем десяток статей.

goto PVS-Studio;

Андрей Карпов
Статей: 316


А ты совершаешь ошибки в коде?

Проверь с помощью
PVS-Studio

Статический анализ
кода для C, C++ и C#

goto PVS-Studio;