Тяп-ляп или я проверил библиотеки Visual C++ 2013 (update 3)

Мне предложили проверить библиотеки, входящие в Visual Studio 2013. Ничего особенно примечательного я не нашёл. Только несколько мелких ошибок и недочётов. Интригующую статью из этого не сделаешь, но я всё равно опишу замеченные недостатки. Надеюсь, это сделает библиотеки чуть лучше, и подвигнет авторов провести более тщательную проверку. У меня нет файлов проектов для сборки библиотек. Поэтому я проверял файлы кое-как, и много могло быть пропущено.

Проверить полноценно библиотеки я не могу. Я поступил очень топорно. Включил в новый проект все файлы, содержащиеся в папках «crt\src» и «atlmfc\src». Плюс я сделал новый test.cpp файл, куда включил все заголовочные файлы, относящиеся к стандартной библиотеке (vector, map, set и так далее).

После этого, немного поколдовав с настройками проекта, я добился, что компилируется около 80% файлов. Думаю, этого достаточно. Даже если файл не компилируется, он, скорее всего, будет проверен PVS-Studio, пусть и не полностью.

Думаю, если эта статья заинтересует разработчиков библиотек, они смогут провести более тщательный анализ. Даже если сборка осуществляется экзотичным образом, теперь это не будет проблемой. Можно будет воспользоваться механизмом отслеживания запусков компилятора.

Для проверки я использовал PVS-Studio версии 5.19. Проверялись исходные коды Си/Си++ библиотек, входящие в состав Visual Studio 2013 (update 3).

 

Результаты проверки
Нашел все-таки недочеты. Да. Например, странно выглядит функция proj(), опасно устроен деструктор ~single_link_registry(). Попробуем найти что-то новое. Готовьте печеньки или поллитру(что вам по вкусу).

 

Неправильная проверка индекса

[c language=»++»]
void _Initialize_order_node(…., size_t _Index, ….)
{
if (_Index < 0)
{
throw std::invalid_argument("_Index");
}
….
}
[/c]

Предупреждение PVS-Studio: V547 Expression ‘_Index < 0′ is always false. Unsigned type value is never < 0. agents.h 8442

Аргумент _Index и имеет беззнаковый тип. Поэтому проверка не имеет смысла. Исключение не будет сгенерировано. Сдается мне, это не ошибка, а просто лишний код.

Ошибка формата

[c language=»++»]
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);
….
}
[/c]

Предупреждение 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’ является не указателем, а просто целочисленным значением. Должно быть написано:

[c language=»++»]
printf("\t_tfpecode = %i\n\n", ptd->_tfpecode);
[/c]

 

Странные повторяющиеся вычисления

[c language=»++»]
unsigned int SchedulerProxy::AdjustAllocationIncrease(….) const
{
….
unsigned int remainingConcurrency =
m_maxConcurrency — m_currentConcurrency;
remainingConcurrency = m_maxConcurrency — m_currentConcurrency;
….
}
[/c]

Предупреждение PVS-Studio: V519 The ‘remainingConcurrency’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1136, 1137. schedulerproxy.cpp 1137

Переменной два раза присваивается результат одного и того же выражения. Этот код избыточен и, скорее всего, получился в результате неудачного рефакторинга.

Подозреваю очепятку…

[c language=»++»]
double HillClimbing::CalculateThroughputSlope(….)
{
….
MeasuredHistory * lastHistory = GetHistory(fromSetting);
MeasuredHistory * currentHistory = GetHistory(toSetting);
….
double varianceOfcurrentHistory = currentHistory->VarianceMean();
double varianceOflastHistory = currentHistory->VarianceMean();
….
}
[/c]

Предупреждение 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 вот так:

[c language=»++»]
double varianceOflastHistory = varianceOfcurrentHistory;
[/c]

Более того, существует ещё указатель ‘lastHistory’. Выскажу предположение, что код содержит опечатку.

 

Вот ета лол…

[c language=»++»]
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);
}
[/c]

Предупреждение PVS-Studio: V503 This is a nonsensical comparison: pointer >= 0. dlgprop.cpp 1206

