Как используя PVS-Studio можно улучшить Visual C++ 2017 Libraries

Название статьи намекает разработчикам Visual Studio, что они могут получать пользу от использования статического анализатора кода PVS-Studio. В статье приводятся результаты анализа библиотек, входящих в состав недавно выпущенной версии Visual C++ 2017, и даются рекомендации по улучшению и устранению ошибок. Приглашаю читателей узнать, как разработчики Visual C++ Libraries отстреливают ноги: будет интересно и познавательно.

Немного предыстории

Это уже не первый мой эксперимент по проверке библиотек, входящих в состав Visual C++. Про предыдущие проверки можно почитать здесь:

• Обнаружены ошибки в библиотеках Visual C++ 2012
• Тяп-ляп, проверил библиотеки Visual C++ 2013 (update 3)

После этих публикаций был долгий перерыв, и я не написал статью касательно VS2015. Было много интересных проектов, которые ждали проверки. Хотя, если быть честным, я просто забыл написать статью по этой тематике. К счастью, твит одного из разработчиков Visual C++ (@MalwareMinigun) напомнил мне про VS2017:

I'm surprised we don't have people yelling at us all the time for stuff you folks find in standard library headers.

Действительно, я не рассказал миру о багах в библиотеках Visual Studio 2017! Что ж, вызов принят!

Как видите, с момента твита (31 марта) прошёл месяц. Признаю, что затянул с ответом, но сейчас исправлюсь.

Что и как проверялось

Для проверки я использовал самую последнюю доступную мне версию анализатора PVS-Studio (6.15).

Проверял я C++ библиотеки, входящие в состав недавно выпущенной версии Visual Studio 2017. Проверялась версия библиотек, которые находились у меня на компьютере 12.04.2017. Впрочем, версия не так важна, так как этот текст не багрепорт, а статья, популяризующая методологию статического анализа вообще, и анализатора PVS-Studio в частности.

Признаюсь, я не утруждал себя полноценной проверкой библиотек, так как это сложно для меня.

Во-первых, мне пришлось откопировать все библиотеки в другую папку, иначе я не смог бы получить для них предупреждения. Анализатор не выдаёт сообщения на системные библиотеки. Копируя файлы в другую папку, я обманываю PVS-Studio, и получаю нужные мне предупреждения.

Кстати, вот и ответ на твит, приведённый выше. Именно по этой причине пользователи Visual C++ не пишут о предупреждениях PVS-Studio, касающихся библиотек. Нет смысла их выдавать, так как они будут только отвлекать людей. Заодно, это немного ускоряет анализ, так как не надо полноценно парсить и анализировать тела inline-функций.

Во-вторых, я не пытался по-честному собирать проекты. Я просто создал новый solution и добавил туда файлы из библиотек. В результате, часть файлов мне не удалось проверить. Но, с точки зрения написания статьи, — это не существенно. Материала для полноценного текста и так получилось вполне достаточно. Более тщательную и правильную проверку стоит выполнить уже самим разработчикам Visual C++, и я готов помогать им в этом.

Ложные срабатывания

В этот раз конкретных чисел и процентов я, к сожалению, привести не могу.

Могу сказать, что:

• Предупреждений общего назначения (GA) с достоверностью High я получил 433 шт.
• Предупреждений общего назначения (GA) с достоверностью Medium я получил 743 шт.

Однако, эти числа никак нельзя интерпретировать, и на их основе нельзя делать никаких выводов!

Напомню, что я проверил только часть файлов, да ещё и странным способом. Плюс с библиотеками есть необычный момент. Анализатор выдаёт много абсолютно правильных сообщений, которые при этом являются абсолютно ложными. Сейчас я объясню причину этого парадокса.

Вредно и опасно самостоятельно объявлять системные типы данных. Пример плохой идеи:

typedef unsigned long       DWORD;

Анализатор PVS-Studio выдаст предупреждение: V677 Custom declaration of a standard 'DWORD' type. The system header file should be used: #include <WinDef.h>.

Анализатор совершенно прав. Следует не объявлять тип «вручную», а подключить соответствующий заголовочный файл.

Как вы понимаете, к библиотекам Visual C++ такая диагностика не применима. Ведь там-то, как раз, и объявляются такие типы. И таких сообщений анализатор выдал более 250.

