Эта статья демонстрирует возможности методологии статического анализа кода. Читателю предлагается примеры ста ошибок, найденных в Open-Source проектах, написанных на языке Си/Си++. Все ошибки найдены с помощью статического анализатора кода PVS-Studio.
Данная статья устарела. Обновляемый список статей о проверенных проектах находится здесь.
Не будем утомлять программиста чтением текстов и сразу перейдем к примерам ошибок. Тем, кто хочет узнать, что такое статический анализ кода, предлагаем познакомиться с текстом по ссылке. Тем, кто хочет узнать, что такое PVS-Studio и скачать триал, предлагаем эту страницу: https://pvs-studio.com/ru/pvs-studio/.
Да, чуть не забыли. Ещё предлагаем познакомиться с заметкой "Ответы на вопросы, которые часто задают после прочтения наших статей".
Примеры найденных ошибок будут разделены на несколько групп. Это деление весьма условно. Часто одну и ту же ошибку можно отнести одновременно к опечатке, к уязвимостям и некорректной работе с массивами.
Из проверенных нами проектов, конечно, взято только по несколько ошибок. Если описывать все найденные дефекты, то это уже будет справочник. Список проверенных проектов:
Ошибки в обработке массивов и строк являются самым обширным классом дефектов в программах на языке Си/Си++. Это плата за возможность эффективной низкоуровневой работы с оперативной памятью. В статье будет показа лишь малая часть подобных ошибок, найденных анализатором PVS-Studio. Но думаем, что любой Си/Си++ программист понимает, как их много и как они коварны.
Пример 1. Проект Wolfenstein 3D. Очистка только части объекта.
void CG_RegisterItemVisuals( int itemNum ) {
...
itemInfo_t *itemInfo;
...
memset( itemInfo, 0, sizeof( &itemInfo ) );
...
}Ошибка найдена с помощью диагностики V568: It's odd that the argument of sizeof() operator is the '&itemInfo' expression. cgame cg_weapons.c 1467.
Оператор sizeof() вычисляет размер указателя, а не структуры 'itemInfo_t'. На самом деле должно быть написано "sizeof(*itemInfo)".
Пример 2. Проект Wolfenstein 3D. Копирование только части матрицы.
ID_INLINE mat3_t::mat3_t( float src[ 3 ][ 3 ] ) {
memcpy( mat, src, sizeof( src ) );
}Ошибка найдена с помощью диагностики V511: The sizeof() operator returns size of the pointer, and not of the array, in 'sizeof(src)' expression. Splines math_matrix.h 94
Как правило, программисты ожидают, что 'sizeof(src)' вернет размер массива равного "3*3*sizeof(float)". Но согласно стандарту языка, 'src' это просто указатель, а вовсе не массив. Таким образом, матрица будет скопирована только частично. Функция 'memcpy' скопирует 4 или 8 байт (размер указателя) в зависимости от того, этот код 32-битный или 64-битный.
Если хочется скопировать матрицу целиком, то можно передать в функцию ссылку на массив. Корректный вариант кода:
ID_INLINE mat3_t::mat3_t( float (&src)[3][3] )
{
memcpy( mat, src, sizeof( src ) );
}Пример 3. Проект FAR Manager. Очистка только части массива.
struct TreeItem
{
int *Last;
size_t LastCount;
...
void Clear()
{
strName.Clear();
memset(Last, 0, sizeof(Last));
Depth=0;
}
};Ошибка найдена с помощью диагностики V579: The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. far treelist.hpp 66
Скорее всего, здесь не хватает умножения на количество очищаемых элементов, и код должен был выглядеть так: "memset(Last, 0, LastCount * sizeof(*Last));".
Пример 4. Проект ReactOS. Некорректное вычисление длины строки.
static const PCHAR Nv11Board = "NV11 (GeForce2) Board";
static const PCHAR Nv11Chip = "Chip Rev B2";
static const PCHAR Nv11Vendor = "NVidia Corporation";
BOOLEAN
IsVesaBiosOk(...)
{
...
if (!(strncmp(Vendor, Nv11Vendor, sizeof(Nv11Vendor))) &&
!(strncmp(Product, Nv11Board, sizeof(Nv11Board))) &&
!(strncmp(Revision, Nv11Chip, sizeof(Nv11Chip))) &&
(OemRevision == 0x311))
...
}Ошибка найдена с помощью диагностики V579: The strncmp function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. vga vbe.c 57
Имеющиеся в коде вызовы функции 'strncmp' сравнивают только несколько первых символов, а не строки целиком. Ошибка в том, что для вычисления длины строк используется совершенно неуместный здесь оператор sizeof(). Оператор sizeof() вычисляет размер указателя, а вовсе не количество байт в строке.
Самое неприятное и коварное с этой ошибкой в том, что этот код почти работает. В 99% случаев сравнение по первым нескольким символам бывает достаточно. Зато 1% может подарить массу удовольствий и долгую отладку.
Пример 5. Проект VirtualDub. Выход за рамки массива (явный индекс).
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];
...
}Фрагмент кода найден с помощью диагностики V557: Array overrun is possible. The '9' index is pointing beyond array bound. VirtualDub f_convolute.cpp 73
Это пример не настоящей ошибки, но зато хорошей диагностики. Почему это не ошибка, поясняет автор: http://www.virtualdub.org/blog/pivot/entry.php?id=359.
Пример 6. Проект CPU Identifying Tool. Выход за рамки массива (индекс в макросе).
#define FINDBUFFLEN 64 // Max buffer find/replace size
...
int WINAPI Sticky (...)
{
...
static char findWhat[FINDBUFFLEN] = {'\0'};
...
findWhat[FINDBUFFLEN] = '\0';
...
}Ошибка найдена с помощью диагностики V557: Array overrun is possible. The '64' index is pointing beyond array bound. stickies stickies.cpp 7947
Эта ошибочная ситуация является разновидностью предыдущей. Терминальный ноль записывается за границей массива. Корректным вариантом кода будет: "findWhat[FINDBUFFLEN - 1] = '\0';".
Пример 7. Проект Wolfenstein 3D. Выход за рамки массива (неверное выражение).
typedef struct bot_state_s
{
...
char teamleader[32]; //netname of the team leader
...
} bot_state_t;
void BotTeamAI( bot_state_t *bs ) {
...
bs->teamleader[sizeof( bs->teamleader )] = '\0';
...
}Ошибка найдена с помощью диагностики V557: Array overrun is possible. The 'sizeof (bs->teamleader)' index is pointing beyond array bound. game ai_team.c 548
И ещё один пример выхода за границу массива при использовании явно заданного индекса. Эти примеры показывают, что такие, на первый взгляд простые, ошибки распространены гораздо шире, чем может показаться.
Терминальный ноль записывается за пределами массива 'teamleader'. Корректный вариант:
bs->teamleader[
sizeof(bs->teamleader) / sizeof(bs->teamleader[0]) - 1
] = '\0';Пример 8. Проект Miranda IM. Копирование только части строки.
typedef struct _textrangew
{
CHARRANGE chrg;
LPWSTR lpstrText;
} TEXTRANGEW;
const wchar_t* Utils::extractURLFromRichEdit(...)
{
...
::CopyMemory(tr.lpstrText, L"mailto:", 7);
...
}Ошибка найдена с помощью диагностики V512: A call of the 'memcpy' function will lead to a buffer overflow or underflow. tabsrmm utils.cpp 1080
Если используются Unicode-строки, то один символ занимает не один байт, а 2 или 4 байта (в зависимости от используемой модели данных в компиляторе). К сожалению, про этом легко забывается, и нередко в программах можно встретить дефекты, аналогичные тому, который показан здесь.
Функция 'CopyMemory' скопирует только часть строки L"mailto:", так как работает с байтами, а не с символами. Код можно исправить, используя более подходящую функцию для копирования строк или, по крайней мере, умножив число 7 на sizeof(wchar_t).
Пример 9. Проект CMake. Выход за границу массива внутри цикла.
static const struct {
DWORD winerr;
int doserr;
} doserrors[] =
{
...
};
static void
la_dosmaperr(unsigned long e)
{
...
for (i = 0; i < sizeof(doserrors); i++)
{
if (doserrors[i].winerr == e)
{
errno = doserrors[i].doserr;
return;
}
}
...
}Ошибка найдена с помощью диагностики V557: Array overrun is possible. The value of 'i' index could reach 367. cmlibarchive archive_windows.c 1140, 1142
Обработчик ошибок сам содержит ошибку. Оператор sizeof() возвращает размер массива в байтах, а не количество элементов в нём. В результате, в цикле программа попытается перебирать намного больше элементов, чем должна. Корректный цикл должен выглядеть так:
for (i = 0; i < sizeof(doserrors) / sizeof(*doserrors); i++)Пример 10. Проект CPU Identifying Tool. Печать строки саму в себя.
char * OSDetection ()
{
...
sprintf(szOperatingSystem,
"%sversion %d.%d %s (Build %d)",
szOperatingSystem,
osvi.dwMajorVersion,
osvi.dwMinorVersion,
osvi.szCSDVersion,
osvi.dwBuildNumber & 0xFFFF);
...
sprintf (szOperatingSystem, "%s%s(Build %d)",
szOperatingSystem, osvi.szCSDVersion,
osvi.dwBuildNumber & 0xFFFF);
...
}Ошибка найдена с помощью диагностики V541: It is dangerous to print the string 'szOperatingSystem' into itself. stickies camel.cpp 572, 603
К достаточно печальным последствиям может привести попытка форматированной печати строки в саму себя. Результат работы такого кода зависит от входных данных и предсказать, что произойдет сложно. Скорее всего, результатом работы станет бессмысленная строка или возникнет Access Violation.
Эту ошибку вполне можно отнести к категории "уязвимость в коде". В ряде программ, подав на вход специальные данные, можно использовать такие участки кода, чтобы привести к переполнению буфера или иным полезным злоумышленнику действиям.
Пример 11. Проект FCE Ultra. Для строки выделяется памяти меньше, чем надо.
int FCEUI_SetCheat(...)
{
...
if((t=(char *)realloc(next->name,strlen(name+1))))
...
}Ошибка найдена с помощью диагностики V518: The 'realloc' function allocates strange amount of memory calculated by 'strlen(expr)'. Perhaps the correct variant is 'strlen(expr) + 1'. fceux cheat.cpp 609
Причиной ошибки является опечатка. Аргументом функции strlen() должен быть указатель 'name', а вовсе не выражение "name+1". В результате, функция realloc выделит на 2 байта меньше памяти, чем необходимо. Один байт потеряется из-за того, что к длине строки не прибавлена единица. Другой байт потеряется из-за того, что функция 'strlen' считает длину строки, пропустив первый символ.
Пример 12. Проект Notepad++. Частичное обнуление массива.
#define CONT_MAP_MAX 50
int _iContMap[CONT_MAP_MAX];
...
DockingManager::DockingManager()
{
...
memset(_iContMap, -1, CONT_MAP_MAX);
...
}Ошибка найдена с помощью диагностики V512: A call of the memset function will lead to a buffer overflow or underflow. notepadPlus DockingManager.cpp 60
Очередная путаница с количеством элементов в массиве и его размером. Забыто умножение на sizeof(int).
Мы готовы продолжать и продолжать показывать ошибки работы с массивами, найденные нами в различных программах. Но надо где-то остановиться.
В начале немного теории.
Неопределённое поведение (англ. undefined behaviour) — свойство некоторых языков программирования (наиболее заметно в Cи и Си++) в определённых ситуациях выдавать результат, зависящий от реализации компилятора. Другими словами, спецификация не определяет поведение языка в любых возможных ситуациях, а говорит: "при условии А результат операции Б не определён". Допускать такую ситуацию в программе считается ошибкой, даже если на некотором компиляторе программа успешно выполняется, она не будет кроссплатформенной и может отказать на другой машине в другой ОС и даже на других настройках компилятора.
Точка следования (англ. Sequence point) — в программировании любая точка программы, в которой гарантируется, что все побочные эффекты предыдущих вычислений уже проявились, а побочные эффекты последующих еще отсутствуют. Подробнее про точки следования и какие ситуации неопределенного поведения с ними связаны можно прочитать здесь: https://pvs-studio.com/ru/blog/terms/0065/.
Пример 1. Проект Chromium. Некорректное использование умного указателя.
void AccessibleContainsAccessible(...)
{
...
auto_ptr<VARIANT> child_array(new VARIANT[child_count]);
...
}Ошибка найдена с помощью диагностики V554: Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. interactive_ui_tests accessibility_win_browsertest.cc 171
Это пример демонстрирует, когда использование умного указателя может привести к неопределенному поведению. Проявить себя это может повреждением кучи, аварийным завершением программы, неполному разрушению объектов или любым другим образом. Ошибка заключается в том, что память выделяется с помощью оператора new [], а освобождается в деструкторе класса 'auto_ptr' с помощью оператора delete:
~auto_ptr() {
delete _Myptr;
}Чтобы исправить ситуации, необходимо использовать более подходящий класс, например boost::scoped_array.
Пример 2. Проект IPP Samples. Классический Undefined behavior.
template<typename T, Ipp32s size> void HadamardFwdFast(...)
{
Ipp32s *pTemp;
...
for(j=0;j<4;j++) {
a[0] = pTemp[0*4] + pTemp[1*4];
a[1] = pTemp[0*4] - pTemp[1*4];
a[2] = pTemp[2*4] + pTemp[3*4];
a[3] = pTemp[2*4] - pTemp[3*4];
pTemp = pTemp++;
...
}
...
}Ошибка найдена с помощью диагностики V567: Undefined behavior. The 'pTemp' variable is modified while being used twice between sequence points. me umc_me_cost_func.h 168
Это классический пример неопределенного поведения программы. Именно такую конструкцию используют для демонстрации Undefined behavior во многих статьях. Неизвестно, увеличится pTemp на единицу или нет. Два действия по изменению значения pTemp находятся в одной точке следования. Это значит, что компилятор может создать следующий псевдокод:
pTemp = pTemp + 1;
pTemp = pTemp;
А может, создать другой вариант кода:
TMP = pTemp;
pTemp = pTemp + 1;
pTemp = TMP;
Какой именно код будет создан, зависит от компилятора и ключей оптимизации.
Пример 3. Проект Fennec Media Project. Сложное выражение.
uint32 CUnBitArrayOld::DecodeValueRiceUnsigned(uint32 k)
{
...
while (!(m_pBitArray[m_nCurrentBitIndex >> 5] &
Powers_of_Two_Reversed[m_nCurrentBitIndex++ & 31])) {}
...
}Ошибка найдена с помощью диагностики V567: Undefined behavior. The 'm_nCurrentBitIndex' variable is modified while being used twice at single sequence point. MACLib unbitarrayold.cpp 78
Между двумя использованиями переменной m_nCurrentBitIndex нет точек следования. Это значит, что стандартом не определено, в какой момент эта переменная увеличится. Соответственно, в зависимости от компилятора и ключей оптимизации, этот код может работать по-разному.
Пример 4. Проект Miranda IM. Сложное выражение.
short ezxml_internal_dtd(ezxml_root_t root,
char *s, size_t len)
{
...
while (*(n = ++s + strspn(s, EZXML_WS)) && *n != '>') {
...
}Ошибка найдена с помощью диагностики V567: Undefined behavior. The 's' variable is modified while being used twice between sequence points.msne zxml.c 371
Здесь используется префиксный инкремент переменной. Но это ничего не значит. Нет никакой гарантии, что переменная 's' будет увеличена перед вызовом функции strspn().
Для легкости понимания примеров освежим в памяти таблицу приоритетов операций.
Пример 1. Проект MySQL. Приоритет операции ! и &.
int ha_innobase::create(...)
{
...
if (srv_file_per_table
&& !mysqld_embedded
&& (!create_info->options & HA_LEX_CREATE_TMP_TABLE)) {
...
}Ошибка найдена с помощью диагностики V564: The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. innobase ha_innodb.cc 6789
По замыслу программиста, часть выражения должна проверить, что определенный бит в переменной 'create_info->options' равен нулю. Однако, приоритет операции '!' выше, чем операции '&'. И выражение работает так:
((!create_info->options) & HA_LEX_CREATE_TMP_TABLE)Чтобы код работал правильно, необходимо использовать дополнительные скобки:
(!(create_info->options & HA_LEX_CREATE_TMP_TABLE))Или, что на наш взгляд более красиво, написать так:
((create_info->options & HA_LEX_CREATE_TMP_TABLE) == 0)Пример 2. Проект eMule Plus. Приоритет операции * и ++.
STDMETHODIMP
CCustomAutoComplete::Next(..., ULONG *pceltFetched)
{
...
if (pceltFetched != NULL)
*pceltFetched++;
...
}Ошибка найдена с помощью диагностики V532: Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. emule customautocomplete.cpp 277
Если указатель 'pceltFetched' не нулевой, функция должна увеличивать переменную типа ULONG, на которую этот указатель указывает. Ошибка в том, что в приоритет операции '++' выше, чем приоритет операции '*' (разыменования указателя). Строка "*pceltFetched++;" эквивалентна следующим действиям:
TMP = pceltFetched + 1;
*pceltFetched;
pceltFetched = TMP;Фактически, здесь просто увеличивается значение указателя. Чтобы код стал корректен, необходимо добавить скобки: "(*pceltFetched)++;".
Пример 3. Проект Chromium. Приоритет операции & и !=.
#define FILE_ATTRIBUTE_DIRECTORY 0x00000010
bool GetPlatformFileInfo(PlatformFile file, PlatformFileInfo* info) {
...
info->is_directory =
file_info.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0;
...
}Ошибка найдена с помощью диагностики V564: The '&' operator is applied to bool type value. You've probably forgotten to include parentheses or intended to use the '&&' operator. base platform_file_win.cc 216
Очень легко забыть, что приоритет операции '!=' выше, чем операции '&'. Так произошло и здесь. В результате получается выражение:
info->is_directory =
file_info.dwFileAttributes & (0x00000010 != 0);Упростим выражение:
info->is_directory = file_info.dwFileAttributes & (true);Ещё раз упростим выражение:
info->is_directory = file_info.dwFileAttributes & 1;Получается, что вместо пятого бита, протестировали первый бит. Чтобы исправить ситуацию, требуются вставить дополнительные скобки.
Пример 4. Проект BCmenu. Путаница с IF и ELSE.
void BCMenu::InsertSpaces(void)
{
if(IsLunaMenuStyle())
if(!xp_space_accelerators) return;
else
if(!original_space_accelerators) return;
...
}Ошибка найдена с помощью диагностики V563: It is possible that this 'else' branch must apply to the previous 'if' statement. fire bcmenu.cpp 1853
Здесь ошибка не с приоритетом операций, но родственная ей. Не учтено, что ветка 'else' относится к ближайшему оператору 'if'. Видно, что код оформлен исходя из того, как будто он работает так:
if(IsLunaMenuStyle()) {
if(!xp_space_accelerators) return;
} else {
if(!original_space_accelerators) return;
}Но на самом деле он эквивалентен следующей конструкции:
if(IsLunaMenuStyle())
{
if(!xp_space_accelerators) {
return;
} else {
if(!original_space_accelerators) return;
}
}Пример 5. Проект IPP Samples. Приоритет операции ?: и |.
vm_file* vm_file_fopen(...)
{
...
mds[3] = FILE_ATTRIBUTE_NORMAL |
(islog == 0) ? 0 : FILE_FLAG_NO_BUFFERING;
...
}Ошибка найдена с помощью диагностики V502: Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '|' operator. vm vm_file_win.c 393
В зависимости от значения переменной 'islog', выражение должно было быть равно "FILE_ATTRIBUTE_NORMAL" или "FILE_ATTRIBUTE_NORMAL | FILE_FLAG_NO_BUFFERING". Но этого не происходит. Приоритет операции '?:' ниже, чем операции '|'. В результате код работает так:
mds[3] = (FILE_ATTRIBUTE_NORMAL | (islog == 0)) ?
0 : FILE_FLAG_NO_BUFFERING;Упростим выражение:
mds[3] = (0x00000080 | ...) ? 0 : FILE_FLAG_NO_BUFFERING;Так как FILE_ATTRIBUTE_NORMAL равняется 0x00000080, то условие всегда истинно. Это означает, что в mds[3] всегда будет записываться 0.
Пример 6. Проект Newton Game Dynamics. Приоритет операции ?: и *.
dgInt32 CalculateConvexShapeIntersection (...)
{
...
den = dgFloat32 (1.0e-24f) *
(den > dgFloat32 (0.0f)) ?
dgFloat32 (1.0f) : dgFloat32 (-1.0f);
...
}Ошибка найдена с помощью диагностики V502: Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '*' operator. physics dgminkowskiconv.cpp 1061
В этом коде ошибка вновь связана с низким приоритетом операции '?:'. Условием для оператора '?:' является бессмысленное подвыражение "dgFloat32 (1.0e-24f) * (den > dgFloat32 (0.0f))". Исправить ситуацию можно, используя круглые скобки.
Кстати, программисты часто забывают о коварстве оператора '?:'. Предлагаю заметку на эту тему: "Как уменьшить вероятность ошибки на этапе написания кода. Заметка N2".
Примеры этих ошибок однообразны и скучны, поэтому рассмотрим совсем немного примеров. Их суть в том, что функции с переменным количеством аргументов принимают фактические аргументы, несовместимые со строкой, задающей формат. Любой программист, использующий такие функции, как printf(), хорошо знаком с данной разновидностью ошибок.
Пример 1. Проект ReactOS. Некорректная печать символа типа WCHAR.
static void REGPROC_unescape_string(WCHAR* str)
{
...
default:
fprintf(stderr,
"Warning! Unrecognized escape sequence: \\%c'\n",
str[str_idx]);
...
}Ошибка найдена с помощью диагностики V576: Incorrect format. Consider checking the third actual argument of the 'fprintf' function. The char type argument is expected. regedit regproc.c 293
Функция fprinf() должна распечатать символ типа char. Но третьим аргументов является символ типа WCHAR. Пользователю будет выдано некорректно сформированное сообщение. Чтобы код стал корректен, в строке, задающей формат, следует заменить '%c' на '%C'.
Пример 2. Проект Intel AMT SDK. Пропущенный символ '%'.
void addAttribute(...)
{
...
int index = _snprintf(temp, 1023,
"%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
"%02x%02x:02x%02x:%02x%02x:%02x%02x",
value[0],value[1],value[2],value[3],value[4],
value[5],value[6],value[7],value[8],
value[9],value[10],value[11],value[12],
value[13],value[14],value[15]);
...
}Ошибка найдена с помощью диагностики V576: Incorrect format. A different number of actual arguments is expected while calling '_snprintf' function. Expected: 18. Present: 19. mod_pvs mod_pvs.cpp 308
На взгляд найти здесь ошибку очень непросто. Однако, статический анализатор PVS-Studio неутомим и замечает, что функция принимает больше фактических аргументов, чем задано в строке форматирования. Дело в том, что в одном месте пропущен символ '%'. Выделим этот фрагмент:
"%02x%02x:[HERE]02x%02x:%02x%02x:%02x%02x",Пример 3. Проект Intel AMT SDK. Неиспользуемый аргумент.
bool GetUserValues(...)
{
...
printf("Error: illegal value. Aborting.\n", tmp);
return false;
}Ошибка найдена с помощью диагностики V576: Incorrect format. A different number of actual arguments is expected while calling 'printf' function. Expected: 1. Present: 2. RemoteControlSample remotecontrolsample.cpp 792
Ошибка в том, что переменная 'tmp' никак не используется при выводе информационного сообщения.
Пример 4. Проект G3D Content Pak. Печать бессмысленных данных.
class Matrix3 {
...
inline float* operator[] (int iRow) {
...
};
void AnyVal::serialize(G3D::TextOutput& t) const {
...
const Matrix3& m = *(Matrix3*)m_value;
...
t.printf("%10.5f, %10.5f, %10.5f,\n
%10.5f, %10.5f, %10.5f,\n
%10.5f, %10.5f, %10.5f)",
m[0, 0], m[0, 1], m[0, 2],
m[1, 0], m[1, 1], m[1, 2],
m[2, 0], m[2, 1], m[2, 2]);
...
}Ошибка найдена с помощью диагностики V520: The comma operator ',' in array index expression '[0, 0]'. graphics3D anyval.cpp 275
Программа вместо матрицы распечатает бессмысленные значения. Такой код можно написать, если работать с разными языками программирования и забыть на время, как получать доступ к элементу двухмерного массива в языке Си.
Рассмотрим, как работает выражение 'm[0, 1]'. Вначале вычисляется выражение "0, 1". Результатом такого выражения является 1. Затем вызывается функция 'operator[]' в классе Matrix3. Функция принимает фактический аргумент 1 и вернет указатель на первую строку в матрице. Именно значение этого указателя и будет распечатано функцией 'printf()', хотя она ожидает значение типа float.
Корректный вариант:
t.printf("%10.5f, %10.5f, %10.5f,\n
%10.5f, %10.5f, %10.5f,\n
%10.5f, %10.5f, %10.5f)",
m[0][0], m[0][1], m[0][2],
m[1][0], m[1][1], m[1][2],
m[2][0], m[2][1], m[2][2]);Огромное количество ошибок при программировании допускается из-за опечаток. Большинство этих ошибок быстро выявляются на самых ранних этапах тестирования. Однако, некоторые такие ошибки надолго остаются в коде программы, доставляя беспокойство программистам и неудобства пользователям.
Количество таких ошибок можно существенно сократить, используя статический анализатор PVS-Studio. Анализатор найдет их еще до начала тестирования, что существенно сократит цену нахождения и устранения дефектов.
Пример 1. Проект Miranda IM. Присваивание внутри IF.
void CIcqProto::handleUserOffline(BYTE *buf, WORD wLen)
{
...
else if (wTLVType = 0x29 && wTLVLen == sizeof(DWORD))
...
}Ошибка найдена с помощью диагностики V560: A part of conditional expression is always true: 0x29. icqoscar8 fam_03buddy.cpp 632
Из-за опечатки, внутри условия оператора 'if' происходит присваивание. Корректное условие: "if (wTLVType == 0x29 && wTLVLen == sizeof(DWORD))".
Пример 2. Проект ReactOS. Ошибка присваивания.
BOOL WINAPI GetMenuItemInfoA(...)
{
...
mii->cch = mii->cch;
...
}Ошибка найдена с помощью диагностики V570: The 'mii->cch' variable is assigned to itself. user32 menu.c 4347
Значение переменной присваивается само себе. Очевидно, планировалось написать так: "mii->cch = miiW->cch;".
Пример 3. Проект Clang. Опечатка в названии объекта.
static Value *SimplifyICmpInst(...) {
...
case Instruction::Shl: {
bool NUW =
LBO->hasNoUnsignedWrap() && LBO->hasNoUnsignedWrap();
bool NSW =
LBO->hasNoSignedWrap() && RBO->hasNoSignedWrap();
...
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions 'LBO->hasNoUnsignedWrap ()' to the left and to the right of the '&&' operator. LLVMAnalysis instructionsimplify.cpp 1891
Имеется опечатка при использовании переменных с похожими именами. В первой строке надо использовать как переменную LBO, так и RBO. Исправленный вариант кода:
bool NUW = LBO->hasNoUnsignedWrap() && RBO->hasNoUnsignedWrap();Пример 4. Проект Notepad++. Неправильная проверка состояния.
bool _isPointXValid;
bool _isPointYValid;
...
bool isPointValid() {
return _isPointXValid && _isPointXValid;
};Ошибка найдена с помощью диагностики V501: There are identical sub-expressions to the left and to the right of the '&&' operator. _isPointXValid && _isPointXValid
Дважды используется имя '_isPointXValid'. На самом деле, функция должна вернуть: "_isPointXValid && _isPointYValid".
Пример 5. Проект StrongDC++. Неудачная проверка наличия \r\n.
static void getContentLengthAndHeaderLength(...)
{
...
while(line[linelen] != '\r' && line[linelen] != '\r')
...
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions 'line [linelen] != '\r'' to the left and to the right of the '&&' operator. miniupnpc miniupnpc.c 153
Из-за опечатки дважды проверяем наличие символа '\r'. На самом деле еще должно проверяться наличие символа '\n'.
Пример 6. Проект G3D Content Pak. Не там поставлена круглая закрывающаяся скобка.
bool Matrix4::operator==(const Matrix4& other) const {
if (memcmp(this, &other, sizeof(Matrix4) == 0)) {
return true;
}
...
}Ошибка найдена с помощью диагностики V575: The 'memcmp' function processes '0' elements. Inspect the 'third' argument. graphics3D matrix4.cpp 269
Одна круглая скобка закрывается не там, где необходимо. Получается, что размер сравниваемой области памяти вычисляется выражением "sizeof(Matrix4) == 0". Это выражение всегда даёт в результате значение 'false'. Затем 'false' превращается в целочисленное значение, равное 0. Корректный код:
if (memcmp(this, &other, sizeof(Matrix4)) == 0) {Пример 7. Проект QT. Ошибка копирования членов структуры.
PassRefPtr<Structure>
Structure::getterSetterTransition(Structure* structure)
{
...
transition->m_propertyStorageCapacity =
structure->m_propertyStorageCapacity;
transition->m_hasGetterSetterProperties =
transition->m_hasGetterSetterProperties;
transition->m_hasNonEnumerableProperties =
structure->m_hasNonEnumerableProperties;
transition->m_specificFunctionThrashCount =
structure->m_specificFunctionThrashCount;
...
}Ошибка найдена с помощью диагностики V570: The 'transition->m_hasGetterSetterProperties' variable is assigned to itself. QtScript structure.cpp 512
Рассматривая подобный код, очень сложно заметить ошибку. Однако, она здесь есть. Поле 'm_hasGetterSetterProperties' копируется само в себя. Корректный код должен выглядеть так:
transition->m_hasGetterSetterProperties =
structure->m_hasGetterSetterProperties;Пример 8. Проект Apache HTTP Server. Лишний оператор sizeof.
PSECURITY_ATTRIBUTES GetNullACL(void)
{
PSECURITY_ATTRIBUTES sa;
sa = (PSECURITY_ATTRIBUTES)
LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));
sa->nLength = sizeof(sizeof(SECURITY_ATTRIBUTES));
...
}Ошибка найдена с помощью диагностики V568: It's odd that the argument of sizeof() operator is the 'sizeof (SECURITY_ATTRIBUTES)' expression. libhttpd util_win32.c 115
В поле 'nLength' должен был записан размер структуры 'SECURITY_ATTRIBUTES'. В коде допущена опечатка. Оператор 'sizeof' здесь используется два раза. Как результат, в поле 'nLength' записывается размер, которыё имеет тип 'size_t'. Корректный код:
sa->nLength = sizeof(SECURITY_ATTRIBUTES);Пример 9. Проект FCE Ultra. Двойное объявление переменной.
int iNesSaveAs(char* name)
{
...
fp = fopen(name,"wb");
int x = 0;
if (!fp)
int x = 1;
...
}Ошибка найдена с помощью диагностики V561: It's probably better to assign value to 'x' variable than to declare it anew. Previous daclaration: ines.cpp, line 960. fceuxines.cpp 962
Переменная 'x' должна хранить информацию, удалось ли открыть файл или нет. Из-за опечатки, вместо присваивания переменной единицы создается и инициализируется новая переменная с именем 'x'. Корректный код, должен был быть таким:
if (!fp)
x = 1;Пример 10. Проект Notepad++. Использование оператора &&, вместо &.
TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
...
result=ToAscii(wParam,
(lParam >> 16) && 0xff, keys,&dwReturnedValue,0);
...
}Ошибка найдена с помощью диагностики V560: A part of conditional expression is always true: 0xff. notepadPlus babygrid.cpp 694
Выражение "(lParam >> 16) && 0xff" не имеет никакого практического смысла и всегда равно значению 1 (true). Здесь опечатка заключается в том, что используется оператор '&&', хотя должен был использоваться оператор '&'.
Пример 11. Проект WinDjView. Недописанное условие.
inline bool IsValidChar(int c)
{
return c == 0x9 || 0xA || c == 0xD || c >= 0x20 &&
c <= 0xD7FF || c >= 0xE000 && c <= 0xFFFD ||
c >= 0x10000 && c <= 0x10FFFF;
}Ошибка найдена с помощью диагностики V560: A part of conditional expression is always true: 0xA. WinDjView xmlparser.cpp 45 False
Функция IsValidChar всегда возвращает значение 'true'. Из-за опечатки, в одном месте пропущено сравнение: "... || 0xA || ...".
Пример 12. Проект Fennec Media Project. Лишняя точка с запятой.
int settings_default(void)
{
...
for(i=0; i<16; i++);
for(j=0; j<32; j++)
{
settings.conversion.equalizer_bands.boost[i][j] = 0.0;
settings.conversion.equalizer_bands.preamp[i] = 0.0;
}
}Ошибка найдена с помощью диагностики V529: Odd semicolon ';' after 'for' operator. settings.c 483
Про то, как опасна лишняя точка с запятой ';' знают все программисты на Си и Си++. К сожалению, это знание не мешает делать подобные опечатки. После первого оператора 'for' стоит лишняя точка с запятой, что делает этот фрагмент программы неработоспособным.
Пример 13. Проект QT. Забытый оператор break.
int QCleanlooksStyle::pixelMetric(...)
{
...
case PM_SpinBoxFrameWidth:
ret = 3;
break;
case PM_MenuBarItemSpacing:
ret = 6;
case PM_MenuBarHMargin:
ret = 0;
break;
...
}Ошибка найдена с помощью диагностики: V519: The 'ret' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3765, 3767. QtGui qcleanlooksstyle.cpp 3767
Классическая ошибка - пропущен 'break' внутри оператора 'switch'. Думаю, комментарии здесь излишни.
Пример 14. Проект Miranda IM. Присваивание вместо сравнения.
int FindItem(...)
{
...
int ret;
ret=FindItem(hwnd,dat,hItem,
(struct ClcContact ** )&z,
(struct ClcGroup ** )&isv,NULL);
if (ret=0) {return (0);}
...
}Ошибка найдена с помощью диагностики V559: Suspicious assignment inside the condition expression of 'if' operator: ret = 0. clist_mw clcidents.c 179
Опечатка находится внутри условия оператора 'if'. Вместо '==' написано просто '='. Функция некорректно обработает ситуацию, когда некий элемент не будет найден.
Пример 15. Проект IPP Samples. Некорректный индекс.
struct AVS_MB_INFO
{
...
Ipp8u refIdx[AVS_DIRECTIONS][4];
...
};
void AVSCompressor::GetRefIndiciesBSlice(void){
...
if (m_pMbInfo->predType[0] & predType)
{
m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][0];
iRefNum += 1;
}
if (m_pMbInfo->predType[1] & predType)
{
m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][1];
iRefNum += 1;
}
if (m_pMbInfo->predType[2] & predType)
{
m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][2];
iRefNum += 1;
}
if (m_pMbInfo->predType[3] & predType)
{
m_refIdx[iRefNum] = m_pMbInfo->refIdx[dir][30];
iRefNum += 1;
}
...
}Ошибка найдена с помощью диагностики V557: Array overrun is possible. The '30' index is pointing beyond array bound. avs_enc umc_avs_enc_compressor_enc_b.cpp 495
Обратите внимание вот на этот фрагмент: "m_pMbInfo->refIdx[dir][30]". Из-за опечатки вместо индекса 3 написано число 30. Кстати, этот пример хорошо показывает относительность разделения в статье ошибок по типам. Эту ошибку вполне можно отнести к разделу "Ошибки работы с массивами и строками". Деление условно и сделано, чтобы показать разнородность ошибок, которые может найти анализатор PVS-Studio.
Пример 16. Проект ReactOS. Опечатка в макросе.
#define SWAP(a,b,c) c = a;\
a = b;\
a = cОшибка найдена с помощью диагностики V519: The 'v2' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 343, 343. win32k gradient.c 343
Весьма забавная опечатка в макросе, который предназначен для обмена значений в двух переменных. Присмотритесь к коду и помёте в чем дело. Корректный вариант должен был выглядеть так:
#define SWAP(a,b,c) c = a;\
a = b;\
b = cПример 17. Проект Quake-III-Arena. Опечатка. Запятая вместо оператора умножения.
void Q1_AllocMaxBSP(void)
{
...
q1_allocatedbspmem +=
Q1_MAX_MAP_CLIPNODES * sizeof(q1_dclipnode_t);
...
q1_allocatedbspmem +=
Q1_MAX_MAP_EDGES , sizeof(q1_dedge_t);
...
q1_allocatedbspmem +=
Q1_MAX_MAP_MARKSURFACES * sizeof(unsigned short);
...
}Ошибка найдена с помощью диагностики V521: Such expressions using the ',' operator are dangerous. Make sure the expression is correct. bspc l_bsp_q1.c 136
Забавная опечатка. Посмотрите на строчку в середине показанного кода. Вместо '*' написано ','. В результате, к переменной 'q1_allocatedbspmem' всегда прибавляется значение ' sizeof(q1_dedge_t)'. Сложно предположить, как такая опечатка могла получиться.
Пример 18. Проект LibXml. Опечатка =+.
static int
xmlXPathCompOpEvalFirst(...)
{
...
total += xmlXPathCompOpEvalFirst(...);
...
total =+ xmlXPathCompOpEvalFilterFirst(ctxt, op, first);
...
}Ошибка найдена с помощью диагностики V588: The expression of the 'A =+ B' kind is utilized. Consider reviewing it, as it is possible that 'A += B' was meant. libxml xpath.c 12676
В одном месте случайно, вместо "+=" написано "=+". Выглядит похоже, а результат будет совсем разный. Такие ошибки бывает очень непросто найти, просматривая глазами неработающий код.
Очень много в программах ошибок связано именно с опечатками. Гораздо больше, чем думают программисты. Этот раздел можно продолжить и дальше. В нашей коллекции ещё много забавных примеров. Но мы нашли в себе силы всё-таки остановиться на 18 примерах.
Пример 1. Проект Fennec Media Project. Отсутствие двух терминальных нулей.
int JoiningProc(HWND hwnd,UINT uMsg,
WPARAM wParam,LPARAM lParam)
{
...
OPENFILENAME lofn;
memset(&lofn, 0, sizeof(lofn));
...
lofn.lpstrFilter = uni("All Files (*.*)\0*.*");
...
}Ошибка найдена с помощью диагностики V540: Member 'lpstrFilter' should point to string terminated by two 0 characters. base windows.c 5309
В Windows API есть структуры, в которых указатели на строки должны заканчиваться двойным нулем. Именно на такую строку и указывает член 'lpstrFilter' в структуре OPENFILENAME.
Описание 'lpstrFilter' в MSDN:
LPCTSTR
A buffer containing pairs of null-terminated filter strings. The last string in the buffer must be terminated by two NULL characters.
Если забыть написать в конце дополнительный ноль, то диалог работы с файлами в поле фильтров можем содержать мусор. Исправленный код:
lofn.lpstrFilter = uni("All Files (*.*)\0*.*\0");Пример 2. Проект TortoiseSVN. Неверное использование функции 'remove'.
STDMETHODIMP CShellExt::Initialize(....)
{
...
ignoredprops = UTF8ToWide(st.c_str());
// remove all escape chars ('\\')
std::remove(ignoredprops.begin(), ignoredprops.end(), '\\');
break;
...
}Ошибка найдена с помощью диагностики V530: The return value of function 'remove' is required to be utilized. contextmenu.cpp 442
Функция std::remove не удаляет элементы из контейнера. Она только сдвигает элементы и возвращает итератор на начало мусора. Пусть мы имеем контейнер vector<int>, содержащий элементы 1,2,3,1,2,3,1,2,3. Если выполнить код "remove( v.begin(), v.end(), 2 )", то контейнер будет содержать элементы 1,3,1,3,X,X,X, где X - некий мусор. При этом функция вернет итератор на первый мусорный элемент, и если мы хотим удалить эти мусорные элементы, то должны написать код: "v.erase(remove(v.begin(), v.end(), 2), v.end())".
Пример 3. Проект TortoiseSVN. Использование функции 'empty' вместо 'clear'.
CMailMsg& CMailMsg::SetFrom(string sAddress,
string sName)
{
if (initIfNeeded())
{
// only one sender allowed
if (m_from.size())
m_from.empty();
m_from.push_back(TStrStrPair(sAddress,sName));
}
return *this;
}Ошибка найдена с помощью диагностики V530: The return value of function 'empty' is required to be utilized. mailmsg.cpp 40
Ошибка заключается в том, что вместо функции vector::clear() случайно вызывается функция vector::empty() и содержимое массива остается неизменным. Это достаточно распространенная ошибка, так как слова 'clear' и 'empty' достаточно близки по смыслу, и их легко спутать.
Пример 4. Проект WinMerge. Использование функции 'empty' вместо 'clear'.
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();
}
else
{
...
}
}Ошибка найдена с помощью диагностики V530: The return value of function 'empty' is required to be utilized WinMerge DirActions.cpp 1307, 1308
Вновь ошибка связана с тем, что вместо clear() используется функция empty(). Примеры таких ошибок можно взять и из других проектов: InstantVNC, IPP Samples, Chromium, Intel AMT SDK и так далее. К сожалению, все эти примеры будут однообразны, и рассматривать их будет неинтересно. Но поверьте, эти дефекты встречаются в серьезных проектах, разработанных профессиональными разработчиками.
Пример 5. Проект Pixie. Использование функции 'alloca' внутри циклов.
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]);
...
}
...
}Ошибка найдена с помощью диагностики V505: The 'alloca' function is used inside the loop. This can quickly overflow stack. ri polygons.cpp 1120
Функция alloca выделяет память внутри стека, и, следовательно, ее многократный вызов в теле цикла может неожиданно привести к переполнению стека. А здесь не один цикл, а несколько вложенных. Такой код может очень быстро исчерпать всю стековую память.
Пример 6. Проект Miranda IM. Перепутанные аргументы.
static BOOL ImageArray_Alloc(LP_IMAGE_ARRAY_DATA iad, int size)
{
...
memset(&iad->nodes[iad->nodes_allocated_size],
(size_grow - iad->nodes_allocated_size) *
sizeof(IMAGE_ARRAY_DATA_NODE),
0);
...
}Ошибка найдена с помощью диагностики V575: Function receives an odd argument. clist_modern modern_image_array.cpp 59
Функция 'memset' обрабатывает 0 элементов. То есть фактически, ничего не делает. Причина в перепутанных аргументах. Корректный вызов функции memset:
memset(&iad->nodes[iad->nodes_allocated_size],
0,
(size_grow - iad->nodes_allocated_size) *
sizeof(IMAGE_ARRAY_DATA_NODE));Пример 1. Проект IPP Samples. Недописанное условие.
void lNormalizeVector_32f_P3IM(Ipp32f *vec[3],
Ipp32s* mask, Ipp32s len)
{
Ipp32s i;
Ipp32f norm;
for(i=0; i<len; i++) {
if(mask<0) continue;
norm = 1.0f/sqrt(vec[0][i]*vec[0][i]+
vec[1][i]*vec[1][i]+vec[2][i]*vec[2][i]);
vec[0][i] *= norm; vec[1][i] *= norm; vec[2][i] *= norm;
}
}Ошибка найдена с помощью диагностики V503: This is a nonsensical comparison: pointer < 0. ipprsample ippr_sample.cpp 501
Как так получилось неизвестно, но в этом коде потерялось 3 символа: "[i]". В результате, вместо проверки массива масок, происходит бессмысленная проверка, что указатель меньше нуля.
Корректная проверка должна выглядеть так: if(mask[i] < 0).
Пример 2. Проект Pc Ps2 Emulator. Некорректный switch.
LRESULT CALLBACK IOP_DISASM(...)
{
...
switch(LOWORD(wParam))
{
case (IDOK || IDCANCEL):
EndDialog(hDlg,TRUE);
return(TRUE);
break;
}
...
}Ошибка найдена с помощью диагностики V560: A part of conditional expression is always true: 2. pcsx2 debugger.cpp 321
Подобный код не имеет практического смысла. Видимо, на самом деле планировалось написать так:
switch(LOWORD(wParam))
{
case IDOK: //no break
case IDCANCEL:
EndDialog(hDlg,TRUE);
return(TRUE);
break;
}Пример 3. Проект CPU Identifying Tool. Слишком строгое условие.
void projillum(short* wtab, int xdots, int ydots, double dec)
{
...
s = sin(-dtr(dec));
x = -s * sin(th);
y = cos(th);
...
lon = (y == 0 && x == 0) ? 0.0 : rtd(atan2(y, x));
}Ошибка найдена с помощью диагностики V550: An odd precise comparison: x == 0. It's probably better to use a comparison with defined precision: fabs(A - B) '<' Epsilon. clock_dll sunalgo.cpp 155
Странно ожидать, что после сложных вычислений с использованием функций 'sin' и 'cos' результатом будет точное значение 0. Скорее всего, здесь необходимо сравнение с некоторой точностью.
Пример 4. Проект Lugaru. Двойное присваивание.
int Game::DrawGLScene(void)
{
...
radius=fast_sqrt(maxdistance);
radius=110;
...
}Ошибка найдена с помощью диагностики V519: The 'radius' object is assigned values twice successively. Perhaps this is a mistake. Lugaru gamedraw.cpp 1505
Возможно, для эксперимента, в переменную 'radius' явно записали значение 110. А потом забыли удалить эту строку. В результате, имеем бессмысленный и возможно даже ошибочный код.
Пример 5. Проект QT. Дублирующаяся проверка.
Q3TextCustomItem* Q3TextDocument::parseTable(...)
{
...
while (end < length
&& !hasPrefix(doc, length, end, QLatin1String("</td"))
&& !hasPrefix(doc, length, end, QLatin1String("<td"))
&& !hasPrefix(doc, length, end, QLatin1String("</th"))
&& !hasPrefix(doc, length, end, QLatin1String("<th"))
&& !hasPrefix(doc, length, end, QLatin1String("<td"))
&& !hasPrefix(doc, length, end, QLatin1String("</tr"))
&& !hasPrefix(doc, length, end, QLatin1String("<tr"))
&& !hasPrefix(doc, length, end, QLatin1String("</table"))) {
...
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions to the left and to the right of the '&&' operator. Qt3Support q3richtext.cpp 6978
В условии два раза проверяет наличие префикса "<td". Это не имеет смысла. Возможно, одна проверка лишняя. А возможно вместо второго "<td" должен быть написан другой прификс.
Пример 6. Проект Audacity. Странная проверка.
int sf_error (SNDFILE *sndfile)
{
...
if (!sndfile)
{
if (sf_error != 0)
return sf_errno;
return 0;
} ;
...
}Ошибка найдена с помощью диагностики V516: Consider inspecting an odd expression. Non-null function pointer is compared to null: 'sf_error != 0'. libsndfile sndfile.c 491
Проверка "sf_error != 0" всегда возвращает true, так как 'sf_error' это имя функции, в которой мы находимся.
Пример 7. Проект IPP Samples. Странный код внутри цикла.
static IppStatus mp2_HuffmanTableInitAlloc(Ipp32s *tbl, ...)
{
...
for (i = 0; i < num_tbl; i++) {
*tbl++;
}
...
}Ошибка найдена с помощью диагностики V532: Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. mpeg2_dec umc_mpeg2_dec.cpp 59
Видимо, тело цикла не дописано, так как в текущем виде оно не имеет практического смысла.
Очень большой и распространенный вид ошибок. Также эти ошибки очень сильно варьируются по уровню критичности. К неопасным ошибкам можно отнести некорректные условия в ASSERT, которые на самом деле ничего не проверяют. К опасным ошибкам относятся неправильные проверки размеров буфера или индексов.
Пример 1. Проект Shareaza. Диапазон значений типа char.
void CRemote::Output(LPCTSTR pszName)
{
...
CHAR* pBytes = new CHAR[ nBytes ];
hFile.Read( pBytes, nBytes );
...
if ( nBytes > 3 && pBytes[0] == 0xEF &&
pBytes[1] == 0xBB && pBytes[2] == 0xBF )
{
pBytes += 3;
nBytes -= 3;
bBOM = true;
}
...
}Ошибка найдена с помощью диагностики V547: Expression 'pBytes [ 0 ] == 0xEF' is always false. The value range of signed char type: [-128, 127]. Shareaza remote.cpp 350
В данном коде тип 'TCHAR' представляет собой тип 'char'. Диапазон значений char от -128 до 127 включительно. Значение 0xEF в переменной типа char это не что иное, как число -17. При сравнении переменной типа 'char' с числом 0xEF, её тип расширяется до типа 'int'. Но значение по-прежнему лежит в диапазоне [-128..127]. Условие "pBytes[0] == 0xEF" ("-17 == 0xEF") всегда ложно и программа работает не так, как задумывалось.
Корректное сравнение:
if ( nBytes > 3 && pBytes[0] == TCHAR(0xEF) &&
pBytes[1] == TCHAR(0xBB) &&
pBytes[2] == TCHAR(0xBF) )Пример 2. Проект TortoiseSVN. Диапазон значений типа char.
BOOL TortoiseBlame::OpenFile(const TCHAR *fileName)
{
...
// check each line for illegal utf8 sequences.
// If one is found, we treat
// the file as ASCII, otherwise we assume
// an UTF8 file.
char * utf8CheckBuf = lineptr;
while ((bUTF8)&&(*utf8CheckBuf))
{
if ((*utf8CheckBuf == 0xC0)||
(*utf8CheckBuf == 0xC1)||
(*utf8CheckBuf >= 0xF5))
{
bUTF8 = false;
break;
}
...
}
...
}Ошибка найдена с помощью диагностики V547: Expression '* utf8CheckBuf == 0xC0' is always false. The value range of signed char type: [-128, 127]. tortoiseblame.cpp 310
Если дефект в предыдущем примере показался случайной невнимательностью, то здесь это не так. Вот еще аналогичный пример, где условие всегда ложно. Это очень распространенный тип ошибки в разнообразнейших проектах.
Пример 3. Проект VirtualDub. Беззнаковый тип всегда >= 0.
typedef unsigned short wint_t;
...
void lexungetc(wint_t c) {
if (c < 0)
return;
g_backstack.push_back(c);
}Ошибка найдена с помощью диагностики V547: Expression 'c < 0' is always false. Unsigned type value is never < 0. Ami lexer.cpp 225
Условие "c < 0" всегда ложно, так как переменная безнакового типа всегда больше или равна 0.
Пример 4. Проект Swiss-Army Knife of Trace. Работа с сокетами.
static UINT_PTR m_socketHandle;
void TTrace::LoopMessages(void)
{
...
// Socket creation
if ( (m_socketHandle = socket(AF_INET,SOCK_STREAM,0)) < 0)
{
continue;
}
...
}Ошибка найдена с помощью диагностики V547: Expression '(m_socketHandle = socket (2, 1, 0)) < 0' is always false. Unsigned type value is never < 0. Vs8_Win_Lib tracetool.cpp 871
Неправильно проверяется, удалось ли создать сокет. Если сокет не удастся создать, то эта ситуация никак не будет обработана. Чтобы проверка работала правильно, необходимо использовать константу INVALID_SOCKET:
m_socketHandle = socket(AF_INET,SOCK_STREAM, 0);
if (m_socketHandle == INVALID_SOCKET)
...Пример 5. Проект Chromium. Работа со временем.
IdleState CalculateIdleState(...) {
...
DWORD current_idle_time = 0;
...
// Will go -ve if we have been idle for
// a long time (2gb seconds).
if (current_idle_time < 0)
current_idle_time = INT_MAX;
...
}Ошибка найдена с помощью диагностики V547: Expression 'current_idle_time < 0' is always false. Unsigned type value is never < 0. browser idle_win.cc 23
Для работы со временем используется переменная беззнакового типа. Как результат, проверка слишком больших значений не работает. Корректный вариант:
if (current_idle_time > INT_MAX)
current_idle_time = INT_MAX;Пример 6. Проект ICU. Ошибка в условии.
U_CDECL_BEGIN static const char* U_CALLCONV
_processVariableTop(...)
{
...
if(i == locElementCapacity &&
(*string != 0 || *string != '_'))
{
*status = U_BUFFER_OVERFLOW_ERROR;
}
...
}Ошибка найдена с помощью диагностики V547: Expression '*string != 0 || *string != '_'' is always true. Probably the '&&' operator should be used here. icui18n ucol_sit.cpp 242
Условие содержит логическую ошибку. Подвыражение "(*string != 0 || *string != '_')" всегда истинно. Один и тот же символ строки не может быть одновременно не равен 0 и '_'.
Пример 7. Проект QT. Опасный цикл.
bool equals( class1* val1, class2* val2 ) const{
{
...
size_t size = val1->size();
...
while ( --size >= 0 ){
if ( !comp(*itr1,*itr2) )
return false;
itr1++;
itr2++;
}
...
}Ошибка найдена с помощью диагностики V547: Expression '‑‑size >= 0' is always true. Unsigned type value is always >= 0. QtCLucene arrays.h 154
Условие (‑‑size >= 0) всегда истинно, так как переменная size имеет беззнаковый тип. Это значит, что если две сравниваемые последовательности одинаковы, то мы выйдем за их пределы. Это в свою очередь приведет к Access Violation или другим сбоям в работе программы.
Корректный вариант:
for (size_t i = 0; i != size; i++){
if ( !comp(*itr1,*itr2) )
return false;
itr1++;
itr2++;
}Пример 8. Проект MySQL. Ошибка в условии.
enum enum_mysql_timestamp_type
str_to_datetime(...)
{
...
else if (str[0] != 'a' || str[0] != 'A')
continue; /* Not AM/PM */
...
}Ошибка найдена с помощью диагностики V547: Expression 'str [0] != 'a' || str [0] != 'A'' is always true. Probably the '&&' operator should be used here. clientlib my_time.c 340
Условие всегда истинно, ведь символ всегда не равен 'a' или не равен 'A'. Корректная проверка:
else if (str[0] != 'a' && str[0] != 'A')Пример 9. Проект QT. Неправильный учёт количества ссылок.
STDMETHODIMP QEnumPins::QueryInterface(const IID &iid,void **out)
{
...
if (S_OK)
AddRef();
return hr;
}Ошибка найдена с помощью диагностики V545: Such conditional expression of 'if' operator is incorrect for the HRESULT type value '(HRESULT) 0L'. The SUCCEEDED or FAILED macro should be used instead. phonon_ds9 qbasefilter.cpp 60
Условием проверки является константа S_OK. Так как S_OK это 0, то функция AddRef() никогда не вызывается. Здесь должна была быть проверка: if (hr == S_OK).
Пример 10. Проект TickerTape. Неправильный торнадо.
void GetWindAtSingleTornado(...)
{
...
if(radius < THRESH * 5)
*yOut = THRESH * 10 / radius;
else if (radius < THRESH * 5)
*yOut = -3.0f / (THRESH * 5.0f) *
(radius - THRESH * 5.0f) + 3.0f;
else
*yOut = 0.0f;
...
}Ошибка найдена с помощью диагностики V517: The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. TickerTape wind.cpp 118
Второе условие всегда ложно. Причина в том, что первое условие совпадает со вторым. Вероятно, здесь допущена опечатка.
Пример 11. Проект Apache HTTP Server. Ошибка работы с сокетами в Windows.
typedef UINT_PTR SOCKET;
static unsigned int __stdcall win9x_accept(void * dummy)
{
SOCKET csd;
...
do {
clen = sizeof(sa_client);
csd = accept(nsd, (struct sockaddr *) &sa_client, &clen);
} while (csd < 0 && APR_STATUS_IS_EINTR(apr_get_netos_error()));
...
}Ошибка найдена с помощью диагностики V547: Expression 'csd < 0' is always false. Unsigned type value is never < 0. libhttpd child.c 404
Ошибки работы с сокетами очень часто проявляют себя в кросплатформенных программах, собранных под Windows. В Linux дескрипторы сокетов представлены знаковым типом, а в Windows беззнаковым. Про это часто забывают и проверяют статус ошибки, сравнивая значение с 0. Это неверно, и следует использовать специализированные константы.
Пример 12. Проект QT. Опечатка в сравнениях.
QStringList ProFileEvaluator::Private::values(...)
{
...
else if (ver == QSysInfo::WV_NT)
ret = QLatin1String("WinNT");
else if (ver == QSysInfo::WV_2000)
ret = QLatin1String("Win2000");
else if (ver == QSysInfo::WV_2000) <<--
ret = QLatin1String("Win2003");
else if (ver == QSysInfo::WV_XP)
ret = QLatin1String("WinXP");
...
}Ошибка найдена с помощью диагностики V517: The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2303, 2305. lrelease profileevaluator.cpp 2303
В отмеченной строке должно быть написано "ver == QSysInfo::WV_2003". Из-за этой ошибки утверждение "ret = QLatin1String("Win2003")" никогда не будет выполнено.
Конечно, ошибки уязвимости на самом деле это всё те же опечатки, неправильные условия, и некорректная работа с массивами. Но некоторые ошибки захотелось выделить в отдельную группу, так как они связаны с понятием уязвимости программ. Используя такие ошибки, злоумышленник может попытаться нарушить работоспособность программы, провести атаку с целью получения расширенных прав или выполнить некоторые нужные ему действия.
Пример 1. Проект Ultimate TCP/IP. Неправильная проверка пустой строки.
char *CUT_CramMd5::GetClientResponse(LPCSTR ServerChallenge)
{
...
if (m_szPassword != NULL)
{
...
if (m_szPassword != '\0')
{
...
}Ошибка найдена с помощью диагностики V528: It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *m_szPassword != '\0'. UTMail ut_crammd5.cpp 333
Приведенный участок кода должен проверить, что указатель на пароль не равен NULL и что строка не пустая. Но вместо этого два раза проверяет, что указатель не равен NULL. Проверка, что строка пустая не работает. Условие "if (m_szPassword != '\0')" должно было проверять, что в самом начале строки располагается терминальный ноль и значит строка пустая. Но здесь забыто разыменование указателя, и с нулем сравнивается сам указатель. Корректный код:
if (m_szPassword != NULL)
{
...
if (*m_szPassword != '\0')Пример 2. Проект Chromium. Работа с нулевым указателем.
bool ChromeFrameNPAPI::Invoke(...)
{
ChromeFrameNPAPI* plugin_instance =
ChromeFrameInstanceFromNPObject(header);
if (!plugin_instance &&
(plugin_instance->automation_client_.get()))
return false;
...
}Ошибка найдена с помощью диагностики V522: Dereferencing of the null pointer 'plugin_instance' might take place. Check the logical condition. chrome_frame_npapi chrome_frame_npapi.cc 517
Условие, проверяющее нулевой указатель, написано неверно. В результате, при работе с нулевым указателем возникнет ошибка сегментации. Корректный код должен выглядеть так:
if (plugin_instance &&
(plugin_instance->automation_client_.get()))
return false;Пример 3. Проект SMTP Client with SSL/TLS. Неполная очистка буфера.
void MD5::finalize () {
...
uint1 buffer[64];
...
// Zeroize sensitive information
memset (buffer, 0, sizeof(*buffer));
...
}Ошибка найдена с помощью диагностики V512: A call of the 'memset' function will lead to a buffer overflow or underflow. CSmtp md5.cpp 212
Функция в целях безопасности пытается очистить буфер, содержащий деликатную (sensitive) информацию. Но получается это плохо. В буфере будут очищен только первый байт. Ошибка в том, что оператор 'sizeof' вычисляет размер типа 'uint1', а не размер буфера. Корректный код:
memset (buffer, 0, sizeof(buffer));С неполноценной очисткой памяти вообще ошибки встречаются достаточно часто. Рассмотрим еще несколько подобных ситуаций.
Пример 4. Проект Chromium. Неполная очистка буфера.
void Time::Explode(..., Exploded* exploded) const {
...
ZeroMemory(exploded, sizeof(exploded));
...
}Ошибка найдена с помощью диагностики V512: A call of the 'memset' function will lead to underflow of the buffer '(exploded)'. base time_win.cc 227
Функция ZeroMemory очищает только часть структуры Exploded. Причина, что оператор 'sizeof' возвращает размер указателя. Чтобы исправить ошибку, необходимо разыменовать указатель:
ZeroMemory(exploded, sizeof(*exploded));Пример 5. Проект Apache HTTP Server. Неполная очистка буфера.
#define MEMSET_BZERO(p,l) memset((p), 0, (l))
void apr__SHA256_Final(..., SHA256_CTX* context) {
...
MEMSET_BZERO(context, sizeof(context));
...
}Ошибка найдена с помощью диагностики V512: A call of the 'memset' function will lead to underflow of the buffer '(context)'. apr sha2.c 560
Ошибка полностью аналогична рассмотренной ранее. Оператор 'sizeof' вычисляет размер указателя. Чтобы исправить ситуацию, необходимо написать: "sizeof(*context)".
Пример 6. Проект Miranda IM. Некорректная обработка строк.
static char *_skipblank(char * str)
{
char * endstr=str+strlen(str);
while ((*str==' ' || *str=='\t') && str!='\0') str++;
while ((*endstr==' ' || *endstr=='\t') &&
endstr!='\0' && endstr<str)
endstr--;
...
}Ошибка найдена с помощью диагностики: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *str != '\0'. clist_modern modern_skinbutton.cpp 282
V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: *endstr != '\0'. clist_modern modern_skinbutton.cpp 283
Это весьма опасный код, так как он неправильно пытается определить конец строки. Такой код может привести к выходу за границы строк и, как следствие, к возникновению исключения Access Violation. Ошибка кроется здесь: "str!='\0'" и здесь "endstr!='\0'". Не хватает разыменования указателя. Корректный код:
while ((*str==' ' || *str=='\t') && *str!='\0') str++;
while ((*endstr==' ' || *endstr=='\t') &&
*endstr!='\0' && endstr<str)
endstr--;Пример 7. Проект PNG library. Случайное обнуление указателя.
png_size_t
png_check_keyword(png_structp png_ptr, png_charp key,
png_charpp new_key)
{
...
if (key_len > 79)
{
png_warning(png_ptr, "keyword length must be 1 - 79 characters");
new_key[79] = '\0';
key_len = 79;
}
...
}Ошибка найдена с помощью диагностики V527: It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *new_key [79] = '\0'. graphics3D pngwutil.c 1283
Этот пример демонстрирует ошибку, где случайно вместо обрезания длины строки, обнуляется указатель. Дело в том, что new_key это указатель на строку. А это значит, чтобы обрезать её до 79 символов, код должен выглядеть так:
(*new_key)[79] = '\0';Пример 8. Проект Intel AMT SDK. Непроверенное имя пользователя.
static void
wsman_set_subscribe_options(...)
{
...
if (options->delivery_certificatethumbprint ||
options->delivery_password ||
options->delivery_password) {
...
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions 'options->delivery_password' to the left and to the right of the '||' operator. OpenWsmanLib wsman-client.c 631
Из-за невнимательности два раза проверяется наличие пароля, но не проверяется наличие имени пользователя. Корректный код:
if (options->delivery_certificatethumbprint ||
options->delivery_username ||
options->delivery_password) {Пример 9. Проект Ultimate TCP/IP. Некорректная обработка пустых строк.
void CUT_StrMethods::RemoveCRLF(LPSTR buf)
{
// v4.2 changed to size_t
size_t len, indx = 1;
if(buf != NULL){
len = strlen(buf);
while((len - indx) >= 0 && indx <= 2) {
if(buf[len - indx] == '\r' ||
buf[len - indx] == '\n')
buf[len - indx] = 0;
++indx;
}
}
}Ошибка найдена с помощью диагностики V547: Expression '(len - indx) >= 0' is always true. Unsigned type value is always >= 0. UTDns utstrlst.cpp 58
Выражение "len - indx" имеет беззнаковый тип 'size_t' и всегда >= 0. Посмотрим, к чему это приведёт, если на вход будет подана пустая строка.
Если строка пустая, то: len = 0, indx = 1.
Выражение len - indx равно значению 0xFFFFFFFFu.
Так как 0xFFFFFFFFu > 0 и indx <= 2, то выполняется доступ к массиву
"buf[len - indx]".
Операция "buf[0xFFFFFFFFu]" приведет к Access Violation.
Пример 10. Проект Miranda IM. Неработающая защита от Underflow.
void Append( PCXSTR pszSrc, 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. IRC mstring.h 229
Проверка "if (nOldLength < 0)" не работает, так как переменная nOldLength имеет беззнаковый тип.
Пример 11. Проект Apache HTTP Server. Неправильная обработка отрицательных значений.
typedef size_t apr_size_t;
APU_DECLARE(apr_status_t) apr_memcache_getp(...)
{
...
apr_size_t len = 0;
...
len = atoi(length);
...
if (len < 0) {
*new_length = 0;
*baton = NULL;
}
else {
...
}
}Ошибка найдена с помощью диагностики V547: Expression 'len < 0' is always false. Unsigned type value is never < 0. aprutil apr_memcache.c 814
Проверка "if (len < 0)" не работает, так как переменная 'len' имеет беззнаковый тип.
Пример 12. Проект Ultimate TCP/IP. Некорректное условие остановки цикла.
void CUT_StrMethods::RemoveSpaces(LPSTR szString) {
...
size_t loop, len = strlen(szString);
// Remove the trailing spaces
for(loop = (len-1); loop >= 0; loop--) {
if(szString[loop] != ' ')
break;
}
...
}Ошибка найдена с помощью диагностики V547: Expression 'loop >= 0' is always true. Unsigned type value is always >= 0. UTDns utstrlst.cpp 430
Предположим, что вся строка состоит исключительно из пробелов. Перебирая символы, программа дойдёт до нулевого элемента строки и переменная 'loop' станет равна нулю. Затем выполнится очередное уменьшение переменной 'loop'. Так как эта переменная имеет беззнаковый тип, то её значение станет равно 0xFFFFFFFFu или 0xFFFFFFFFFFFFFFFFu (в зависимости от разрядности). Естественно, что это значение >= 0 и начнется новая итерация цикла. Произойдет доступ к памяти по адресу szString[0xFFFFFFFFu], последствия чего хорошо знакомы каждому программисту на Си/Си++.
Пример 13. Проект Crypto++. Ошибка обнуления приватных данных.
void CAST256::Base::UncheckedSetKey(const byte *userKey,
unsigned int keylength, const NameValuePairs &)
{
AssertValidKeyLength(keylength);
word32 kappa[8];
...
memset(kappa, 0, sizeof(kappa));
}Ошибка найдена с помощью диагностики V597: The compiler could delete the 'memset' function call, which is used to flush 'kappa' buffer. The RtlSecureZeroMemory() function should be used to erase the private data. cryptlib cast.cpp 293
Проблема заключается в функции memset(). Аргументы, передаваемые в функцию корректны. Если программист посмотрит, как Debug-версия этого кода работает в отладчике, он тоже не заметит беды. Ошибка возникнет в Release версии проекта. Данные, которые должны быть стерты, останутся в памяти. Причина в том, что компилятор при оптимизации вправе удалить вызов функции memset() и он это делает. Почему так происходит можно прочитать в статье "Перезаписывать память - зачем?".
Программисты также зря недооценивают ошибки связанные с Copy-Paste, как и обыкновенные опечатки. Их очень и очень много. Программисты тратят на них много времени при отладке.
Опечатки и ошибки Copy-Paste, конечно, похожи, но есть отличие, из-за которого в этой статье они разделены. Опечатки часто приводят к тому, что вместо одной переменной используется другая. А при копировании, обычно просто забывают что-то поправить.
Пример 1. Проект Fennec Media Project. Промах при работе с элементами массива.
void* tag_write_setframe(char *tmem,
const char *tid, const string dstr)
{
...
if(lset)
{
fhead[11] = '\0';
fhead[12] = '\0';
fhead[13] = '\0';
fhead[13] = '\0';
}
...
}Ошибка найдена с помощью диагностики V525: The code containing the collection of similar blocks. Check items '11', '12', '13', '13' in lines 716, 717, 718, 719. id3 editor.c 716
Четыре схожие строчки, скорее всего, появились в коде программы с помощью копирования. Затем, при правке индексов допущена ошибка, из-за которой ноль записывается в 'fhead[13] ' два раза и не записывается в 'fhead[14] '.
Пример 2. Проект MySQL. Промах при работе с элементами массива.
static int rr_cmp(uchar *a,uchar *b)
{
if (a[0] != b[0])
return (int) a[0] - (int) b[0];
if (a[1] != b[1])
return (int) a[1] - (int) b[1];
if (a[2] != b[2])
return (int) a[2] - (int) b[2];
if (a[3] != b[3])
return (int) a[3] - (int) b[3];
if (a[4] != b[4])
return (int) a[4] - (int) b[4];
if (a[5] != b[5])
return (int) a[1] - (int) b[5];
if (a[6] != b[6])
return (int) a[6] - (int) b[6];
return (int) a[7] - (int) b[7];
}Ошибка найдена с помощью диагностики V525: The code containing the collection of similar blocks. Check items '0', '1', '2', '3', '4', '1', '6' in lines 680, 682, 684, 689, 691, 693, 695. sql records.cc 680
Ошибка сразу не видна, поэтому выделим её отдельно:
return (int) a[1] - (int) b[5];На самом деле, здесь должно быть написано:
return (int) a[5] - (int) b[5];Пример 3. Проект TortoiseSVN. Неисправленное имя файла.
BOOL GetImageHlpVersion(DWORD &dwMS, DWORD &dwLS)
{
return(GetInMemoryFileVersion(("DBGHELP.DLL"),
dwMS,
dwLS)) ;
}
BOOL GetDbgHelpVersion(DWORD &dwMS, DWORD &dwLS)
{
return(GetInMemoryFileVersion(("DBGHELP.DLL"),
dwMS,
dwLS)) ;
}Ошибка найдена с помощью диагностики V524: It is odd that the 'GetDbgHelpVersion' function is fully equivalent to the 'GetImageHlpVersion' function (SymbolEngine.h, line 98). symbolengine.h 105
Функция 'GetImageHlpVersion', скорее всего, получена копированием функции 'GetInMemoryFileVersion'. Ошибка в том, что в скопированной функции забыли исправить имя файл. Корректный код:
BOOL GetImageHlpVersion(DWORD &dwMS, DWORD &dwLS)
{
return(GetInMemoryFileVersion(("IMAGEHLP.DLL"),
dwMS,
dwLS)) ;
}Пример 4. Проект Clang. Одинаковые тела функций.
MapTy PerPtrTopDown;
MapTy PerPtrBottomUp;
void clearBottomUpPointers() {
PerPtrTopDown.clear();
}
void clearTopDownPointers() {
PerPtrTopDown.clear();
}Ошибка найдена с помощью диагностики V524: It is odd that the body of 'clearTopDownPointers' function is fully equivalent to the body of 'clearBottomUpPointers' function (ObjCARC.cpp, line 1318). LLVMScalarOpts objcarc.cpp 1322
Видимо, тело функции clearBottomUpPointers некорректно, и эта функция должна была выглядеть так:
void clearBottomUpPointers() {
PerPtrBottomUp.clear();
}Пример 5. Проект QT. Неудачный swap.
bool qt_testCollision(...)
{
...
t=x1; x1=x2; x2=t;
t=y1; x1=y2; y2=t;
...
}Ошибка найдена с помощью диагностики V519: The 'x1' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2218, 2219. Qt3Support q3canvas.cpp 2219
Первая строчка совершенно корректна и обменивает значения в переменных x1 и x2. Во второй строке должны быть обменены переменные y1 и y2. Эта строка видима была копией предыдущей. В ней надо было заменить все буквы 'x' на буквы 'y'. К сожалению, в одном месте это забыли сделать: "... x1=y2; ...".
Корректный код:
t=x1; x1=x2; x2=t;
t=y1; y1=y2; y2=t;Пример 6. Проект Crystal Space 3D SDK. Одинаковые подвыражения.
inline_ bool Contains(const LSS& lss)
{
return Contains(Sphere(lss.mP0, lss.mRadius)) &&
Contains(Sphere(lss.mP0, lss.mRadius));
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions to the left and to the right of the '&&' operator. plgcsopcode icelss.h 69
Ошибка в том, что два раза используется переменная 'lss.mP0.' В правой части выражения следовало использовать 'lss.mP1'.
Пример 7. Проект Notepad++. Установка некорректного стиля.
void KeyWordsStyleDialog::updateDlg()
{
...
Style & w1Style =
_pUserLang->_styleArray.getStyler(STYLE_WORD1_INDEX);
styleUpdate(w1Style, _pFgColour[0], _pBgColour[0],
IDC_KEYWORD1_FONT_COMBO, IDC_KEYWORD1_FONTSIZE_COMBO,
IDC_KEYWORD1_BOLD_CHECK, IDC_KEYWORD1_ITALIC_CHECK,
IDC_KEYWORD1_UNDERLINE_CHECK);
Style & w2Style =
_pUserLang->_styleArray.getStyler(STYLE_WORD2_INDEX);
styleUpdate(w2Style, _pFgColour[1], _pBgColour[1],
IDC_KEYWORD2_FONT_COMBO, IDC_KEYWORD2_FONTSIZE_COMBO,
IDC_KEYWORD2_BOLD_CHECK, IDC_KEYWORD2_ITALIC_CHECK,
IDC_KEYWORD2_UNDERLINE_CHECK);
Style & w3Style =
_pUserLang->_styleArray.getStyler(STYLE_WORD3_INDEX);
styleUpdate(w3Style, _pFgColour[2], _pBgColour[2],
IDC_KEYWORD3_FONT_COMBO, IDC_KEYWORD3_FONTSIZE_COMBO,
IDC_KEYWORD3_BOLD_CHECK, IDC_KEYWORD3_BOLD_CHECK,
IDC_KEYWORD3_UNDERLINE_CHECK);
Style & w4Style =
_pUserLang->_styleArray.getStyler(STYLE_WORD4_INDEX);
styleUpdate(w4Style, _pFgColour[3], _pBgColour[3],
IDC_KEYWORD4_FONT_COMBO, IDC_KEYWORD4_FONTSIZE_COMBO,
IDC_KEYWORD4_BOLD_CHECK, IDC_KEYWORD4_ITALIC_CHECK,
IDC_KEYWORD4_UNDERLINE_CHECK);
...
}Ошибка найдена с помощью диагностики V525: The code containing the collection of similar blocks. Check items '7', '7', '6', '7' in lines 576, 580, 584, 588
Глазами найти здесь ошибку практически нереально, поэтому сократим текст, чтобы выделить интересное:
styleUpdate(...
IDC_KEYWORD1_BOLD_CHECK, IDC_KEYWORD1_ITALIC_CHECK,
...);
styleUpdate(...
IDC_KEYWORD2_BOLD_CHECK, IDC_KEYWORD2_ITALIC_CHECK,
...);
styleUpdate(...
IDC_KEYWORD3_BOLD_CHECK, IDC_KEYWORD3_BOLD_CHECK, <<--
...);
styleUpdate(...
IDC_KEYWORD4_BOLD_CHECK, IDC_KEYWORD4_ITALIC_CHECK,
...);Случайно используется IDC_KEYWORD3_BOLD_CHECK вместо IDC_KEYWORD3_ITALIC_CHECK.
Пример 8. Проект ReactOS. Выбор не того объекта.
void CardButton::DrawRect(HDC hdc, RECT *rect, bool fNormal)
{
...
HPEN hhi = CreatePen(0, 0, MAKE_PALETTERGB(crHighlight));
HPEN hsh = CreatePen(0, 0, MAKE_PALETTERGB(crShadow));
...
if(fNormal)
hOld = SelectObject(hdc, hhi);
else
hOld = SelectObject(hdc, hhi);
...
}Ошибка найдена с помощью диагностики V523: The 'then' statement is equivalent to the 'else' statement. cardlib cardbutton.cpp 83
Объект 'hsh' не используется, зато 'hhi' используется два раза. Корректный код должен выглядеть так:
if(fNormal)
hOld = SelectObject(hdc, hhi);
else
hOld = SelectObject(hdc, hsh);Пример 9. Проект IPP Samples. Неправильная проверка.
Status VC1VideoDecoder::ResizeBuffer()
{
...
if(m_pContext && m_pContext->m_seqLayerHeader &&
m_pContext->m_seqLayerHeader->heightMB &&
m_pContext->m_seqLayerHeader->heightMB)
...
}Ошибка найдена с помощью диагностики V501: There are identical sub-expressions 'm_pContext->m_seqLayerHeader->heightMB' to the left and to the right of the '&&' operator. vc1_dec umc_vc1_video_decoder.cpp 1347
Правильный код:
if(m_pContext && m_pContext->m_seqLayerHeader &&
m_pContext->m_seqLayerHeader->heightMB &&
m_pContext->m_seqLayerHeader->widthMB)Пример 10. Проект ReactOS. Ошибка в имени переменной.
BOOL APIENTRY
GreStretchBltMask(...)
{
...
MaskPoint.x += DCMask->ptlDCOrig.x;
MaskPoint.y += DCMask->ptlDCOrig.x;
...
}Ошибка найдена с помощью диагностики V537: Consider reviewing the correctness of 'x' item's usage. win32k bitblt.c 670
Очень хороший пример, где видно, что строку скопировали. Потом одно имя 'x' поправили, а второе нет. Должно быть:
MaskPoint.x += DCMask->ptlDCOrig.x;
MaskPoint.y += DCMask->ptlDCOrig.y;Программисты на Си/Си++ вынуждены постоянно проверять множество указателей, чтобы убедиться, что они не равны нулю. Так как таких проверок много, то и много шансов допустить ошибку. Нередко получается так, что вначале указатель используется, а уже только затем сравнивается с NULL. Данный вид ошибок редко проявляет себя. Как правило, в стандартном режиме программа корректно функционирует. Сбой возникает в случае нестандартной ситуации. Вместо того чтобы в штатно обработать нулевой указатель, произойдёт Access Violation и будет сгенерировано исключение.
Пример 1. Проект Quake-III-Arena. Запоздалая проверка.
void Item_Paint(itemDef_t *item) {
vec4_t red;
menuDef_t *parent = (menuDef_t*)item->parent;
red[0] = red[3] = 1;
red[1] = red[2] = 0;
if (item == NULL) {
return;
}
...
}Ошибка найдена с помощью диагностики V595: The 'item' pointer was utilized before it was verified against nullptr. Check lines: 3865, 3869. cgame ui_shared.c 3865
Вначале указатель 'item' используется, и только потом сравнивается с NULL.
Пример 2. Проект LAME Ain't an MP3 Encoder. Запоздалая проверка.
static int
check_vbr_header(PMPSTR mp, int bytes)
{
...
buf = buf->next;
pos = buf->pos;
if(!buf) return -1; /* fatal error */
...
}Ошибка найдена с помощью диагностики V595: The 'buf' pointer was utilized before it was verified against nullptr. Check lines: 226, 227. mpglib interface.c 226
Если 'buf' равен NULL, то вместо возврата кода ошибки, будет сгенерировано исключение. А если исключения не используются, то программа завершится в аварийном режиме.
Пример 3. Проект daoParanoia library. Запоздалая проверка.
static long i_stage2_each(root_block *root,
v_fragment *v, void(*callback)(long,int))
{
cdrom_paranoia *p=v->p;
long dynoverlap=p->dynoverlap/2*2;
if (!v || !v->one) return(0);
...
}Ошибка найдена с помощью диагностики V595: The 'v' pointer was utilized before it was verified against nullptr. Check lines: 532, 535. daoParanoia paranoia.c 532
Ситуация аналогична ошибкам, рассмотренным ранее.
Пример 4. Проект TrinityCore. Запоздалая проверка.
bool OnCheck(Player* player, Unit* /*target*/)
{
bool checkArea =
player->GetAreaId() == AREA_ARGENT_TOURNAMENT_FIELDS ||
player->GetAreaId() == AREA_RING_OF_ASPIRANTS ||
player->GetAreaId() == AREA_RING_OF_ARGENT_VALIANTS ||
player->GetAreaId() == AREA_RING_OF_ALLIANCE_VALIANTS ||
player->GetAreaId() == AREA_RING_OF_HORDE_VALIANTS ||
player->GetAreaId() == AREA_RING_OF_CHAMPIONS;
return player && checkArea && player->duel &&
player->duel->isMounted;
}Ошибка найдена с помощью диагностики V595: The 'player' pointer was utilized before it was verified against nullptr. Check lines: 310, 312. scripts achievement_scripts.cpp 310
Из условия "player && ..." видно, что указатель 'player' может быть равен нулю. Однако, как и во всех предыдущих примерах, эта проверка запоздала.
Можно привести ещё много примеров подобных ошибок. Но они все однотипны. Если вы видели пару таких ошибок, то можете считать, что видели все.
Пример 1. Проект Image Processing SDK. Восьмеричное число.
inline
void elxLuminocity(const PixelRGBus& iPixel,
LuminanceCell< PixelRGBus >& oCell)
{
oCell._luminance = uint16(0.2220f*iPixel._red +
0.7067f*iPixel._blue + 0.0713f*iPixel._green);
oCell._pixel = iPixel;
}
inline
void elxLuminocity(const PixelRGBi& iPixel,
LuminanceCell< PixelRGBi >& oCell)
{
oCell._luminance = 2220*iPixel._red +
7067*iPixel._blue + 0713*iPixel._green;
oCell._pixel = iPixel;
}Ошибка найдена с помощью диагностики V536: Be advised that the utilized constant value is represented by an octal form. Oct: 0713, Dec: 459. IFF plugins pixelservices.inl 146
Если рассмотреть вторую функцию, то станет ясно, что хотелось использовать число 713, а вовсе на 0713. Число 0713 задано в восьмеричной системе счисления. Об этом легко забыть, если редко использовать восьмеричные константы.
Пример 2. Проект IPP Samples. Одна переменная для двух циклов.
JERRCODE CJPEGDecoder::DecodeScanBaselineNI(void)
{
...
for(c = 0; c < m_scan_ncomps; c++)
{
block = m_block_buffer + (DCTSIZE2*m_nblock*(j+(i*m_numxMCU)));
// skip any relevant components
for(c = 0; c < m_ccomp[m_curr_comp_no].m_comp_no; c++)
{
block += (DCTSIZE2*m_ccomp[c].m_nblocks);
}
...
}Ошибка найдена с помощью диагностики V535: The variable 'c' is being used for this loop and for the outer loop. jpegcodec jpegdec.cpp 4652
Одна и та же переменная используется для организации внешнего и внутреннего цикла. В результате, такой код обработает только часть данных или бесконечно зациклится.
Пример 3. Проект Quake-III-Arena. Забытый return.
static ID_INLINE int BigLong(int l)
{ LongSwap(l); }Ошибка найдена с помощью диагностики V591: Non-void function should return a value. botlib q_shared.h 155
Этот код на языке Си. А значит, компилятор не требует обязательного наличия return. Однако здесь он необходим. Впрочем, код благодаря удачному стечению обстоятельств может даже работать. Всё зависит от того, что будет содержаться в регистре EAX. Однако это только везение и не более. Тело функции должно было выглядеть так: { return LongSwap(l); }.
Пример 4. Проект Notepad++. Странное условие.
int Notepad_plus::getHtmlXmlEncoding(....) const
{
...
if (langT != L_XML && langT != L_HTML && langT == L_PHP)
return -1;
...
}Ошибка найдена с помощью диагностики V590: Consider inspecting this expression. The expression is excessive or contains a misprint. Notepad++ notepad_plus.cpp 853
Возможно, эта ошибка является простой опечаткой, а возможно она возникла в процессе факторинга. Тем не менее, ошибка очевидна. Условие можно упростить: if (langT == L_PHP). Это значит, что, скорее всего, код должен был выглядеть так:
if (langT != L_XML && langT != L_HTML && langT != L_PHP)