Странно проверять, что значение указателя больше или равно нулю. Это опечатка и, на самом деле, хотели проверить переменную ‘nPage’.

Конечно, это всего лишь ASSERT, и ошибка не приведёт ни к каким негативным последствиям. Но всё равно это ошибка.

Одни и те же действия, независимо от условия.

[c language=»++»]
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());
}
}
….
}
[/c]

Предупреждение PVS-Studio: V523 The ‘then’ statement is equivalent to the ‘else’ statement. afxvisualmanager.cpp 2118

В независимости от условия (pGroup->m_bIsSpecial), выполняются одни и те же действия. Это странно.

Неправильная проверка номера порта

[c language=»++»]
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;
….
}
[/c]

Предупреждение PVS-Studio: V547 Expression ‘m_nPortNumber < 0′ is always false. Unsigned type value is never < 0. atlutil.h 2773

Переменная ‘m_nPortNumber’ имеет беззнаковый тип WORD.

Нету визуального деструктора

[c language=»++»]
class CDataSourceControl
{
….
~CDataSourceControl();
….
virtual IUnknown* GetCursor();
virtual void BindProp(….);
virtual void BindProp(….);
….
}

CDataSourceControl* m_pDataSourceControl;

COleControlSite::~COleControlSite()
{
….
delete m_pDataSourceControl;
….
}
[/c]

Предупреждение 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, используя указатель на базовый класс.

Банальность — недописанный код

[c language=»++»]
BOOL CMFCWindowsManagerDialog::OnHelpInfo(HELPINFO* pHelpInfo)
{
pHelpInfo->iCtrlId;
CWnd* pParentFrame = AfxGetMainWnd();
pParentFrame->SendMessage(AFX_WM_WINDOW_HELP, 0,
(LPARAM) this);
return FALSE;
}
[/c]

Предупреждение PVS_Studio: V607 Ownerless expression ‘pHelpInfo->iCtrlId’. afxwindowsmanagerdialog.cpp 472

Что такое «pHelpInfo->iCtrlId;»? Что бы это значило?

Подозрительная двойная инициализация

[c language=»++»]
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; //<<—
….
}
[/c]

Предупреждение 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’ записывается значение другой переменной. А затем, то значение вдруг затирается нулём.

Подозрительная проверка статуса

[c language=»++»]
#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))
….
}
[/c]

Предупреждение 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.

Я не знаю, здесь ошибка или нет, но этот код сбивает с толку, и его следует проверить.

Неправильный аргумент для MAKE_HRESULT

[c language=»++»]
#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);
….
}
[/c]

Предупреждение 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 обоснованно. Вдруг в процессе рефакторинга переменная станет знаковой, а алгоритм не готов работать с отрицательными числами. Однако, существование некоторых из них скорее всего бессмысленно. Их нужно удалить из кода или заменить на другую полезную проверку.

Лишние приведения типов

[c language=»++»]
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);
….
}
[/c]

Предупреждение 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() количество символов в очень длинной строке, то он получит неверный результат.

Мне кажется, это явное приведении осталось в коде случайно и его следует просто удалить.

Использование до проверки

[c language=»++»]
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());
….
}
[/c]

Предупреждение 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’.

Такой код опасен и, думаю, не стоит объяснять почему.

Лишние переменные

Лишние переменные — не ошибка. Но лишние, они и есть лишние, и от них стоит избавиться.

[c language=»++»]
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();
}
[/c]

Предупреждение PVS-Studio: V808 ‘rectImage’ object of ‘CRect’ type was created but was not utilized. afxcontrolrenderer.h 89

Прямоугольник ‘rectImage’ создаётся, но никак потом не используется. Лишняя строчка в программе и несколько лишних тактов при работе Debug версии.

Заключение

Как видите получилась достаточно большая статья. Но, на самом деле, ничего существенного не найдено. Код библиотек Visual C++ однозначно качественный и отлаженный.

Буду рад, если эта статья поможет в дальнейшем сделать библиотеки Visual C++ чуть лучше. Отмечу ещё раз, что проверка была выполнена неполноценно. Разработчики библиотек Visual C++ смогут сделать это более качественно, так как имеют скрипты/проекты для сборки библиотек.

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *