Toп 10 ошибок в C++ проектах за 2017 год
За окном уже почти как 3 месяца стоит 2018 год, а это значит, что пришло время (пусть и немного запоздало) составить топ 10 ошибок, найденных анализатором PVS-Studio в C++ проектах за прошедший год. Итак, начнём!
Примечание. Для большего интереса я рекомендую вам сначала попробовать самостоятельно найти ошибки в приведённых фрагментах кода, и только после этого читать предупреждение анализатора и пояснения. Думаю, так будет намного интереснее.
Десятое место
Источник: Notepad++: проверка кода пять лет спустя
Ошибка была обнаружена при проверке одного из самых известных текстовых редакторов — Notepad++.
Фрагмент кода, содержащий ошибку:
TCHAR GetASCII(WPARAM wParam, LPARAM lParam)
{
int returnvalue;
TCHAR mbuffer[100];
int result;
BYTE keys[256];
WORD dwReturnedValue;
GetKeyboardState(keys);
result = ToAscii(static_cast(wParam),
(lParam >> 16) && 0xff, keys, &dwReturnedValue, 0);
returnvalue = (TCHAR) dwReturnedValue;
if(returnvalue < 0){returnvalue = 0;}
wsprintf(mbuffer, TEXT("return value = %d"), returnvalue);
if(result!=1){returnvalue = 0;}
return (TCHAR)returnvalue;
}
Предупреждение PVS-Studio: V560 A part of conditional expression is always true: 0xff. babygrid.cpp 711
Анализатор счёл подозрительным выражение (lParam >> 16) && 0xff. Второй аргумент, передаваемый в функцию ToAscii, всегда будет иметь значение 0 или 1, причём результирующее значение зависит только от левого подвыражения — (lParam >> 16). Очевидно, что вместо оператора && должен был использоваться оператор &.
Девятое место
Источник: Передаю привет разработчикам компании Yandex
На девятом месте расположилась ошибка из проекта ClickHouse, разрабатываемого компанией Yandex.
bool executeForNullThenElse(....)
{
....
const ColumnUInt8 * cond_col =
typeid_cast(arg_cond.column.get());
....
if (cond_col)
{
....
}
else if (cond_const_col)
{
....
}
else
throw Exception(
"Illegal column " + cond_col->getName() +
" of first argument of function " + getName() +
". Must be ColumnUInt8 or ColumnConstUInt8.",
ErrorCodes::ILLEGAL_COLUMN);
....
}
Предупреждение PVS-Studio: V522 Dereferencing of the null pointer 'cond_col' might take place. FunctionsConditional.h 765
В данном коде неправильно обрабатывается ошибочная ситуация, когда необходимо сгенерировать исключение. Обратите внимание на указатель cond_col. Для него в операторе if выполняется проверка, что указатель — ненулевой. Если управление дошло до else ветви, где генерируется исключение, указатель cond_col точно нулевой. Однако при формировании сообщения исключения, cond_col разыменовывается в выражении cond_col→getName ().
Восьмое место
Источник: Сравнение качества кода Firebird, MySQL и PostgreSQL
На восьмом месте расположилась одна из ошибок, найденных в проекте MySQL во время сравнения качества кода Firebird, MySQL и PostgreSQL.
Код метода, содержащего ошибку:
mysqlx::XProtocol* active()
{
if (!active_connection)
std::runtime_error("no active session");
return active_connection.get();
}
Предупреждение PVS-Studio: V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error (FOO); mysqlxtest.cc 509
В случае отсутствия активного соединения (! active_connection) создаётся объект исключения типа std: runtime_error и… на этом всё. После создания он будет попросту удалён, при этом продолжится выполнение метода. Очевидно, что разработчик забыл ключевое слово throw, чтобы сгенерировать исключение.
Седьмое место
Источник: Как найти 56 потенциальных уязвимостей в коде FreeBSD за один вечер
Как найти 56 потенциальных уязвимостей за вечер? С помощью статического анализа, конечно же!
Одна из проблем, обнаруженных в коде FreeBSD:
int mlx5_core_create_qp(struct mlx5_core_dev *dev,
struct mlx5_core_qp *qp,
struct mlx5_create_qp_mbox_in *in,
int inlen)
{
....
struct mlx5_destroy_qp_mbox_out dout;
....
err_cmd:
memset(&din, 0, sizeof(din));
memset(&dout, 0, sizeof(dout));
din.hdr.opcode = cpu_to_be16(MLX5_CMD_OP_DESTROY_QP);
din.qpn = cpu_to_be32(qp->qpn);
mlx5_cmd_exec(dev, &din, sizeof(din), &out, sizeof(dout));
return err;
}
Предупреждение PVS-Studio: V597 The compiler could delete the 'memset' function call, which is used to flush 'dout' object. The memset_s () function should be used to erase the private data. mlx5_qp.c 159
Обратите внимание на выражение memset (&dout, 0, sizeof (dout)). Разработчик хотел «затереть» данные в блоке памяти, соответствующем dout, выставив нулевые значения. Обычно такой подход используется, когда необходимо очистить какие-то приватные данные, чтобы они не «висели» в памяти.
Однако дальше dout не используется (sizeof (dout) не в счёт), что позволит компилятору удалить приведённый выше вызов функции memset, т.к. подобная оптимизация не влияет на поведение программы с точки зрения C/C++. Как следствие — данные, которые должны были быть зачищены, могут так и остаться в памяти.
Чтобы глубже погрузиться в тему, я рекомендую для прочтения следующие статьи:
- Безопасная очистка приватных данных.
- Документация к диагностическому правилу V597.
- Самая опасная функция в мире С/С++.
Шестое место
Источник: Долгожданная проверка CryEngine V
Первый игровой движок, к коду которого мы обратимся в этом топе — CryEngine V.
int CTriMesh::Slice(....)
{
....
bop_meshupdate *pmd = new bop_meshupdate, *pmd0;
pmd->pMesh[0]=pmd->pMesh[1] = this; AddRef();AddRef();
for(pmd0=m_pMeshUpdate; pmd0->next; pmd0=pmd0->next);
pmd0->next = pmd;
....
}
Предупреждение PVS-Studio: V529 Odd semicolon ';' after 'for' operator. boolean3d.cpp 1314
Согласитесь, что если бы этот фрагмент не был выписан вот так вот — будучи сокращённым и отделённым от остального кода, не так легко было бы обнаружить в нём тот подозрительный участок, который нашёл анализатор — символ ';', завершающий цикл for. При этом форматирование кода (сдвиг следующего выражения) также наводит на мысли о том, что символ ';' тут лишний, а выражение pmd0→next = pmd; должно быть телом цикла.
Пятое место
Источник: Статический анализ как часть процесса разработки Unreal Engine
Следующая ошибка была обнаружена в ходе работы над исправлением ошибок, найденных PVS-Studio в коде игрового движка Unreal Engine.
for(int i = 0; i < SelectedObjects.Num(); ++i)
{
UObject* Obj = SelectedObjects[0].Get();
EdObj = Cast(Obj);
if(EdObj)
{
break;
}
}
Предупреждение PVS-Studio: V767 Suspicious access to element of 'SelectedObjects' array by a constant index inside a loop. skeletonnotifydetails.cpp 38
В цикле хотели обойти все элементы и найти среди них первый, имеющий тип UEditorSkeletonNotifyObj. Но допустили досадную опечатку, использовав в выражении SelectedObjects[0].Get () константный индекс 0 вместо счётчика цикла i. Как результат — всегда будет проверяться только первый элемент.
Четвёртое место
Источник: 27000 ошибок в операционной системе Tizen
Ошибка была найдена при проверке операционной системы Tizen, а также сторонних компонентов, используемых в ней. Статья большая, и в ней выписано много интересных примеров ошибок — настоятельно рекомендую ознакомиться.
Но вернёмся к конкретному предупреждению:
int _read_request_body(http_transaction_h http_transaction,
char **body)
{
....
*body = realloc(*body, new_len + 1);
....
memcpy(*body + curr_len, ptr, body_size);
body[new_len] = '\0';
curr_len = new_len;
....
}
Предупреждение PVS-Studio: V527 It is odd that the '\0' value is assigned to 'char' type pointer. Probably meant: *body[new_len] = '\0'. http_request.c 370
Ошибка кроется в выражении body[new_len] = '\0'. Обратите внимание, что параметр body имеет тип char**, соответственно, тип выражения body[new_len] — char*. Но разработчик дал маху, и, забыв ещё одно разыменование, попытался записать в указатель значение '\0' (будет преобразовано в нулевой указатель).
Это приводит к двум проблемам:
- Куда-то (body[new_len]) будет записан нулевой указатель.
- Терминальный ноль не будет записан в конец строки.
Корректный код:
(*body)[new_len] = '\0';
Третье место
Источник: Как PVS-Studio может помочь в поиске уязвимостей?
Вот мы и подобрались к тройке лидеров. Приведённый ниже код был обнаружен в ходе поиска ответа на вопрос: «Как PVS-Studio справляется с поиском CVE?» (ответ смотрите в указанной выше статье). Код из проекта illumos-gate.
static int devzvol_readdir(....)
{
....
char *ptr;
....
ptr = strchr(ptr + 1, '/') + 1;
rw_exit(&sdvp->sdev_contents);
sdev_iter_datasets(dvp, ZFS_IOC_DATASET_LIST_NEXT, ptr);
....
}
Предупреждение PVS-Studio: V769 The 'strchr (ptr + 1, '/')' pointer in the 'strchr (ptr + 1, '/') + 1' expression could be nullptr. In such case, resulting value will be senseless and it should not be used.
Функция strchr возвращает указатель на первое вхождение символа, заданного вторым аргументом, в строке, заданной первым аргументом. Если же такого символа не находится, strchr возвращает NULL. Однако этот факт не учитывается, и к возвращаемому значению всегда прибавляется значение '1'. Как результат, указатель ptr всегда будет ненулевым, а значит, дальнейшие проверки вида ptr!= NULL не дадут информации о валидности указателя. В итоге, при определённых условиях данный код приводил к возникновению kernel panic.
Данной ошибке был присвоен идентификатор CVE-2014–9491: The devzvol_readdir function in illumos does not check the return value of a strchr call, which allows remote attackers to cause a denial of service (NULL pointer dereference and panic) via unspecified vectors.
Несмотря на то, что сама CVE была обнаружена в 2014 году, в ходе собственных исследований мы обнаружили эту ошибку в 2017, поэтому она попала в этот топ.
Второе место
Источник: Статический анализ как часть процесса разработки Unreal Engine
Ошибка, расположившаяся на втором месте, была обнаружена… да, опять в Unreal Engine. Мне она показалась очень интересной, так что я не смог удержаться и не выписать её.
Примечание. На самом деле, я бы выписал ещё парочку ошибок из приведённой выше статьи про Unreal Engine, но всё же не хочется так часто обращаться к одному и тому же проекту. Поэтому я настоятельно рекомендую посмотреть упомянутую выше статью самостоятельно, в частности — предупреждения V714 и V709.
Дальше будет много кода, но он необходим, чтобы понять суть проблемы.
bool FCreateBPTemplateProjectAutomationTests::RunTest(
const FString& Parameters)
{
TSharedPtr NewProjectWizard;
NewProjectWizard = SNew(SNewProjectWizard);
TMap> >& Templates =
NewProjectWizard->FindTemplateProjects();
int32 OutMatchedProjectsDesk = 0;
int32 OutCreatedProjectsDesk = 0;
GameProjectAutomationUtils::CreateProjectSet(Templates,
EHardwareClass::Desktop,
EGraphicsPreset::Maximum,
EContentSourceCategory::BlueprintFeature,
false,
OutMatchedProjectsDesk,
OutCreatedProjectsDesk);
int32 OutMatchedProjectsMob = 0;
int32 OutCreatedProjectsMob = 0;
GameProjectAutomationUtils::CreateProjectSet(Templates,
EHardwareClass::Mobile,
EGraphicsPreset::Maximum,
EContentSourceCategory::BlueprintFeature,
false,
OutMatchedProjectsMob,
OutCreatedProjectsMob);
return ( OutMatchedProjectsDesk == OutCreatedProjectsDesk ) &&
( OutMatchedProjectsMob == OutCreatedProjectsMob );
}
Отметим следующий важный момент, необходимый для понимания проблемы. Переменные OutMatchedProjectsDesk, OutCreatedProjectsDesk и OutMatchedProjectsMob, OutCreatedProjectsMob при объявлении инициализируются нулями, после чего передаются в качестве аргументов методу CreateProjectSet.
После этого переменные сравниваются в выражении оператора return. Следовательно, метод CreateProjectSet должен выполнять инициализацию последних двух аргументов.
Теперь же обратимся к методу CreateProjectSet, который и содержит ошибки.
static void CreateProjectSet(.... int32 OutCreatedProjects,
int32 OutMatchedProjects)
{
....
OutCreatedProjects = 0;
OutMatchedProjects = 0;
....
OutMatchedProjects++;
....
OutCreatedProjects++;
....
}
Предупреждения PVS-Studio:
- V763 Parameter 'OutCreatedProjects' is always rewritten in function body before being used. gameprojectautomationtests.cpp 88
- V763 Parameter 'OutMatchedProjects' is always rewritten in function body before being used. gameprojectautomationtests.cpp 89
Параметры OutCreatedProjects и OutMatchedProjects забыли сделать ссылками, и в итоге значения соответствующих аргументов просто копируются. Как результат — возвращаемое значение метода RunTest, приведённого выше, всегда true, так как все сравниваемые переменные имеют одно и то же значение, заданное при инициализации — 0.
Правильный вариант кода:
static void CreateProjectSet(.... int32 &OutCreatedProjects,
int32 &OutMatchedProjects)
Первое место
Источник: Любите статический анализ кода!
Как только я увидел эту ошибку, у меня не осталось ни единого сомнения по поводу того, кто должен возглавить топ. В общем, смотрите сами. Ни за что не переходите к описанию проблемы, пока не найдёте ошибку в приведённом фрагменте кода. Кстати, проект — StarEngine — опять является игровым движком.
PUGI__FN bool set_value_convert(
char_t*& dest,
uintptr_t& header,
uintptr_t header_mask,
int value)
{
char buf[128];
sprintf(buf, "%d", value);
return set_value_buffer(dest, header, header_mask, buf);
}
Ну что, как успехи с поиском ошибки? :)
Предупреждение PVS-Studio: V614 Uninitialized buffer 'buf' used. Consider checking the first actual argument of the 'printf' function. pugixml.cpp 3362
Наверняка у вас возник вопрос:»printf? Откуда в предупреждении анализатора printf, если в коде есть вызов только функции sprintf?»
В этом-то и суть! sprintf — это макрос, который раскрывается в (!) std: printf!
#define sprintf std::printf
В итоге неинициализированный буфер buf используется как строка форматирования. Здорово, не так ли? Я думаю, эта ошибка вполне себе заслуженно заняла первое место.
Заключение
Надеюсь, вам понравились собранные ошибки. Лично мне они показались достаточно интересными. Но, конечно, ваше видение может отличаться от моего, поэтому вы можете составить свой «Tоп 10…», почитав статьи из нашего блога или посмотрев список ошибок, найденных PVS-Studio в open source проектах.
Также напоминаю, что все приведённые в статье ошибки (а также множество других) были найдены с помощью анализатора PVS-Studio, который советую попробовать и на вашем проекте: ссылка на страницу загрузки.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Vasiliev. Top 10 Bugs in the C++ Projects of 2017