Или вот ещё один интересный пример. Анализатор PVS-Studio совершенно прав, когда критикует код, в котором указатель this проверяется на равенство NULL. Согласно современному стандарту языка C++, указатель this не может быть NULL.

У Visual C++ с этим большие проблемы. Видимо, в этом плане он никогда не будет соответствовать стандарту, ну или это произойдет очень нескоро. Дело в том, что библиотеки (например, MFC) архитектурно построены так, что this равный NULL — это типовая ситуация.

В коде библиотек присутствует большое количество функций, проверяющих указатель this. Вот пара таких функций:

_AFXWIN_INLINE CDC::operator HDC() const
{ return this == NULL ? NULL : m_hDC; }
_AFXWIN_INLINE HDC CDC::GetSafeHdc() const
{ return this == NULL ? NULL : m_hDC; }

Анализатор PVS-Studio, естественно, выдаст для них предупреждения:

• V704 'this == 0' expression should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 314
• V704 'this == 0' expression should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 316

Таких сообщений более 40, при этом толку, конечно, от них нет. В ближайшие годы их можно считать ложными срабатываниями.

Как мы видим на примере сообщений V677 и V704, не все диагностики применимы к библиотекам Visual C++. Проблемы конечно никакой нет: эти и другие предупреждения можно просто выключить и, тем самым, сразу уменьшить количество сообщений на 300 штук.

Всё это я пишу, чтобы в очередной раз продемонстрировать, что очень часто бессмысленно обсуждать процент ложных срабатываний, если не провести предварительную настройку анализатора.

В общем, в этот раз без процентов, прошу прощения. Моё субъективное мнение — ложных срабатываний мало.

Что интересного нашел анализатор

Я решил пойти от безобидного к страшному. В начале я рассмотрю рекомендации по мелким улучшениям. Потом перейду к нестрашным ошибкам, а закончу повествование тем, что на мой взгляд, можно назвать «жутью». В общем, буду постепенно повышать градус. Итак, вперёд, спасать мир программ от багов!

Микрооптимизации

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

Начнём с предупреждения V808, которое говорит о том, что некий объект создаётся, но не используется. Рассмотрим эту ситуации на примере двух функций.

void CMFCToolBarComboBoxButton::AdjustRect()
{
  ....
  if (m_pWndEdit != NULL)
  {
    CRect rectEdit = m_rect;

    const int iBorderOffset = 3;

    m_pWndEdit->SetWindowPos(
      NULL, m_rect.left + nHorzMargin + iBorderOffset,
      m_rect.top + iBorderOffset,
      m_rect.Width() - 2 * nHorzMargin - m_rectButton.Width() -
        iBorderOffset - 3,
      m_rectCombo.Height() - 2 * iBorderOffset,
      SWP_NOZORDER | SWP_NOACTIVATE);
  }
  ....
}

Предупреждение PVS-Studio: V808 'rectEdit' object of 'CRect' type was created but was not utilized. afxtoolbarcomboboxbutton.cpp 607

Объект rectEdit после создания и инициализации нигде не используется. Он просто лишний и его можно смело удалить. Код станет чуть короче.

Ещё один случай:

BOOL CALLBACK AFX_EXPORT
CMFCToolBarFontComboBox::EnumFamPrinterCallBackEx(....)
{
  ....
  CString strName = pelf->elfLogFont.lfFaceName;

  pCombo->AddFont((ENUMLOGFONT*)pelf, FontType,
                  CString(pelf->elfScript));
  return 1;
}

V808 'strName' object of 'CStringT' type was created but was not utilized. afxtoolbarfontcombobox.cpp 138

Объект типа CString создаётся и инициализируется, но нигде не используется. Не знаю, догадается ли компилятор выбросить лишний код для создания и копирования строки. Возможно, и не догадается, так как CStirng — это сложный класс. Впрочем, это не важно, в любом случае объект strName следует удалить и, тем самым, сделать код проще.

И таких вот лишних объектов очень много. Помимо уже рассмотренных, анализатор выдал ещё 50 сообщений. Чтобы не загромождать текст статьи, я выписал эти сообщения в отдельный файл: vs2017_V808.txt.

Теперь пришло время лишних проверок.

TaskStack::~TaskStack()
{
  if (m_pStack)
    delete [] m_pStack;
}

Предупреждение PVS-Studio: V809 Verifying that a pointer value is not NULL is not required. The 'if (m_pStack)' check can be removed. taskcollection.cpp 29

Оператору delete совершенно безопасно отдать на вход nullptr. Таким образом, проверка является лишней и код можно упростить:

TaskStack::~TaskStack()
{
  delete [] m_pStack;
}

Таких лишних проверок тоже очень много. Я выписал 68 сообщений в файл vs2017_V809.txt.

Следующая мелочь, которую можно изменить — это заменить постфиксный инкремент итераторов на префиксный. Пример:

size_type count(const key_type& _Keyval) const
{
  size_type _Count = 0;
  const_iterator _It = _Find(_Keyval);
  for (;_It != end() && !this->_M_comparator(....); _It++)
  {
    _Count++;
  }
  return _Count;
}

Предупреждение PVS-Studio: V803 Decreased performance. In case '_It' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. internal_concurrent_hash.h 509

Станет чуть лучше, если написать:

for (;_It != end() && !this->_M_comparator(....); ++_It)

Вопрос, есть ли польза в таком рефакторинге, я рассматривал в статье: "Есть ли практический смысл использовать для итераторов префиксный оператор инкремента ++it, вместо постфиксного it++". Ответ: есть, хотя и не большая.

Если авторы библиотек решат, что исправления стоит сделать, то вот ещё 26 аналогичных предупреждений: vs2017_V803.txt.

Следующая микрооптимизация. Лучше очищать строку с помощью вызова функции str.Empty(), чем присваивать ей значение _T(""). Класс ведь не знает заранее, сколько памяти надо выделить под строку. Поэтому он начинает зря считать длину строки. В общем, лишние действия.

CString m_strRegSection;

CFullScreenImpl::CFullScreenImpl(CFrameImpl* pFrameImpl)
{
  m_pImpl = pFrameImpl;
  m_pwndFullScreenBar = NULL;
  m_bFullScreen = FALSE;
  m_bShowMenu = TRUE;
  m_bTabsArea = TRUE;
  m_uiFullScreenID = (UINT)-1;
  m_strRegSection = _T("");
}

Предупреждение PVS-Studio: V815 Decreased performance. Consider replacing the expression 'm_strRegSection = L""' with 'm_strRegSection.Empty()'. afxfullscreenimpl.cpp 52

Здесь лучше заменить строчку

m_strRegSection = _T("");

на

m_strRegSection.Empty();

Мелочь, но перфекционист будет рад такому улучшению кода.

Примечание. На самом деле это строчку вообще можно убрать, так как этот код находится в конструкторе и строка и так пустая.

Вот ещё 27 предупреждений на эту тему: vs2017_V815.txt.

И, напоследок, вот такой код:

