# Обнаружены ошибки в библиотеках Visual C\+\+ 2012 Одной из методик выявления ошибок в программах является статический анализ кода\. Мы рады, что это методология становится всё более популярной\. Во многом этому способствует 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 места, которые кажутся наиболее подозрительными\. ### Странный цикл Этот подозрительный код был найден первым\. Он и послужил толчком к продолжению исследования\. ```cpp template class ATL_NO_VTABLE CUtlProps : public CUtlPropsBase { .... HRESULT GetIndexOfPropertyInSet(....) { .... for(ULONG ul=0; ul::\_Isinf\(real\(\_Left\)\)' to the left and to the right of the '\|\|' operator\. xcomplex 780 В условии два раза повторяется выражение "\_CTR\(\_Ty\)::\_Isinf\(real\(\_Left\)\)"\. Затруднительно сказать, есть ли здесь ошибка и как следует исправить код\. Однако эта функция явно заслуживает внимания\. ### Ненужная проверка ```cpp template class CSimpleStringT { .... void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength) { .... UINT nOldLength = GetLength(); if (nOldLength < 0) { // protects from underflow nOldLength = 0; } .... }; ``` [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'nOldLength < 0' is always false\. Unsigned type value is never < 0\. atlsimpstr\.h 420 Ошибки здесь нет\. Если судить по коду, то длина строки отрицательной стать не может\. В классе CSimpleStringT имеются соответствующие проверки\. То, что переменная nOldLength имеет беззнаковый тип, ничего не портит\. Все равно длина всегда положительна\. Это просто лишний код\. ### Неправильное формирование строки ```cpp template 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](https://pvs-studio.ru/ru/docs/warnings/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\. ### Порт с отрицательным номером ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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\+\+ есть следующий макрос\. ```cpp #define DXVABitMask(__n) (~((~0) << __n)) ``` Везде где он используется, возникает неопределённо поведение\. Конечно, разработчикам Visual C\+\+ виднее, безопасна эта конструкция или нет\. Возможно, есть уверенность, что Visual C\+\+ будет всегда одинаково обрабатывать сдвиг отрицательных чисел\. Формально, сдвиг отрицательного числа приводит к Undefined behavior\. Подробнее эта тематика освещена в статье "[Не зная брода, не лезь в воду\. Часть третья](https://pvs-studio.ru/ru/blog/posts/cpp/0142/)"\. ### Некорректная работа в 64\-битном режиме Данный паттерн 64\-битной ошибки подробно рассмотрен в составленных нами уроках по разработке 64\-битных приложений на языке Си/Си\+\+\. Чтобы понять, в чем состоит ошибка, предлагаем ознакомиться с [уроком под номером 12](https://pvs-studio.ru/ru/blog/lessons/0012/)\. ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 Таких мест было найдено достаточно много\. Анализировать, опасен каждый конкретный случай или нет, весьма утомительно\. Намного лучше с этим справятся создатели библиотек\. Мы приведём только парочку примеров\. ```cpp BOOL CDockablePane::PreTranslateMessage(MSG* pMsg) { .... CBaseTabbedPane* pParentBar = GetParentTabbedPane(); CPaneFrameWnd* pParentMiniFrame = pParentBar->GetParentMiniFrame(); if (pParentBar != NULL && (pParentBar->IsTracked() || pParentMiniFrame != NULL && pParentMiniFrame->IsCaptured() ) ) .... } ``` [V595](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'pParentBar' pointer was utilized before it was verified against nullptr\. Check lines: 2840, 2841\. afxdockablepane\.cpp 2840 Смотрите, в начале указатель 'pParentBar' используется для вызова функции GetParentMiniFrame\(\)\. Затем вдруг появляется подозрение, что этот указатель может быть равен NULL и выполняется соответствующая проверка\. ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'pDockingBar' pointer was utilized before it was verified against nullptr\. Check lines: 582, 587\. afxdockingmanager\.cpp 582 В начале указатель 'pDockingBar' активно используется, а потом вдруг сравнивается с NULL\. И ещё один пример напоследок: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v595/) The 'pButtonCurr' pointer was utilized before it was verified against nullptr\. Check lines: 1412, 1414\. afxframeimpl\.cpp 1412 Смело обращаемся к члену класса 'm\_nID'\. А затем из условия видно, что указатель 'pButtonCurr' может быть равен 0\. ### Использование разрушенного объекта ```cpp CString m_strBrowseFolderTitle; void CMFCEditBrowseCtrl::OnBrowse() { .... LPCTSTR lpszTitle = m_strBrowseFolderTitle != _T("") ? m_strBrowseFolderTitle : (LPCTSTR)NULL; .... } ``` [V623](https://pvs-studio.ru/ru/docs/warnings/v623/) Consider inspecting the '?:' operator\. A temporary object is being created and subsequently destroyed\. afxeditbrowsectrl\.cpp 308 Тернарный оператор не может возвращать значения разных типов\. Поэтому, из "\(LPCTSTR\)NULL" будет неявно создан объект типа CString\. Затем из этой пустой строки будет неявно взят указатель на её буфер\. Беда в том, что временный объект типа CString будет разрушен\. В результате значения указателя 'lpszTitle' становится не валидным и работать с ним нельзя\. [Здесь](https://pvs-studio.ru/ru/docs/warnings/v623/) можно почитать более подробное описание данного паттерна ошибки\. ### Неправильная работа с временем ```cpp UINT CMFCPopupMenuBar::m_uiPopupTimerDelay = (UINT) -1; .... void CMFCPopupMenuBar::OnChangeHot(int iHot) { .... SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE, max(0, m_uiPopupTimerDelay - 1), NULL); .... } ``` [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression '\(0\) \> \(m\_uiPopupTimerDelay \- 1\)' is always false\. Unsigned type value is never < 0\. afxpopupmenubar\.cpp 968 Значение '\-1' используется как специальный флаг\. Используя макрос 'max' программисты хотели защититься от отрицательных значений в переменной m\_uiPopupTimerDelay\. Это не получится, так как переменная имеет беззнаковый тип\. Она всегда больше или равна нулю\. Корректный код должен выглядеть приблизительно так: ```cpp 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 ### Бессмысленная строка ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'data\.m\_ptAccHit'\. afxtaskspane\.cpp 96 Что такое "data\.m\_ptAccHit;" ? Быть может здесь забыли присвоить этой переменной какое\-то значение? ### Возможно нужен ещё один 0? ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v512/) A call of the 'memset' function will lead to underflow of the buffer 'lpcszBuffer'\. afxtaskspane\.cpp 2626 Есть подозрение, что этот код может вернуть строку, которая не будет завершаться терминальным нулём\. Скорее всего, следовало обнулить и последний элемент массива: ```cpp memset(lpcszBuffer, 0, (MAX_NAME_LEN + 1) * sizeof(TCHAR)); ``` ### Странный 'if' ```cpp void CMFCVisualManagerOfficeXP::OnDrawBarGripper(....) { .... if (bHorz) { rectFill.DeflateRect(4, 0); } else { rectFill.DeflateRect(4, 0); } .... } ``` [V523](https://pvs-studio.ru/ru/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. afxvisualmanagerofficexp\.cpp 264 ### Опасный класс single\_link\_registry Если вы используете класс 'single\_link\_registry' то ваше приложение может неожиданно завершиться, даже если вы корректно обрабатываете все исключения\. Посмотрим на деструктор класса 'single\_link\_registry': ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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 ### Ещё один странный цикл ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v612/) An unconditional 'break' within a loop\. viewprev\.cpp 476 В цикле нет ни одного оператора 'continue'\. В конце цикла стоит 'break'\. Это очень странно\. Цикл всегда выполняется только один раз\. Здесь или ошибка или лучше заменить 'while' на 'if'\. ### Странная константа Есть и другие несущественные замечания к коду, которые перечислять не интересно\. Приведём только один пример, чтобы было понятно, о чём идёт речь\. В файле afxdrawmanager\.cpp зачем то заведена константа для числа Пи: ```cpp const double AFX_PI = 3.1415926; ``` [V624](https://pvs-studio.ru/ru/docs/warnings/v624/) The constant 3\.1415926 is being utilized\. The resulting value could be inaccurate\. Consider using the M\_PI constant from \. afxdrawmanager\.cpp 22 Это конечно не ошибка и точности константы достаточно\. Но не понятно, почему\-бы не использовать константу M\_PI, которая задана гораздо точнее: ```cpp #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 можно [здесь](https://pvs-studio.ru/ru/pvs-studio/)\.