Мне предложили проверить библиотеки, входящие в Visual Studio 2013. Ничего особенно примечательного я не нашёл. Только несколько мелких ошибок и недочётов. Интригующую статью из этого не сделаешь, но я всё равно опишу замеченные недостатки. Надеюсь, это сделает библиотеки чуть лучше, и подвигнет авторов провести более тщательную проверку. У меня нет файлов проектов для сборки библиотек. Поэтому я проверял файлы кое-как, и много могло быть пропущено.
Это уже вторая статья о проверке библиотек, входящих в Visual C++. О результатах предыдущей проверки можно узнать из статьи: Обнаружены ошибки в библиотеках Visual C++ 2012.
Проверить полноценно библиотеки я не могу. Я поступил очень топорно. Включил в новый проект все файлы, содержащиеся в папках "crt\src" и "atlmfc\src". Плюс я сделал новый test.cpp файл, куда включил все заголовочные файлы, относящиеся к стандартной библиотеке (vector, map, set и так далее).
После этого, немного поколдовав с настройками проекта, я добился, что компилируется около 80% файлов. Думаю, этого достаточно. Даже если файл не компилируется, он, скорее всего, будет проверен PVS-Studio, пусть и не полностью.
Думаю, если эта статья заинтересует разработчиков библиотек, они смогут провести более тщательный анализ. Даже если сборка осуществляется экзотичным образом, теперь это не будет проблемой. Можно будет воспользоваться механизмом отслеживания запусков компилятора.
Для проверки я использовал PVS-Studio версии 5.19. Проверялись исходные коды Си/Си++ библиотек, входящие в состав Visual Studio 2013 (update 3).
Встретились некоторые недочёты, которые мы находили и в предыдущей версии Visual Studio 2012. Например, всё так же странно выглядит функция proj(), опасно устроен деструктор ~single_link_registry(). Но повторяться не интересно. Попробуем найти что-то новое.
void _Initialize_order_node(...., size_t _Index, ....)
{
if (_Index < 0)
{
throw std::invalid_argument("_Index");
}
....
}Предупреждение PVS-Studio: V547 Expression '_Index < 0' is always false. Unsigned type value is never < 0. agents.h 8442
Аргумент _Index и имеет беззнаковый тип. Поэтому проверка не имеет смысла. Исключение никогда не будет сгенерировано. Думаю, это не ошибка, а просто лишний код.
int _tfpecode; /* float point exception code */
void __cdecl _print_tiddata1 (
_ptiddata ptd
)
{
....
printf("\t_gmtimebuf = %p\n", ptd->_gmtimebuf);
printf("\t_initaddr = %p\n", ptd->_initaddr);
printf("\t_initarg = %p\n", ptd->_initarg);
printf("\t_pxcptacttab = %p\n", ptd->_pxcptacttab);
printf("\t_tpxcptinfoptrs = %p\n", ptd->_tpxcptinfoptrs);
printf("\t_tfpecode = %p\n\n", ptd->_tfpecode);
....
}Предупреждение PVS-Studio: V576 Incorrect format. Consider checking the second actual argument of the 'printf' function. The pointer is expected as an argument. tidprint.c 133
Здесь мы имеем дело с эффектом последней строки. В конце блока однотипных строк допущена ошибка. Везде следует распечатывать значение указателя. Но в самом конце переменная '_tfpecode' является не указателем, а просто целочисленным значением. Должно быть написано:
printf("\t_tfpecode = %i\n\n", ptd->_tfpecode);unsigned int SchedulerProxy::AdjustAllocationIncrease(....) const
{
....
unsigned int remainingConcurrency =
m_maxConcurrency - m_currentConcurrency;
remainingConcurrency = m_maxConcurrency - m_currentConcurrency;
....
}Предупреждение PVS-Studio: V519 The 'remainingConcurrency' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1136, 1137. schedulerproxy.cpp 1137
Переменной два раза присваивается результат одного и того же выражения. Этот код избыточен и, скорее всего, получился в результате неудачного рефакторинга.
double HillClimbing::CalculateThroughputSlope(....)
{
....
MeasuredHistory * lastHistory = GetHistory(fromSetting);
MeasuredHistory * currentHistory = GetHistory(toSetting);
....
double varianceOfcurrentHistory = currentHistory->VarianceMean();
double varianceOflastHistory = currentHistory->VarianceMean();
....
}Предупреждение PVS-Studio: V656 Variables 'varianceOfcurrentHistory', 'varianceOflastHistory' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'currentHistory->VarianceMean()' expression. Check lines: 412, 413. hillclimbing.cpp 413
Подозрительно, что переменной varianceOfcurrentHistory и varianceOflastHistory присваивается одно и то же значение. Логично было-бы инициализировать varianceOflastHistory вот так:
double varianceOflastHistory = varianceOfcurrentHistory;Более того, существует ещё указатель 'lastHistory'. Выскажу предположение, что код содержит опечатку. Возможно, код должен быть таким:
double varianceOfcurrentHistory = currentHistory->VarianceMean();
double varianceOflastHistory = lastHistory->VarianceMean();BOOL CPropertySheet::SetActivePage(CPropertyPage* pPage)
{
ASSERT_VALID(this);
ENSURE_VALID(pPage);
ASSERT_KINDOF(CPropertyPage, pPage);
int nPage = GetPageIndex(pPage);
ASSERT(pPage >= 0);
return SetActivePage(nPage);
}Предупреждение PVS-Studio: V503 This is a nonsensical comparison: pointer >= 0. dlgprop.cpp 1206
Странно проверять, что значение указателя больше или равно нулю. Это опечатка и, на самом деле, хотели проверить переменную 'nPage':
int nPage = GetPageIndex(pPage);
ASSERT(nPage >= 0);Конечно, это всего лишь ASSERT, и ошибка не приведёт ни к каким негативным последствиям. Но всё равно это ошибка.
void CMFCVisualManager::OnDrawTasksGroupCaption(....)
{
....
if (pGroup->m_bIsSpecial)
{
if (!pGroup->m_bIsCollapsed)
{
CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
rectButton.TopLeft());
}
else
{
CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
rectButton.TopLeft());
}
}
else
{
if (!pGroup->m_bIsCollapsed)
{
CMenuImages::Draw(pDC, CMenuImages::IdArrowUp,
rectButton.TopLeft());
}
else
{
CMenuImages::Draw(pDC, CMenuImages::IdArrowDown,
rectButton.TopLeft());
}
}
....
}Предупреждение PVS-Studio: V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanager.cpp 2118
В независимости от условия (pGroup->m_bIsSpecial), выполняются одни и те же действия. Это странно.
typedef WORD ATL_URL_PORT;
ATL_URL_PORT m_nPortNumber;
inline BOOL Parse(_In_z_ LPCTSTR lpszUrl)
{
....
m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf);
if (m_nPortNumber < 0)
goto error;
....
}Предупреждение PVS-Studio: V547 Expression 'm_nPortNumber < 0' is always false. Unsigned type value is never < 0. atlutil.h 2773
Переменная 'm_nPortNumber' имеет беззнаковый тип WORD.
class CDataSourceControl
{
....
~CDataSourceControl();
....
virtual IUnknown* GetCursor();
virtual void BindProp(....);
virtual void BindProp(....);
....
}
CDataSourceControl* m_pDataSourceControl;
COleControlSite::~COleControlSite()
{
....
delete m_pDataSourceControl;
....
}Предупреждение PVS-Studio: V599 The destructor was not declared as a virtual one, although the 'CDataSourceControl' class contains virtual functions. occsite.cpp 77
Класс CDataSourceControl содержит в себе виртуальные методы, но при этом деструктор не является виртуальным. Это опасно. Если от класса CDataSourceControl будет унаследован класс X, то нельзя будет уничтожать объекты типа X, используя указатель на базовый класс.
BOOL CMFCWindowsManagerDialog::OnHelpInfo(HELPINFO* pHelpInfo)
{
pHelpInfo->iCtrlId;
CWnd* pParentFrame = AfxGetMainWnd();
pParentFrame->SendMessage(AFX_WM_WINDOW_HELP, 0,
(LPARAM) this);
return FALSE;
}Предупреждение PVS-Studio: V607 Ownerless expression 'pHelpInfo->iCtrlId'. afxwindowsmanagerdialog.cpp 472
Что такое "pHelpInfo->iCtrlId;"? Что бы это значило?
CMFCStatusBar::CMFCStatusBar()
{
m_hFont = NULL;
// setup correct margins
m_cxRightBorder = m_cxDefaultGap; // <=
m_cxSizeBox = 0;
m_cxLeftBorder = 4;
m_cyTopBorder = 2;
m_cyBottomBorder = 0;
m_cxRightBorder = 0; // <=
....
}Предупреждение PVS-Studio: V519 The 'm_cxRightBorder' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 74, 80. afxstatusbar.cpp 80
В начале в переменную 'm_cxRightBorder' записывается значение другой переменной. А затем, то значение вдруг затирается нулём.
#define S_OK ((HRESULT)0L)
#define E_NOINTERFACE _HRESULT_TYPEDEF_(0x80004002L)
HRESULT GetDocument(IHTMLDocument2** ppDoc) const
{
const T* pT = static_cast<const T*>(this);
return pT->GetDHtmlDocument(ppDoc) ? S_OK : E_NOINTERFACE;
}
HRESULT GetEvent(IHTMLEventObj **ppEventObj) const
{
....
if (GetDocument(&sphtmlDoc))
....
}Предупреждение PVS-Studio: V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'GetDocument(& sphtmlDoc)'. The SUCCEEDED or FAILED macro should be used instead. afxhtml.h 593
Оформление кода может не соответствовать его логике работы. Кажется, что, если условие 'GetDocument(...)' истинно, то удалось "получить" документ. Но, на самом деле, всё не так. Функция GetDocument() возвращает значение типа HRESULT. С этим типом всё наоборот. Например, статус S_OK закодирован как 0, а статус E_NOINTERFACE как 0x80004002L. Для проверки значений типа HRESULT следует использовать специальные макросы: SUCCEEDED, FAILED.
Я не знаю, есть здесь ошибка или нет, но этот код сбивает с толку, и его следует проверить.
#define MAKE_HRESULT(sev,fac,code) \
((HRESULT) \
(((unsigned long)(sev)<<31) | \
((unsigned long)(fac)<<16) | \
((unsigned long)(code))) )
ATLINLINE ATLAPI AtlSetErrorInfo(....)
{
....
hRes = MAKE_HRESULT(3, FACILITY_ITF, nID);
....
}Предупреждение PVS-Studio: V673 The '(unsigned long)(3) << 31' expression evaluates to 6442450944. 33 bits are required to store the value, but the expression evaluates to the 'unsigned' type which can only hold '32' bits. atlcom.h 6650
Всё будет работать правильно, но ошибка есть. Поясню.
Функция должна сформировать информацию об ошибке в переменной типа HRESULT. Для этого используется макрос MAKE_HRESULT. Но используется он неправильно. Программист посчитал, что первый параметр 'severity' может лежать в приделах от 0 до 3. Видимо, он перепутал это со способом формирования кодов ошибки, используемые при работе с функциями GetLastError()/SetLastError().
Макрос MAKE_HRESULT в качестве первого аргумента может принимать только 0 (success) или 1 (failure). Подробнее этот вопрос рассматривался на форуме сайта CodeGuru: Warning! MAKE_HRESULT macro doesn't work.
Так как в качестве первого фактического аргумента используется число 3, то возникает переполнение. Число 3 "превратится" в 1. Благодаря этой случайности ошибка не повлияет на работу программы.
Встречается достаточно много ситуаций, когда условие для ASSERT выглядит так: (X >= 0). При этом, переменная X объявлена, как беззнаковый целочисленный тип. Получается, что условие всегда истинно.
В некоторых случаях наличие таких ASSERT обоснованно. Вдруг в процессе рефакторинга переменная станет знаковой, а алгоритм не готов работать с отрицательными числами. Однако, существование некоторых из них скорее всего бессмысленно. Их нужно удалить из кода или заменить на другую полезную проверку. Поэтому я и решил упомянуть их в статье.
Рассмотрим пример:
DWORD m_oversubscribeCount;
void ExternalContextBase::Oversubscribe(....)
{
if (beginOversubscription)
{
ASSERT(m_oversubscribeCount >= 0);
++m_oversubscribeCount;
}
....
}Предупреждение PVS-Studio: V547 Expression 'm_oversubscribeCount >= 0' is always true. Unsigned type value is always >= 0. externalcontextbase.cpp 204
Остальные предупреждения приведу просто списком:
Нашлось несколько явных приведений типов, которые не только лишние, но ещё и могут испортить значения.
Рассмотрим первый пример:
size_t __cdecl strnlen(const char *str, size_t maxsize);
size_t __cdecl _mbstrnlen_l(const char *s,
size_t sizeInBytes,
_locale_t plocinfo)
{
....
if ( _loc_update.GetLocaleT()->locinfo->mb_cur_max == 1 )
/* handle single byte character sets */
return (int)strnlen(s, sizeInBytes);
....
}Предупреждение PVS-Studio: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'strnlen(s, sizeInBytes)'. _mbslen_s.c 67
Функция strnlen() возвращает значение типа 'size_t'. Затем оно вдруг явно приводится к типу 'int'. После чего, значение неявно вновь будет расширено до типа size_t.
Этот код содержит потенциальную 64-битную ошибку. Если вдруг кто-то захочет в 64-битной программе посчитать с помощью функции _mbstrnlen_l() количество символов в очень длинной строке, то он получит неверный результат.
Мне кажется, это явное приведении осталось в коде случайно и его следует просто удалить.
Рассмотрим второй пример:
WINBASEAPI SIZE_T WINAPI GlobalSize (_In_ HGLOBAL hMem);
inline void __cdecl memcpy_s(
_Out_writes_bytes_to_(_S1max,_N) void *_S1,
_In_ size_t _S1max,
_In_reads_bytes_(_N) const void *_S2,
_In_ size_t _N);
AFX_STATIC HGLOBAL AFXAPI _AfxCopyGlobalMemory(....)
{
ULONG_PTR nSize = ::GlobalSize(hSource);
....
Checked::memcpy_s(lpDest, (ULONG)::GlobalSize(hDest),
lpSource, (ULONG)nSize);
....
}Предупреждение PVS-Studio: V220 Suspicious sequence of types castings: memsize -> 32-bit integer -> memsize. The value being casted: 'nSize'. olemisc.cpp 684.
Функция GlobalSize() возвращает тип SIZE_T. Аргументы функции memcpy_s() тоже имеют тип size_t.
Так зачем тогда сделано явное приведение типа "(ULONG)::GlobalSize(hDest)" ?
Если мы будем работать с буфером более 4Gb, то функция memcpy_s() будет копировать только часть массива.
Есть ещё несколько подозрительных приведений типов:
CMFCPopupMenu* CMFCCustomizeButton::CreatePopupMenu()
{
....
if (m_pWndParentToolbar->IsLocked())
{
pMenu->GetMenuBar()->m_pRelatedToolbar = m_pWndParentToolbar;
}
pMenu->m_bRightAlign = m_bMenuRightAlign &&
(m_pWndParentToolbar->GetExStyle() & WS_EX_LAYOUTRTL) == 0;
BOOL bIsLocked = (m_pWndParentToolbar == NULL ||
m_pWndParentToolbar->IsLocked());
....
}Предупреждение PVS-Studio: V595 The 'm_pWndParentToolbar' pointer was utilized before it was verified against nullptr. Check lines: 192, 199. afxcustomizebutton.cpp 192
В начале указатель 'm_pWndParentToolbar' разыменовывается в выражении 'm_pWndParentToolbar->IsLocked()'. А затем проверяется на равенство нулю: 'm_pWndParentToolbar == NULL'.
Такой код опасен и, думаю, не стоит объяснять почему.
Ещё один такой случай:
void COleControlSite::BindDefaultProperty(....)
{
....
if (pDSCWnd != NULL)
{
....
m_pDSCSite = pDSCWnd->m_pCtrlSite;
....
m_pDSCSite->m_pDataSourceControl->BindProp(this, TRUE);
if (m_pDSCSite != NULL)
m_pDSCSite->m_pDataSourceControl->BindColumns();
}
....
}Предупреждение PVS-Studio: V595 The 'm_pDSCSite' pointer was utilized before it was verified against nullptr. Check lines: 1528, 1529. occsite.cpp 1528
Лишние переменные - не ошибка. Но лишние, они и есть лишние, и от них стоит избавиться. Пример:
int GetImageCount() const
{
CRect rectImage(m_Params.m_rectImage);
if (m_Bitmap.GetCount() == 1)
{
HBITMAP hBmp = m_Bitmap.GetImageWell();
BITMAP bmp;
if (::GetObject(hBmp, sizeof(BITMAP), &bmp) ==
sizeof(BITMAP))
{
return bmp.bmHeight / m_Params.m_rectImage.Height();
}
return 0;
}
return m_Bitmap.GetCount();
}Предупреждение PVS-Studio: V808 'rectImage' object of 'CRect' type was created but was not utilized. afxcontrolrenderer.h 89
Прямоугольник 'rectImage' создаётся, но никак потом не используется. Лишняя строчка в программе и несколько лишних тактов при работе Debug версии.
Привожу список найденных ненужных переменных: vs2003_V808.txt
Достаточно много предупреждений можно отнести не к ошибкам, а скорее к неудачному стилю программирования. Как мне кажется, исходные коды библиотек Visual C++ должны служить другим программистам образцом для подражания. Поэтому не стоит учить их плохим вещам.
Перечислю некоторые фрагменты, которые можно улучшить.
_PHNDLR __cdecl signal(int signum, _PHNDLR sigact)
{
....
if ( SetConsoleCtrlHandler(ctrlevent_capture, TRUE)
== TRUE )
....
}Предупреждение PVS-Studio: V676 It is incorrect to compare the variable of BOOL type with TRUE. winsig.c 255
Везде, в том числе и в MSDN говорится, что нехорошо сравнивать что-то с TRUE. Функция может вернуть любое значение, отличное от 0, и это будет считаться истинной. TRUE, это 1. Правильно всегда сравнивать: Foo() != FALSE.
Аналогичные проверки можно встретить здесь:
void _To_array(
::Concurrency::details::_Dynamic_array<_EType>& _Array)
{
_LockHolder _Lock(_M_lock);
_M_iteratorCount++;
for(_LinkRegistry::iterator _Link = _M_links.begin();
*_Link != NULL; _Link++)
{
_Array._Push_back(*_Link);
}
}Предупреждение PVS-Studio: V803 Decreased performance. In case '_Link' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. agents.h 1713
Мелочь, конечно, но везде рекомендуют использовать ++iterator. Если здесь можно, то лучше использовать префиксный оператор, чтобы демонстрировать хороший стиль для обучения других.
Примечание. Заметки на эту тему:
Если авторы библиотек посчитают, что стоит позаниматься с инкрементом, то вот список мест для этого: vs2003_V803.txt.
#pragma warning (disable : 4311)
SetClassLongPtr(m_hWnd,
GCLP_HBRBACKGROUND,
PtrToLong(reinterpret_cast<void*>(
::GetSysColorBrush(COLOR_BTNFACE))));
#pragma warning (default : 4311)Предупреждение 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: 165, 167. afxbasepane.cpp 167
Правильным способом возвращения предыдущего состояние предупреждения является использование "#pragma warning(push[ ,n ])" и "#pragma warning(pop)".
Остальные места: vs2003_V665.txt.
Классика жанра:
_AFXWIN_INLINE CWnd::operator HWND() const
{ return this == NULL ? NULL : m_hWnd; }Предупреждение PVS-Studio: V704 'this == 0' expression should be avoided - this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin2.inl 19
К сожалению, это очень частый паттерн. Особенно в MFC. Но стоит потихоньку отучать людей использовать такие конструкции и показывать хороший пример.
Для тех читателей, которые ещё не знают, почему это плохо, предлагаю познакомиться с документацией к диагностике V704. Там достаточно подробно всё описано.
Я понимаю, что поправить operator HWND() нет никакой возможности. Обратная совместимость важней. Но вдруг где-то это можно будет сделать безболезненно. Список таких проверок: vs2003_V704.txt
Как видите получилась достаточно большая статья. Но, на самом деле, ничего существенного не найдено. Код библиотек Visual C++ однозначно качественный и отлаженный.
Буду рад, если эта статья поможет в дальнейшем сделать библиотеки Visual C++ чуть лучше. Отмечу ещё раз, что проверка была выполнена неполноценно. Разработчики библиотек Visual C++ смогут сделать это более качественно, так как имеют скрипты/проекты для сборки библиотек. Если будут какие-то сложности, я готов помочь - пишите нам в поддержку.