HRESULT  GetPropertyInfo(....)
{
  ....
  for(ul=0; ul<m_cPropSetDex; ul++)
  {
    ....
    for(ULONG ulProp=0; ....)
    {
      ....
      pDescBuffer += (wcslen(L"UNKNOWN") + 1);
  ....
}

Предупреждение PVS-Studio: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2374

Обратите внимание, что функция wcslen будет вызываться многократно, так как находится внутри вложенных циклов. Было бы логичнее посчитать заранее и запомнить длину строки L«UNKNOWN».

Ещё одно сообщение: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2438

Всё, с рекомендациями закончили. Давайте перейдём к чему-то более интересному.

Ошибки мелкого и среднего калибра

В заголовочных файлах неправильно подавляются предупреждения компилятора. Рассмотрим неправильный способ временного отключения предупреждений:

#ifdef _MSC_VER
#pragma warning(disable:4200)
#endif

typedef struct adpcmwaveformat_tag {
        WAVEFORMATEX    wfx;
        WORD            wSamplesPerBlock;
        WORD            wNumCoef;
#if defined( _MSC_VER )        
        ADPCMCOEFSET    aCoef[];
#else
        ADPCMCOEFSET    aCoef[1];
#endif        
} ADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT       *PADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT NEAR *NPADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT FAR  *LPADPCMWAVEFORMAT;

#ifdef _MSC_VER
#pragma warning(default:4200)
#endif

Предупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 2610, 2628. mmreg.h 2628

Я понимаю, что увидеть, в чем собственно ошибка — не просто, поэтому я выделю суть.

#pragma warning(disable:4200)
....
#pragma warning(default:4200)

Сначала предупреждение компилятора отключается. Далее состояние предупреждения 4200 устанавливается в значение по умолчанию. Так делать нельзя. Предположим, пользователь вообще отключил диагностику 4200 для одного своего файла. К своему несчастью, в файле он написал:

#include <mmreg.h>

В результате, предупреждение опять включится и будет выдаваться для кода пользователя.

Правильным решением будет сохранить состояние, а потом вернуть предыдущее:

#pragma warning(push)
#pragma warning(disable:4200)
....
#pragma warning(pop)

Вот где ещё в заголовочных файлах неправильно используется pragma warning:

• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 586, 601. workstealingqueue.h 601
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1669, 1697. usbioctl.h 1697
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1631, 1646. usbioctl.h 1646
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1490, 1518. usbioctl.h 1518
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 986, 1002. usbioctl.h 1002
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 960, 978. usbioctl.h 978
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 913, 925. usbioctl.h 925
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 861, 876. usbioctl.h 876
• V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 860, 875. usbioctl.h 875

Есть такие ошибки и в *.cpp файлах, но я их не стал выписывать, так как они не представляют никакой опасности для кода пользователей Visual C++. Хотя, конечно, их тоже будет полезно поправить.

Теперь поговорим об операторе new.

inline HRESULT CreatePhraseFromWordArray(....)
{
  ....
  SPPHRASEELEMENT *pPhraseElement = new SPPHRASEELEMENT[cWords];
  if(pPhraseElement == NULL)
  {
    ::CoTaskMemFree(pStringPtrArray);
    return E_OUTOFMEMORY;
  }
  memset(pPhraseElement, 0, sizeof(SPPHRASEELEMENT) * cWords);
  ....
}

Предупреждение PVS-Studio: V668 There is no sense in testing the 'pPhraseElement' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sphelper.h 2973

Формально, этот код неправильный. В случае ошибки выделения памяти, оператор new должен сгенерировать исключение. В результате, мы не попадём внутрь тела оператора if и не будет вызвана функция CoTaskMemFree. И, вообще, программа начнет работать не так, как ожидал программист.

Я не уверен, что это настоящая ошибка. Есть вероятность, что проект слинкован с nothrownew.obj. В этом случае, оператор new не будет генерировать исключение. Такой возможностью, например, пользуются разработчики драйверов. Подробнее: new and delete operators. Так что, если это ложные срабатывания, то можно просто выключить предупреждение V668.

Но возможен другой сценарий. Этот код остался с древних времён, когда оператор new возвращал в случае ошибки значение NULL. Тогда всё плохо, так как анализатор выдал 112 таких предупреждений: vs2017_V668.txt.

Продолжим. Анализатор выдаёт много предупреждений V730, которое говорит, что в конструкторе инициализированы не все члены. Поясню суть предупреждения на двух примерах.

В начале рассмотрим класс CMFCScanliner. В нем объявлены следующие члены:

class CMFCScanliner
{
  ....
  private:
  LPBYTE  m_line;
  LPBYTE  m_line_begin;
  LPBYTE  m_line_end;
  size_t  m_pitch;
  DWORD   m_start_row;
  DWORD   m_start_col;
  DWORD   m_rows;
  DWORD   m_cols;
  long    m_offset;
  BYTE    m_channels;
  size_t  m_height;
};

Теперь посмотрим на конструктор:

CMFCScanliner()
{
  empty();
}

Собственно, смотреть в конструкторе нечего. Нам следует перейти к изучению функции empty:

void empty()
{
  m_line      = NULL;
  m_pitch     = 0;
  m_start_row = 0;
  m_start_col = 0;
  m_rows      = 0;
  m_cols      = 0;
  m_offset    = 0;
  m_height    = 0;
  m_line_begin = NULL;
  m_line_end   = NULL;
}

Предупреждение PVS-Studio: V730 It is possible that not all members of a class are initialized inside the constructor. Consider inspecting: m_channels. afxtoolbarimages.cpp 510

Инициализированы все члены, кроме m_channels. Согласитесь, это странно, так как этот член не выделяется ничем среди остальных. Это очень похоже на ошибку, хотя до конца я не могу быть уверенным, так как не знаком с принципами работы рассматриваемого класса.

Теперь рассмотрим структуру AFX_EVENT.

struct AFX_EVENT
{
  enum 
  {
    event, propRequest, propChanged, propDSCNotify
  };

  AFX_EVENT(int eventKind);

  AFX_EVENT(int eventKind, DISPID dispid, ....);

  int m_eventKind;
  DISPID m_dispid;
  DISPPARAMS* m_pDispParams;
  EXCEPINFO* m_pExcepInfo;
  UINT* m_puArgError;
  BOOL m_bPropChanged;
  HRESULT m_hResult;
  DSCSTATE m_nDSCState;
  DSCREASON m_nDSCReason;
};

AFX_INLINE AFX_EVENT::AFX_EVENT(int eventKind)
{
  m_eventKind = eventKind;
  m_dispid = DISPID_UNKNOWN;
  m_pDispParams = NULL;
  m_pExcepInfo = NULL;
  m_puArgError = NULL;
  m_hResult = NOERROR;
  m_nDSCState = dscNoState;
  m_nDSCReason = dscNoReason;
}

Предупреждение PVS-Studio: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_bPropChanged. afxpriv2.h 104

В этот раз неинициализированной осталась переменная m_bPropChanged.

В обоих случаях я не берусь судить нужно ли инициализировать эти переменные. Поэтому я предлагаю разработчикам изучить эти и другие подозрительные случаи, на которые обращает внимание анализатор PVS-Studio. Я выписал в файл vs2017_V730.txt ещё 183 предупреждения анализатора. Наверняка среди этих сообщений найдется несколько, которые указывают на настоящие серьезные ошибки. Если бы я был уверен, что члены точно надо инициализировать, я бы отнес все эти предупреждения к следующей главе этой статьи. Неинициализированные переменные крайне коварны, так как ошибки могут проявляться редко и нерегулярно.

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

HRESULT
SetDpiCompensatedEffectInput(....)
{
  ....
  hr = deviceContext->CreateEffect(CLSID_D2D1DpiCompensation,
                                   &dpiCompensationEffect);
  if (SUCCEEDED(hr))
  {
    if (SUCCEEDED(hr))
    {
  ....
}

Предупреждение PVS-Studio: V571 Recurring check. The 'if (((HRESULT)(hr)) >= 0)' condition was already verified in line 881. d2d1_1helper.h 883

Два раза подряд проверяется значение переменной hr. Мы имеем дело или с лишним кодом, или это какая-то опечатка и нужно изменить второе условие.

void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength)
{
  // See comment in SetString() about why we do this
  UINT_PTR nOffset = pszSrc-GetString();

  UINT nOldLength = GetLength();
  if (nOldLength < 0)
  {
    // protects from underflow
    nOldLength = 0;
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. atlsimpstr.h 392

Переменная nOldLength имеет беззнаковый тип UINT, а значит не может быть меньше нуля.

Теперь поговорим о функции FreeLibrary.

extern "C"
BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID)
{
  ....
  ::FreeLibrary(pState->m_appLangDLL);
  ....
}

Предупреждение PVS-Studio: V718 The 'FreeLibrary' function should not be called from 'DllMain' function. dllinit.cpp 639

Вот что сказано в MSDN по поводу функции FreeLibrary: It is not safe to call FreeLibrary from DllMain. For more information, see the Remarks section in DllMain.

Благодаря везению, код может работать. Однако, все равно код плох и следует обратить на него внимание.

В заключении этой главы хочу обратить внимание вот на эту шаблонную функцию:

template<class _FwdIt>
  string_type transform_primary(_FwdIt _First, _FwdIt _Last) const
{  // apply locale-specific case-insensitive transformation
  string_type _Res;

  if (_First != _Last)
    {  // non-empty string, transform it
    vector<_Elem> _Temp(_First, _Last);

    _Getctype()->tolower(&*_Temp.begin(),
      &*_Temp.begin() + _Temp.size());
    _Res = _Getcoll()->transform(&*_Temp.begin(),
      &*_Temp.begin() + _Temp.size());
    }
  return (_Res);
}

Предупреждение PVS-Studio: V530 The return value of function 'tolower' is required to be utilized. regex 319

Я впервые вижу этот код и мне сложно сориентироваться. Поэтому, я не знаю прав ли анализатор, указывая на вызов функции tolower. Обычно результат функции tolower обязательно надо использовать, но я не знаю какая именно функция tolower здесь вызывается. Поэтому просто обращаю внимание разработчиков библиотеки на этот код и прошу его проверить.

Hardcore

Здесь начинается, на мой взгляд, самое интересное.

_AFXCMN_INLINE int CToolBarCtrl::GetString(
  _In_ int nString,
  _Out_writes_to_(cchMaxLen, return + 1) LPTSTR lpstrString,
  _In_ size_t cchMaxLen) const
{
  ASSERT(::IsWindow(m_hWnd));
  return (int) ::SendMessage(m_hWnd, ...., (LPARAM)lpstrString);
  lpstrString[cchMaxLen]=_T('');
}

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. afxcmn2.inl 111

Явная ошибка: последняя строчка функции никогда не выполняется.

Следующий фрагмент кода содержит крайне подозрительный вызов оператора return внутри тела цикла:

HRESULT GetIndexOfPropertyInSet(
  _In_ const GUID* pPropSet,
  _In_ DBPROPID dwPropertyId,
  _Out_ ULONG* piCurPropId,
  _Out_ ULONG* piCurSet)
{
  HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet);
  if (hr == S_FALSE)
    return hr;
  UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo;
  for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
  {
    if( dwPropertyId == pUPropInfo[ul].dwPropId )
      *piCurPropId = ul;
    return S_OK;
  }

  return S_FALSE;
}

Предупреждение PVS-Studio: V612 An unconditional 'return' within a loop. atldb.h 4837

Не понятно, зачем было делать цикл, если всё равно в этот цикл не сможет выполнить больше одной итерации. Код можно упросить. Однако, мне кажется, что здесь надо не упрощать код, а исправлять ошибку. У меня есть подозрение, что здесь забыты фигурные скобки и, на самом деле, функция должна выглядеть так:

HRESULT GetIndexOfPropertyInSet(
  _In_ const GUID* pPropSet,
  _In_ DBPROPID dwPropertyId,
  _Out_ ULONG* piCurPropId,
  _Out_ ULONG* piCurSet)
{
  HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet);
  if (hr == S_FALSE)
    return hr;
  UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo;
  for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
  {
    if( dwPropertyId == pUPropInfo[ul].dwPropId )
    {
      *piCurPropId = ul;
      return S_OK;
    }
  }
  return S_FALSE;
}

Помимо рассмотренного цикла, стоит обратить внимание на пару операторов break, которые всегда прерывают циклы:

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

Теперь поговорим про Copy-Paste. Нельзя написать большой проект и не наделать ошибок, связанных с копированием и правкой блоков текста.

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

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();
}

В первом цикле обрабатывается контейнер m_lstControlBars, а во втором контейнер m_lstSliders.

С вероятностью 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;
  }
  ....
};

