В статье показаны ошибки, выявленные с помощью статического анализатора кода, встроенного в Visual Studio 2010. Исследование проводилось на пяти open source проектах. Эти же проекты были проверены с помощью PVS-Studio. Приведены результаты сравнения этих двух инструментов.
В статье "Трудности сравнения анализаторов кода или не забывайте об удобстве использования" [1] говорится о том, что сравнить между собой два инструмента не так просто как кажется, потому что помимо собственно технических характеристик анализаторов очень большое значение имеет такой параметр как удобство использования.
Но все-таки от сравнения по обнаруживаемым ошибкам никуда не деться. Естественно, просто посчитать их количество – смысла нет. Поэтому мы решили провести практический эксперимент по обнаружению ошибок в реальных проектах.
Мы проверили пять случайных open source проектов сначала с помощью статического анализатора, встроенного в Visual Studio 2010 Premium. Просмотрели весь список сообщений и выбрали явные ошибки. Затем также поступили с PVS-Studio.
Вот список проектов, участвовавших в исследовании:
Итак, поехали!
Всего сообщений от статического анализатора Visual Studio – 237. Из них реальных ошибок – 4.
Всего сообщений от PVS-Studio – 68. Из них реальных ошибок – 3.
Visual Studio: warning C6054: String 'szwThemeFile' might not be zero-terminated. c:\emuleplus\dialogmintraybtn.hpp 445
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
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
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
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
CFontPreviewCombo::CFontPreviewCombo()
{
...
m_clrSample = GetSysColor(COLOR_WINDOWTEXT);
m_clrSample = RGB(60,0,0);
...
}Возможно, попробовали как будет смотреться цвет RGB(60,0,0) и забыли убрать.
Всего сообщений от статического анализатора 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
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
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
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
inline void triangulatePolygon(...) {
...
for (i=1;i<nloops;i++) {
...
do {
...
do {
...
CTriVertex *snVertex = (CTriVertex *)
alloca(2*sizeof(CTriVertex));
...
} while(dVertex != loops[0]);
...
} while(sVertex != loops[i]);
...
}
...
}Из-за большой вложенности вызов alloca может привести к переполнению стека.
Всего сообщений от статического анализатора Visual Studio – 24. Из них реальных ошибок – 0.
Всего сообщений от PVS-Studio – 41. Из них реальных ошибок – 2.
PVS-Studio: V547 Expression 'c < 0' is always false. Unsigned type value is never < 0. lexer.cpp 279
typedef unsigned short wint_t;
wint_t lexgetescape() {
wint_t c = lexgetc();
if (c < 0)
fatal("Newline found in escape sequence");
...
}Код никогда не будет вызван, поскольку выражение всегда false.
PVS-Studio: V557 Array overrun is possible. The '9' index is pointing beyond array bound. f_convolute.cpp 73
struct ConvoluteFilterData {
long m[9];
long bias;
void *dyna_func;
DWORD dyna_size;
DWORD dyna_old_protect;
BOOL fClip;
};
static unsigned long __fastcall do_conv(
unsigned long *data,
const ConvoluteFilterData *cfd,
long sflags, long pit)
{
long rt0=cfd->m[9], gt0=cfd->m[9], bt0=cfd->m[9];
...
}Банальный выход за границы массива.
Всего сообщений от статического анализатора 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
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
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
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
TCHAR ch = strText[i];
switch (ch)
{
case 0xB7:
case 0xBB:
strHTML += ch;
strHTML += _T("<wbr>");
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
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
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
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
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
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
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
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
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
// Get length of translated array of bytes from text.
int Text2BinTranslator::iLengthOfTransToBin(
char* src, int srclen )
{
...
for (k=i; i<srclen; k++)
{
if (src[k]=='>')
break;
}
...
}Анализатор нашел подозрительный цикл. Этот код предрасположен к Access Violation. Цикл должен продолжаться пока не найдется символ '>' или не закончится строка длиной в 'srclen' символов. Вот только случайно для сравнения использована переменная 'i', а не 'k'. Если символ '>' найден не будет, то все вероятно закончится печально.
Всего сообщений от статического анализатора 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
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
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
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
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
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
HDWP CItemCtrl::OnSize(...)
{
...
if (m_styTop == CST_ZOOM ||
m_styTop == CST_ZOOM ||
m_styBottom == CST_DELTA_ZOOM ||
m_styBottom == CST_DELTA_ZOOM)
...
}Вероятно, код должен выглядеть так:
HDWP CItemCtrl::OnSize(...)
{
...
if (m_styTop == CST_ZOOM ||
m_styTop == CST_DELTA_ZOOM ||
m_styBottom == CST_ZOOM ||
m_styBottom == CST_DELTA_ZOOM)
...
}Мы не делаем никаких конкретных выводов. В каких-то проектах лучше себя показал один инструмент, в каких-то – другой.