# Сравнение статического анализа общего назначения из VS 2010 и PVS\-Studio на примере пяти открытых проектов В статье показаны ошибки, выявленные с помощью статического анализатора кода, встроенного в Visual Studio 2010\. Исследование проводилось на пяти open source проектах\. Эти же проекты были проверены с помощью PVS\-Studio\. Приведены результаты сравнения этих двух инструментов\. ## Введение В статье "Трудности сравнения анализаторов кода или не забывайте об удобстве использования" \[[1](https://pvs-studio.ru/ru/blog/posts/a0071/)\] говорится о том, что сравнить между собой два инструмента не так просто как кажется, потому что помимо собственно технических характеристик анализаторов очень большое значение имеет такой параметр как удобство использования\. Но все\-таки от сравнения по обнаруживаемым ошибкам никуда не деться\. Естественно, просто посчитать их количество – смысла нет\. Поэтому мы решили провести практический эксперимент по обнаружению ошибок в реальных проектах\. Мы проверили пять случайных open source проектов сначала с помощью статического анализатора, встроенного в Visual Studio 2010 Premium\. Просмотрели весь список сообщений и выбрали явные ошибки\. Затем также поступили с PVS\-Studio\. Вот список проектов, участвовавших в исследовании: * [eMule Plus](https://emuleplus.info/); * [Pixie](http://www.renderpixie.com/); * [VirtualDub](http://www.virtualdub.org/); * [WinMerge](https://winmerge.org/); * [XUIFramework](https://www.codeproject.com/articles/11979/xuiframework-a-gui-framework-based-on-xml-and-mfc)\. Итак, поехали\! ## eMule Plus Всего сообщений от статического анализатора Visual Studio – 237\. Из них реальных ошибок – 4\. Всего сообщений от PVS\-Studio – 68\. Из них реальных ошибок – 3\. Visual Studio: warning C6054: String 'szwThemeFile' might not be zero\-terminated\. c:\\emuleplus\\dialogmintraybtn\.hpp 445 ```cpp WCHAR szwThemeFile[MAX_PATH]; WCHAR szwThemeColor[256]; if (m_themeHelper.GetCurrentThemeName(szwThemeFile, ARRSIZE(szwThemeFile), szwThemeColor, ARRSIZE(szwThemeColor), NULL, 0) != S_OK) return NULL; WCHAR *p; if ((p = wcsrchr(szwThemeFile, L'\\')) == NULL) ``` Действительно строка может не заканчиваться на 0, что приведет к потенциальным проблемам\. Однако в данном случае эта ошибка не проявит себя, скорее всего\. Visual Studio: warning C6269: Possibly incorrect order of operations: dereference ignored\. c:\\emuleplus\\customautocomplete\.cpp 277 PVS\-Studio: V532 Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. customautocomplete\.cpp 277 ```cpp if (pceltFetched != NULL) *pceltFetched++; ``` Здесь программист "не умеет" пользоваться выражением \(\*ptr\)\+\+\. Хотя, казалось бы, такая конструкция выглядит достаточно безобидно, тем не менее, подобная ошибка очень распространена\. Visual Studio: warning C6298: Argument '6': using a read\-only string as a writable string argument\. This will attempt to write into static read\-only memory and cause random crashes\. c:\\emuleplus\\firewallopener\.cpp 183 ```cpp HRESULT hr = pNSC->AddPortMapping( riPortRule.m_strRuleName.AllocSysString(), riPortRule.m_byProtocol, riPortRule.m_nPortNumber, riPortRule.m_nPortNumber, 0, L"127.0.0.1", ICSTT_IPADDRESS, &pNSPM); ``` Хотя это и не ошибка, но сообщение анализатора справедливое\. Вообще это проблема всех инструментов статического анализа\. Они выдают совершенно корректные сообщения, но далеко не всегда это действительно ошибки\. Означает ли это, что инструменты и подобные сообщения бесполезны? Нет, не означает, поскольку исправление даже подобных замечаний в целом повышает качество кода\. Visual Studio: warning C6314: Incorrect order of operations: bitwise\-or has higher precedence than the conditional\-expression operator\. Add parentheses to clarify intent\. c:\\emuleplus\\searchlistctrl\.cpp 659 PVS\-Studio: V502 Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '\|' operator\. searchlistctrl\.cpp 659 ```cpp menuSearchFile.AppendMenu( MF_STRING | ((iSelectionMark != -1) && (dwSelectedCount > 0) && g_App.m_pServerConnect->IsConnected() && ((pCurServer = g_App.m_pServerConnect->GetCurrentServer())!= NULL)&& (pCurServer->GetTCPFlags() & SRV_TCPFLG_RELATEDSEARCH)) ? MF_ENABLED : MF_GRAYED, MP_SEARCHRELATED, GetResString(IDS_SEARCHRELATED)); ``` Здесь \(из\-за сложности конструкции\) получился неправильный приоритет операторов\. Уж сколько раз твердили миру\.\.\. Ну вот кто мешал писать это все не в одну строку \(как в оригинальной программе\), а разбить на несколько отдельных выражений? Нет же, программисты часто хотят "записать красиво"\. PVS\-Studio: V519 The 'm\_clrSample' object is assigned values twice successively\. Perhaps this is a mistake\. fontpreviewcombo\.cpp 61 ```cpp CFontPreviewCombo::CFontPreviewCombo() { ... m_clrSample = GetSysColor(COLOR_WINDOWTEXT); m_clrSample = RGB(60,0,0); ... } ``` Возможно, попробовали как будет смотреться цвет RGB\(60,0,0\) и забыли убрать\. ## Pixie Всего сообщений от статического анализатора Visual Studio – 18\. Из них реальных ошибок – 0\. Всего сообщений от PVS\-Studio – 65\. Из них реальных ошибок – 5\. PVS\-Studio: V519 The 'numRays' object is assigned values twice successively\. Perhaps this is a mistake\. bundles\.cpp 579 ```cpp void CGatherBundle::post() { numRays = last; numRays = 0; last = 0; depth++; } ``` Ой, как подозрительно, что сначала numRays инициализируется одним значением, а затем другим\. Ошибка? Или нет? Знает только автор кода\. Но настораживает\! PVS\-Studio: V501 There are identical sub\-expressions to the left and to the right of the '\|' operator: PARAMETER\_DPDU \| PARAMETER\_DPDU quadrics\.cpp 880 ```cpp if (up & (PARAMETER_DPDU | PARAMETER_DPDU)) { ``` Явно здесь должно быть что\-то еще\. Кстати общее замечание по исправлению ошибок, обнаруженных статическим анализатором\. Если в каких\-то случаях исправление очевидно и его может сделать любой, то в каких\-то лишь автор кода может разобраться с тем, что же он хотел написать\. Это к вопросу о том, а нельзя ли сделать инструмент, который предлагает исправление ошибок "как в Ворде"\. PVS\-Studio: V501 There are identical sub\-expressions to the left and to the right of the '\|' operator: SLC\_VECTOR \| SLC\_VECTOR expression\.cpp 2604 ```cpp lock(N, getConversion(SLC_VECTOR | SLC_VECTOR,parameters[2])); ``` Дважды упомянутый SLC\_VECTOR в подобном контексте – конечно же, признак ошибки\. PVS\-Studio: V505 The 'alloca' function is used inside the loop\. This can quickly overflow stack\. polygons\.cpp 1120 ```cpp inline void triangulatePolygon(...) { ... for (i=1;im[9], gt0=cfd->m[9], bt0=cfd->m[9]; ... } ``` Банальный выход за границы массива\. ## WinMerge Всего сообщений от статического анализатора Visual Studio – 343\. Из них реальных ошибок – 3\. Всего сообщений от PVS\-Studio – 69\. Из них реальных ошибок – 12\. Visual Studio: warning C6313: Incorrect operator: zero\-valued flag cannot be tested with bitwise\-and\. Use an equality test to check for zero\-valued flags\. c:\\winmerge\\src\\bcmenu\.cpp 1489 ```cpp else if (nFlags&MF_STRING){ ASSERT(!(nFlags&MF_OWNERDRAW)); ModifyMenu(pos,nFlags,nID,mdata->GetString()); } ``` Неудачно записано условие\. Хотели, конечно, записать другое, но уж "что получилось"\. Visual Studio: warning C6287: Redundant code: the left and right sub\-expressions are identical\. c:\\winmerge\\src\\editlib\\ccrystaleditview\.cpp 1560 PVS\-Studio: V501 There are identical sub\-expressions to the left and to the right of the '\|\|' operator: c \=\= '\}' \|\| c \=\= '\}' ccrystaleditview\.cpp 1560 ```cpp bool isclosebrace (TCHAR c) { return c == _T ('}') || c == _T ('}') || c == _T (']') || c == _T ('>'); } ``` Не все типы скобочек проверяются\. Почему? "Copy\-paste\-technology", как это нередко бывает, приводит к ошибкам\. Visual Studio: warning C6287: Redundant code: the left and right sub\-expressions are identical\. c:\\winmerge\\src\\mergedoc\.cpp 1165 PVS\-Studio: V501 There are identical sub\-expressions to the left and to the right of the '\|\|' operator\. mergedoc\.cpp 1165 ```cpp if ((m_nBufferType[nBuffer] == BUFFER_UNNAMED) || (m_nBufferType[nBuffer] == BUFFER_UNNAMED)) nSaveErrorCode = SAVE_NO_FILENAME; ``` Опять неудачное условие\. И опять, похоже, из\-за копи\-паста\. PVS\-Studio: V551 The code under this 'case' label is unreachable\. The value range of signed char type: \[\-128, 127\]\. ccrystaltextview\.cpp 1646 ```cpp TCHAR ch = strText[i]; switch (ch) { case 0xB7: case 0xBB: strHTML += ch; strHTML += _T(""); bLastCharSpace = FALSE; nNonbreakChars = 0; break; ``` А вот здесь пример кода, который никогда не будет использован\. Вроде бы и case написан, да никогда он не получит управление\. Потому что диапазон значений слишком узкий\. TCHAR в данном случае представляет собой тип char\. PVS\-Studio: V524 It is odd that the body of 'IsValidTextPosX' function is fully equivalent to the body of 'IsValidTextPos' function \(ccrystaltextview\.cpp, line 3700\)\. ccrystaltextview\.cpp 3707 ```cpp bool CCrystalTextView::IsValidTextPos (const CPoint &point) { return GetLineCount () > 0 && m_nTopLine >= 0 && m_nOffsetChar >= 0 && point.y >= 0 && point.y < GetLineCount () && point.x >= 0 && point.x <= GetLineLength (point.y); } bool CCrystalTextView::IsValidTextPosX (const CPoint &point) { return GetLineCount () > 0 && m_nTopLine >= 0 && m_nOffsetChar >= 0 && point.y >= 0 && point.y < GetLineCount () && point.x >= 0 && point.x <= GetLineLength (point.y); } bool CCrystalTextView::IsValidTextPosY (const CPoint &point) { return GetLineCount () > 0 && m_nTopLine >= 0 && m_nOffsetChar >= 0 && point.y >= 0 && point.y < GetLineCount (); } ``` Крайне похожие функции\.\.\. Снова и снова копи\-пастили и забыли поправить результат\. Функция IsValidTextPosX\(\) делает лишнюю проверку\. PVS\-Studio: V563 It is possible that this 'else' branch must apply to the previous 'if' statement\. bcmenu\.cpp 1852 ```cpp if(IsLunaMenuStyle()) if(!xp_space_accelerators)return; else if(!original_space_accelerators)return; ``` Кто, глядя на код, точно скажет, к какому if относится else? А это ли хотел сделать программист? А вы уверены? PVS\-Studio: V556 The values of different enum types are compared: switch\(ENUM\_TYPE\_A\) \{ case ENUM\_TYPE\_B: \.\.\. \}\. diffwrapper\.cpp 956 ```cpp enum output_style {} ... enum DiffOutputType m_outputStyle; switch (m_options.m_outputStyle) { case OUTPUT_CONTEXT: ``` Немножечко попутали тип enum в switch\. Но это ведь не страшно? Или страшно? PVS\-Studio: V530 The return value of function 'empty' is required to be utilized\. diractions\.cpp 1307 ```cpp void CDirView::GetItemFileNames(int sel, String& strLeft, String& strRight) const { UINT_PTR diffpos = GetItemKey(sel); if (diffpos == (UINT_PTR)SPECIAL_ITEM_POS) { strLeft.empty(); strRight.empty(); ``` Тот случай, когда _empty\(\)_ не делает empty\. Пример крайне неудачного названия метода\. PVS\-Studio: V524 It is odd that the body of 'OnUpdateLastdiff' function is fully equivalent to the body of 'OnUpdateFirstdiff' function \(DirView\.cpp, line 2189\)\. dirview\.cpp 2220 ```cpp void CDirView::OnUpdateLastdiff(CCmdUI* pCmdUI) { int firstDiff = GetFirstDifferentItem(); if (firstDiff > -1) pCmdUI->Enable(TRUE); else pCmdUI->Enable(FALSE); } void CDirView::OnUpdateFirstdiff(CCmdUI* pCmdUI) { int firstDiff = GetFirstDifferentItem(); if (firstDiff > -1) pCmdUI->Enable(TRUE); else pCmdUI->Enable(FALSE); } ``` Еще две крайне похожие функции\.\.\. PVS\-Studio: V501 There are identical sub\-expressions 'pView1\-\>GetTextBufferEol \(line\)' to the left and to the right of the '\!\=' operator\. mergedoclinediffs\.cpp 216 ```cpp if (pView1->GetTextBufferEol(line) != pView1->GetTextBufferEol(line)) { ``` Или то, или то\.\.\. Или не то? Наверное, здесь должно быть pView2\. PVS\-Studio: V530 The return value of function 'empty' is required to be utilized\. varprop\.cpp 133 ```cpp void VariantValue::Clear() { m_vtype = VT_NULL; m_bvalue = false; m_ivalue = 0; m_fvalue = 0; m_svalue.empty(); m_tvalue = 0; } ``` И опять _empty\(\)_ вовсе не очищает строку\. PVS\-Studio: V510 The 'Format' function is not expected to receive class\-type variable as 'N' actual argument"\. PropShel 105 ```cpp String GetSysError(int nerr); ... CString msg; msg.Format( _T("Failed to open registry key HKCU/%s:\n\t%d : %s"), f_RegDir, retVal, GetSysError(retVal) ); ``` При возникновении различных аварийных ситуаций WinMerge попробует сообщить об ошибках, но в некоторых случаях у него это плохо получится\. На первый взгляд все хорошо\. Вот только тип "String" есть не что иное как "std::wstring"\. А, следовательно, в лучшем случае мы распечатаем абракадабру, а в худшем произойдет ошибка доступа к памяти \(Access Violation\)\. Корректный код должен содержать вызов c\_str\(\)\. PVS\-Studio: V534 It is likely that a wrong variable is being compared inside the 'for' operator\. Consider reviewing 'i'\." BinTrans\.cpp 357 ```cpp // Get length of translated array of bytes from text. int Text2BinTranslator::iLengthOfTransToBin( char* src, int srclen ) { ... for (k=i; i') break; } ... } ``` Анализатор нашел подозрительный цикл\. Этот код предрасположен к Access Violation\. Цикл должен продолжаться пока не найдется символ '\>' или не закончится строка длиной в 'srclen' символов\. Вот только случайно для сравнения использована переменная 'i', а не 'k'\. Если символ '\>' найден не будет, то все вероятно закончится печально\. ## XUIFramework Всего сообщений от статического анализатора Visual Studio – 93\. Из них реальных ошибок – 2\. Всего сообщений от PVS\-Studio – 30\. Из них реальных ошибок – 5\. Visual Studio: warning C6269: Possibly incorrect order of operations: dereference ignored c:\\xui\-gui framework\\widgets\\cstatichtml\\ppdrawmanager\.cpp 298 PVS\-Studio: V532 Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. ppdrawmanager\.cpp 298 ```cpp for (DWORD pixel = 0; pixel < dwWidth * dwHeight; pixel++, *pBits++) ``` Опять кто\-то не умеет пользоваться \*ptr\+\+\. Как говорилось выше, ошибка распространенная\. Visual Studio: warning C6283: 'pBuffer' is allocated with array new\[\], but deleted with scalar delete\. c:\\xui\-gui framework\\widgets\\cxstatic\\cxstatic\.cpp 544 ```cpp BYTE* pBuffer = new BYTE [ nBufferLen ]; ... delete pBuffer; ``` Путают люди delete и delete\[\]\. Это приводит к проблемам\. Иногда проблемы проявляются, иногда нет\. Но так делать не надо\. PVS\-Studio: V519 The 'm\_xSt' object is assigned values twice successively\. Perhaps this is a mistake\. resizedlg\.cpp 244 ```cpp m_xSt = CST_RESIZE; m_xSt = CST_RESIZE; ``` Судя по коду, там должно быть m\_ySt\. Но от копи\-паста ведь снова и снова не удержаться, да? V531 It is odd that a sizeof\(\) operator is multiplied by sizeof\(\)\. pphtmldrawer\.cpp 258 ```cpp DWORD dwLen = ::LoadString(hInstDll, dwID, szTemp, (sizeof(szTemp) * sizeof(TCHAR))); ``` Должно быть sizeof\(szTemp\) / sizeof\(TCHAR\) \. PVS\-Studio: V556 The values of different enum types are compared: enuHAlign \=\= Center\. cxstatic\.cpp 151 ```cpp if (enuHAlign == Center) ``` Должно быть enuHAlign \=\= Midle\. И в коде рядом есть еще if \(enuVAlign \=\= Middle\), хотя должно быть Center\. Попутали enum, короче\. PVS\-Studio: V501 There are identical sub\-expressions to the left and to the right of the '\|\|' operator\. resizedlg\.cpp 157f ```cpp HDWP CItemCtrl::OnSize(...) { ... if (m_styTop == CST_ZOOM || m_styTop == CST_ZOOM || m_styBottom == CST_DELTA_ZOOM || m_styBottom == CST_DELTA_ZOOM) ... } ``` Вероятно, код должен выглядеть так: ```cpp HDWP CItemCtrl::OnSize(...) { ... if (m_styTop == CST_ZOOM || m_styTop == CST_DELTA_ZOOM || m_styBottom == CST_ZOOM || m_styBottom == CST_DELTA_ZOOM) ... } ``` ## Результаты сравнения ![a0073_VS_vs_PVS-Studio_ru/image1.png](https://import.viva64.com/docx/blog/a0073_VS_vs_PVS-Studio_ru/image1.png) Мы не делаем никаких конкретных выводов\. В каких\-то проектах лучше себя показал один инструмент, в каких\-то – другой\. ## Библиографический список 1. Андрей Карпов, Евгений Рыжков\. [Трудности сравнения анализаторов кода или не забывайте об удобстве использования](https://pvs-studio.ru/ru/blog/posts/a0071/)\.