Предупреждение PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. afxtoolbarimages.cpp 656

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

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

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;
  }
  ....
}

Предупреждение PVS-Studio: V773 The function was exited without releasing the 'pWndSpin' pointer. A memory leak is possible. afxpropertygridctrl.cpp 1490

Если функция 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

Согласно стандарту языка C++, вызов оператора delete для указателя типа void* приводит к неопределённому поведению. И, как уже догадался читатель, это происходит в библиотеках Visual C++:

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);
}

Предупреждение PVS-Studio: V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1039

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

• 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

Функция CMFCReBar::CalcFixedLayout принимает параметр bStretch, но не использует его. Вернее, перед первым использованием, в bStretch явно записывается 1. Чтобы продемонстрировать, что я ничего не напутал и не просмотрел, привожу эту функцию целиком.

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());
}

Предупреждение PVS-Studio: V763 Parameter 'bStretch' is always rewritten in function body before being used. afxrebar.cpp 209

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

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

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

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

Предупреждение PVS-Studio: V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'AdjustDockingLayout' in derived class 'CDockSite' and base class 'CBasePane'. afxdocksite.h 94

Мне кажется, что в какой-то момент в базовом классе в объявлении функции добавили аргумент 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

Раз уж мы заговорили о функциях в классах, давайте поговорим о виртуальных деструкторах, вернее, об их отсутствии. Для начала заглянем в класс CAccessToken:

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

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

