Одной из методик выявления ошибок в программах является статический анализ кода. Мы рады, что это методология становится всё более популярной. Во многом этому способствует 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. А именно такие папки как:
Полноценной проверки не получилось, так как у нас нет проектов или make-файлов для сборки библиотек. Так что, удалось проверить только скромную часть кода библиотек. Не смотря на незавершенность проверки, результаты весьма интересны.
Давайте познакомимся, что нашел анализатор PVS-Studio внутри библиотек для Visual C++. Как видно, все эти ошибки просочились мимо анализатора, встроенного в сам Visual C++.
Не будем утверждать, что все приведённые ниже фрагменты кода содержат ошибки. Мы просто выбрали из списка предложенного анализатором PVS-Studio места, которые кажутся наиболее подозрительными.
Этот подозрительный код был найден первым. Он и послужил толчком к продолжению исследования.
template <class T>
class ATL_NO_VTABLE CUtlProps :
public CUtlPropsBase
{
....
HRESULT GetIndexOfPropertyInSet(....)
{
....
for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
{
if( dwPropertyId == pUPropInfo[ul].dwPropId )
*piCurPropId = ul;
return S_OK;
}
return S_FALSE;
}
....
};V612 An unconditional 'return' within a loop. atldb.h 4829
Тело цикла выполняется только один раз. Ошибка в пояснениях не нуждается. Скорее всего, оператор 'return' должен быть вызван, когда найдено искомое значение. В этом случае код должен выглядеть так:
for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
{
if( dwPropertyId == pUPropInfo[ul].dwPropId )
{
*piCurPropId = ul;
return S_OK;
}
}Прошу прощения за тяжёлый для прочтения пример. Обратите внимание на условие в тернарном операторе.
// TEMPLATE FUNCTION proj
_TMPLT(_Ty) inline
_CMPLX(_Ty) proj(const _CMPLX(_Ty)& _Left)
{ // return complex projection
return (_CMPLX(_Ty)(
_CTR(_Ty)::_Isinf(real(_Left)) ||
_CTR(_Ty)::_Isinf(real(_Left))
? _CTR(_Ty)::_Infv(real(_Left)) : real(_Left),
imag(_Left) < 0 ? -(_Ty)0 : (_Ty)0));
}V501 There are identical sub-expressions '_Ctraits < _Ty >::_Isinf(real(_Left))' to the left and to the right of the '||' operator. xcomplex 780
В условии два раза повторяется выражение "_CTR(_Ty)::_Isinf(real(_Left))". Затруднительно сказать, есть ли здесь ошибка и как следует исправить код. Однако эта функция явно заслуживает внимания.
template<typename BaseType, bool t_bMFCDLL = false>
class CSimpleStringT
{
....
void Append(_In_reads_(nLength) PCXSTR pszSrc,
_In_ int nLength)
{
....
UINT nOldLength = GetLength();
if (nOldLength < 0)
{
// protects from underflow
nOldLength = 0;
}
....
};V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. atlsimpstr.h 420
Ошибки здесь нет. Если судить по коду, то длина строки отрицательной стать не может. В классе CSimpleStringT имеются соответствующие проверки. То, что переменная nOldLength имеет беззнаковый тип, ничего не портит. Все равно длина всегда положительна. Это просто лишний код.
template <class T>
class CHtmlEditCtrlBase
{
....
HRESULT SetDefaultComposeSettings(
LPCSTR szFontName=NULL, .....) const
{
CString strBuffer;
....
strBuffer.Format(_T("%d,%d,%d,%d,%s,%s,%s"),
bBold ? 1 : 0,
bItalic ? 1 : 0,
bUnderline ? 1 : 0,
nFontSize,
szFontColor,
szBgColor,
szFontName);
....
}
};V576 Incorrect format. Consider checking the eighth actual argument of the 'Format' function. The pointer to string of wchar_t type symbols is expected. afxhtml.h 826
Этот код формирует неправильное сообщение в UNICODE программах. Функция 'Format()' ожидает, что восьмой аргумент будет иметь тип LPCTSTR. Но переменная 'szFontName' всегда имеет тип LPCSTR.
typedef WORD ATL_URL_PORT;
class CUrl
{
ATL_URL_PORT m_nPortNumber;
....
inline BOOL Parse(_In_z_ LPCTSTR lpszUrl)
{
....
//get the port number
m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf);
if (m_nPortNumber < 0)
goto error;
....
};V547 Expression 'm_nPortNumber < 0' is always false. Unsigned type value is never < 0. atlutil.h 2775
Проверка, что номер порта меньше нуля, не сработает. Переменная 'm_nPortNumber' имеет беззнаковый тип ' WORD'. Тип 'WORD' это 'unsigned short'.
В заголовочных файлах Visual C++ есть следующий макрос.
#define DXVABitMask(__n) (~((~0) << __n))Везде где он используется, возникает неопределённо поведение. Конечно, разработчикам Visual C++ виднее, безопасна эта конструкция или нет. Возможно, есть уверенность, что Visual C++ будет всегда одинаково обрабатывать сдвиг отрицательных чисел. Формально, сдвиг отрицательного числа приводит к Undefined behavior. Подробнее эта тематика освещена в статье "Не зная брода, не лезь в воду. Часть третья".
Данный паттерн 64-битной ошибки подробно рассмотрен в составленных нами уроках по разработке 64-битных приложений на языке Си/Си++. Чтобы понять, в чем состоит ошибка, предлагаем ознакомиться с уроком под номером 12.
class CWnd : public CCmdTarget
{
....
virtual void WinHelp(DWORD_PTR dwData,
UINT nCmd = HELP_CONTEXT);
....
};
class CFrameWnd : public CWnd
{
....
};
class CFrameWndEx : public CFrameWnd
{
....
virtual void WinHelp(DWORD dwData,
UINT nCmd = HELP_CONTEXT);
....
};V301 Unexpected function overloading behavior. See first argument of function 'WinHelpW' in derived class 'CFrameWndEx' and base class 'CFrameWnd'. afxframewndex.h 154
Функция 'WinHelp' объявлена в классе 'CFrameWndEx' неправильно. Тип первого аргумента должен быть 'DWORD_PTR'. Аналогичную ошибку можно видеть и в некоторых других классах:
Таких мест было найдено достаточно много. Анализировать, опасен каждый конкретный случай или нет, весьма утомительно. Намного лучше с этим справятся создатели библиотек. Мы приведём только парочку примеров.
BOOL CDockablePane::PreTranslateMessage(MSG* pMsg)
{
....
CBaseTabbedPane* pParentBar = GetParentTabbedPane();
CPaneFrameWnd* pParentMiniFrame =
pParentBar->GetParentMiniFrame();
if (pParentBar != NULL &&
(pParentBar->IsTracked() ||
pParentMiniFrame != NULL &&
pParentMiniFrame->IsCaptured()
)
)
....
}V595 The 'pParentBar' pointer was utilized before it was verified against nullptr. Check lines: 2840, 2841. afxdockablepane.cpp 2840
Смотрите, в начале указатель 'pParentBar' используется для вызова функции GetParentMiniFrame(). Затем вдруг появляется подозрение, что этот указатель может быть равен NULL и выполняется соответствующая проверка.
AFX_CS_STATUS CDockingManager::DeterminePaneAndStatus(....)
{
....
CDockablePane* pDockingBar =
DYNAMIC_DOWNCAST(CDockablePane, *ppTargetBar);
if (!pDockingBar->IsFloating() &&
(pDockingBar->GetCurrentAlignment() &
dwEnabledAlignment) == 0)
{
return CS_NOTHING;
}
if (pDockingBar != NULL)
{
return pDockingBar->GetDockingStatus(
pt, nSensitivity);
}
....
}V595 The 'pDockingBar' pointer was utilized before it was verified against nullptr. Check lines: 582, 587. afxdockingmanager.cpp 582
В начале указатель 'pDockingBar' активно используется, а потом вдруг сравнивается с NULL.
И ещё один пример напоследок:
void CFrameImpl::AddDefaultButtonsToCustomizePane(....)
{
....
for (POSITION posCurr = lstOrigButtons.GetHeadPosition();
posCurr != NULL; i++)
{
CMFCToolBarButton* pButtonCurr =
(CMFCToolBarButton*)lstOrigButtons.GetNext(posCurr);
UINT uiID = pButtonCurr->m_nID;
if ((pButtonCurr == NULL) ||
(pButtonCurr->m_nStyle & TBBS_SEPARATOR) ||
(....)
{
continue;
}
....
}V595 The 'pButtonCurr' pointer was utilized before it was verified against nullptr. Check lines: 1412, 1414. afxframeimpl.cpp 1412
Смело обращаемся к члену класса 'm_nID'. А затем из условия видно, что указатель 'pButtonCurr' может быть равен 0.
CString m_strBrowseFolderTitle;
void CMFCEditBrowseCtrl::OnBrowse()
{
....
LPCTSTR lpszTitle = m_strBrowseFolderTitle != _T("") ?
m_strBrowseFolderTitle : (LPCTSTR)NULL;
....
}V623 Consider inspecting the '?:' operator. A temporary object is being created and subsequently destroyed. afxeditbrowsectrl.cpp 308
Тернарный оператор не может возвращать значения разных типов. Поэтому, из "(LPCTSTR)NULL" будет неявно создан объект типа CString. Затем из этой пустой строки будет неявно взят указатель на её буфер. Беда в том, что временный объект типа CString будет разрушен. В результате значения указателя 'lpszTitle' становится не валидным и работать с ним нельзя. Здесь можно почитать более подробное описание данного паттерна ошибки.
UINT CMFCPopupMenuBar::m_uiPopupTimerDelay = (UINT) -1;
....
void CMFCPopupMenuBar::OnChangeHot(int iHot)
{
....
SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE,
max(0, m_uiPopupTimerDelay - 1),
NULL);
....
}V547 Expression '(0) > (m_uiPopupTimerDelay - 1)' is always false. Unsigned type value is never < 0. afxpopupmenubar.cpp 968
Значение '-1' используется как специальный флаг. Используя макрос 'max' программисты хотели защититься от отрицательных значений в переменной m_uiPopupTimerDelay. Это не получится, так как переменная имеет беззнаковый тип. Она всегда больше или равна нулю. Корректный код должен выглядеть приблизительно так:
SetTimer(AFX_TIMER_ID_MENUBAR_REMOVE,
m_uiPopupTimerDelay == (UINT)-1 ? 0 : m_uiPopupTimerDelay - 1,
NULL);Аналогичная ошибка находится здесь:
BOOL CMFCTasksPaneTask::SetACCData(CWnd* pParent, CAccessibilityData&
data)
{
....
data.m_nAccHit = 1;
data.m_strAccDefAction = _T("Press");
data.m_rectAccLocation = m_rect;
pParent->ClientToScreen(&data.m_rectAccLocation);
data.m_ptAccHit;
return TRUE;
}V607 Ownerless expression 'data.m_ptAccHit'. afxtaskspane.cpp 96
Что такое "data.m_ptAccHit;" ? Быть может здесь забыли присвоить этой переменной какое-то значение?
BOOL CMFCTasksPane::GetMRUFileName(....)
{
....
const int MAX_NAME_LEN = 512;
TCHAR lpcszBuffer [MAX_NAME_LEN + 1];
memset(lpcszBuffer, 0, MAX_NAME_LEN * sizeof(TCHAR));
if (GetFileTitle((*pRecentFileList)[nIndex],
lpcszBuffer, MAX_NAME_LEN) == 0)
{
strName = lpcszBuffer;
return TRUE;
}
....
}V512 A call of the 'memset' function will lead to underflow of the buffer 'lpcszBuffer'. afxtaskspane.cpp 2626
Есть подозрение, что этот код может вернуть строку, которая не будет завершаться терминальным нулём. Скорее всего, следовало обнулить и последний элемент массива:
memset(lpcszBuffer, 0, (MAX_NAME_LEN + 1) * sizeof(TCHAR));void CMFCVisualManagerOfficeXP::OnDrawBarGripper(....)
{
....
if (bHorz)
{
rectFill.DeflateRect(4, 0);
}
else
{
rectFill.DeflateRect(4, 0);
}
....
}V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanagerofficexp.cpp 264
Если вы используете класс 'single_link_registry' то ваше приложение может неожиданно завершиться, даже если вы корректно обрабатываете все исключения. Посмотрим на деструктор класса 'single_link_registry':
virtual ~single_link_registry()
{
// It is an error to delete link registry with links
// still present
if (count() != 0)
{
throw invalid_operation(
"Deleting link registry before removing all the links");
}
}V509 The 'throw' operator inside the destructor should be placed within the try..catch block. Raising exception inside the destructor is illegal. agents.h 759
Этот деструктор может бросить исключение. Это плохая идея. Если в программе возникает исключение, начинается разрушение объектов путем вызова деструкторов. Если в деструкторе 'single_link_registry' произойдет ошибка, то возникнет ещё еще одно исключение. Оно в деструкторе не обрабатывается. В результате библиотека C++ немедленно аварийно завершает программу, вызывая функцию terminate().
Аналогичные плохие деструкторы:
void CPreviewView::OnPreviewClose()
{
....
while (m_pToolBar && m_pToolBar->m_pInPlaceOwner)
{
COleIPFrameWnd *pInPlaceFrame =
DYNAMIC_DOWNCAST(COleIPFrameWnd, pParent);
if (!pInPlaceFrame)
break;
CDocument *pViewDoc = GetDocument();
if (!pViewDoc)
break;
// in place items must have a server document.
COleServerDoc *pDoc =
DYNAMIC_DOWNCAST(COleServerDoc, pViewDoc);
if (!pDoc)
break;
// destroy our toolbar
m_pToolBar->DestroyWindow();
m_pToolBar = NULL;
pInPlaceFrame->SetPreviewMode(FALSE);
// restore toolbars
pDoc->OnDocWindowActivate(TRUE);
break;
}
....
}V612 An unconditional 'break' within a loop. viewprev.cpp 476
В цикле нет ни одного оператора 'continue'. В конце цикла стоит 'break'. Это очень странно. Цикл всегда выполняется только один раз. Здесь или ошибка или лучше заменить 'while' на 'if'.
Есть и другие несущественные замечания к коду, которые перечислять не интересно. Приведём только один пример, чтобы было понятно, о чём идёт речь.
В файле afxdrawmanager.cpp зачем то заведена константа для числа Пи:
const double AFX_PI = 3.1415926;V624 The constant 3.1415926 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. afxdrawmanager.cpp 22
Это конечно не ошибка и точности константы достаточно. Но не понятно, почему-бы не использовать константу M_PI, которая задана гораздо точнее:
#define M_PI 3.14159265358979323846К сожалению, у нас нет проекта и make-файлов для сборки библиотек, входящих в состав Visual C++. Поэтому наш анализ весьма поверхностен. Мы просто что-то нашли и написали про это. Не стоит думать, что нет других подозрительных мест :).
Уверены, что вам будет гораздо удобнее воспользоваться PVS-Studio для проверки библиотек. Если потребуется, мы готовы подсказать и помочь с интеграцией в make-файлы. Также вам будет легче понять, что является ошибкой, а что нет.
Смотрите, в Visual Studio 2012 есть статический анализ Си/Си++ кода. Однако это не означает, что этого достаточно. Это только первый шаг. Это только возможность легко и дёшево попробовать новую технологию для повышения качества кода. А когда понравится - приходите к нам и приобретайте PVS-Studio. Этот инструмент гораздо интенсивнее борется с дефектами. Он под это заточен. На нём мы зарабатываем деньги, а значит, очень активно его развиваем.
Мы нашли ошибки в библиотеках Visual C++, хотя там есть статический анализ. Мы нашли ошибки в компиляторе Clang, хотя в нём есть статический анализ. Приобретайте нас и мы будем регулярно находить ошибки в вашем проекте. Мы отлично интернируемся в Visual Studio 2005, 2008, 2010, 2012 и умеем искать ошибки в фоновом режиме.
Скачать и попробовать PVS-Studio можно здесь: http://www.viva64.com/ru/pvs-studio/.