Анализатор выдаёт предупреждение: V599 The virtual destructor is not present, although the 'CRevert' class contains virtual functions. atlsecurity.h 5252

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

m_pRevert = _ATL_NEW CRevertToSelf;

Класс CRevertToSelf наследуется от класса CRevert. Давайте теперь познакомимся с этими классами:

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

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

Чего не хватает в этом классе CRevert? В нём не хватает виртуального деструктора.

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

template <class TAccessor>
class CBulkRowset : public CRowset<TAccessor>
{
  ....
  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;
  ....
}

Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] m_phRow;'. atldbcli.h 5689

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

m_phRow = _ATL_NEW HROW[m_nRows];

Память освобождается в функции SetRows с помощью оператора delete:

delete m_phRow;

Результат: неопределённое поведение.

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

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

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

Перед нами типовая ситуация. Все члены структуры очищаются (заполняются нулями) с помощью вызова функции memset. Затем в структуру записывается её размер. Это стандартная практика для WinAPI. Так многие функции узнают с какой версией (форматом) структуры они работают.

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

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

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

Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& ti'. barcore.cpp 384

Структура имеет тип 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));
  ....
}

Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& m_Id'. traceloggingactivity.h 656

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

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

ZeroMemory(&m_Id, sizeof(m_Id));

Не обошлось и без предупреждений V595. В этом нет ничего удивительного, так как эта диагностика выявляет одну из самых распространённых ошибок в программах на языке C и C++. Впрочем, в C# всё тоже не идеально.

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

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

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;
  }
  ....
}

Предупреждение PVS-Studio: V595 The 'pszHelp' pointer was utilized before it was verified against nullptr. Check lines: 1324, 1328. afxbasepane.cpp 1324

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

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

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

Анализатор рассуждает следующим образом. Указатель разыменовывается. Однако, ниже есть проверка на равенство этого указателя 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);
}

Предупреждение PVS-Studio: V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonpanel.cpp 4107

Функция возвращает тип 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

Примечание

Как уже говорилось в начале статьи, я проверил не все файлы. Вдобавок, я мог пропустить что-то и среди выданных анализатором предупреждений. Поэтому прошу не рассматривать разработчиков этот документ, как руководство по исправлению некоторых ошибок. Намного лучше, если разработчики сами полноценно проверят библиотеки и внимательно изучат предупреждения анализатора.

Заключение

Я в очередной раз смог продемонстрировать читателю, какую пользу могут приносить инструменты статического анализа кода.

Хочу предостеречь от одной ошибки. Время от времени я слышу, что некоторые программисты запускают статические анализаторы при подготовке к релизу. Если кто-то так делает и утверждает, что это нормально, то он глубоко ошибается, и прошу вас наставить его на путь истинный. Это самый неправильный способ использования статических анализаторов. Это то же самое, что включать перед релизом предупреждения компилятора, а все остальное время работать над проектом, полностью их отключив.

Приглашаю всех установить демонстрационную версию анализатора PVS-Studio и попробовать проверить свои проекты.

Страница продукта PVS-Studio: https://www.viva64.com/ru/pvs-studio/

Поддерживаемые языки и компиляторы:

• Windows. Visual Studio 2017 C, C++, C++/CLI, C++/CX (WinRT), C#
• Windows. Visual Studio 2015 C, C++, C++/CLI, C++/CX (WinRT), C#
• Windows. Visual Studio 2013 C, C++, C++/CLI, C++/CX (WinRT), C#
• Windows. Visual Studio 2012 C, C++, C++/CLI, C++/CX (WinRT), C#
• Windows. Visual Studio 2010 C, C++, C++/CLI, C#
• Windows. MinGW C, C++
• Windows/Linux. Clang C, C++
• Linux. GCC C, C++

Спасибо за внимание и подписывайтесь на твиттер @Code_Analysis.