Главная » Хабрахабр » Авторы игры 0 A.D. — молодцы

Авторы игры 0 A.D. — молодцы

PVS-Studio & 0 A.D.

0 A.D. — это трёхмерная игра в жанре исторической стратегии в реальном времени, разрабатываемая сообществом добровольцев. Размер кодовой базы маленький и я решил проверить игру в качестве отдыха от больших проектов, таких как Android и XNU Kernel. Итак, перед нами проект, содержащий 165000 строк кода на языке C++. Посмотрим, что интересного в нём можно найти с помощью статического анализатора PVS-Studio.

0 A.D.

0 A.D. (0 год н. э.) — свободная трёхмерная игра в жанре исторической стратегии в реальном времени, разрабатываемая сообществом добровольцев (основные разработчики объединены в команду Wildfire Games). Игра позволяет управлять цивилизациями, существовавшими в период 500 год до н. э.—1 год до н. э. По состоянию на лето 2018 года, проект находится в альфа-версии. [Описание взято из Wikipedia].

D.? Почему именно 0 A.

Он прислал мне лог для проекта 0 A. Я попросил коллегу Егора Бредихина (EgorBredikhin) выбрать и проверить для меня какой-нибудь небольшой открытый проект, с которым я бы мог позаниматься в перерывах между другими задачами. На вопрос: «Почему именно этот проект?» — был ответ: «Да просто играл в эту игру, неплохая стратегия в реальном времени». D. D. Ok, значит будет 0 A. :).

Плотность ошибок

Сразу хочу похвалить авторов 0 A.D. за хорошее качество С++ кода. Молодцы, редко удаётся встретить такую низкую плотность ошибок. Имеются в виду, конечно, не все ошибки, а те, которые можно обнаружить с помощью PVS-Studio. Как я уже сказал, хотя PVS-Studio находит не все ошибки, тем не менее, можно смело говорить о взаимосвязи между плотностью ошибок и качеством кода в целом.

Общее количество непустых строк кода 231270. Немного чисел. 7% являются комментариями. Из них 28. Итого, 165000 строк чистого кода на языке C++.

Все эти ошибки я рассмотрю далее в статье. Количество сообщений, выданных анализатором, было небольшим, и просмотрев их все, я выписал 19 ошибок. Однако в целом это картины не меняет. Возможно, я что-то пропустил, посчитав ошибку безобидным неаккуратным кодом.

Вычислим плотность ошибок: 19*1000/165000 = 0. Итак, я нашел 19 ошибок на 165000 строк кода. 115.

1 ошибок на 1000 строк кода. Для простоты округлим и будем считать, что анализатор PVS-Studio обнаруживает в коде игры 0.

Для сравнения, в моей недавней статье про Android я посчитал, что я обнаруживал не менее 0. Отличный результат! На самом деле, плотность ошибок там даже больше, просто я не нашел сил проанализировать внимательно весь отчёт. 25 ошибок на 1000 строк кода.

В ней PVS-Studio обнаруживает 0. Или возьмём для примера библиотеку EFL Core Libraries, которую я тщательно изучил и посчитал количество дефектов. 71 ошибок на 1000 строк кода.

D. Итак, авторы 0 A. К сожалению, чем больше проект, тем быстрее растёт его сложность, и плотность ошибок увеличивается нелинейно (подробнее). — молодцы, однако, ради справедливости следует отметить, что им помогает малый объём кода, написанного на C++.

Ошибки

Давайте теперь посмотрим на 19 ошибок, найденных мною в игре. Для анализа проекта я использовал PVS-Studio версии 6.24. Предлагаю попробовать скачать демонстрационную версию и проверить проекты, над которыми вы работаете.

Для маленьких проектов и индивидуальных разработчиков у нас есть вариант бесплатной лицензии: "Как использовать PVS-Studio бесплатно". Примечание. Мы позиционируем PVS-Studio как B2B решение.

Ошибка N1

На самом деле, она не сложная, но придётся ознакомиться с достаточно большим фрагментом кода.
Начнём с рассмотрения сложной ошибки.

void WaterManager::CreateWaveMeshes()
++nbNeighb; // We've found a new point around us. // Move there xx = xx + around[p][0]; yy = yy + around[p][1]; indexx = xx + yy*SideSize; if (i == 0) Chain.push_back(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); else Chain.push_front(CoastalPoint(indexx,CVector2D(xx*2,yy*2))); CoastalPointsSet.erase(xx + yy*SideSize); found = true; break; } } if (!found) endedChain = true; ....
}

Предупреждение PVS-Studio: V547 CWE-570 Expression 'nbNeighb >= 2' is always false. WaterManager.cpp 581

Почему условие nbNeighb >= 2 всегда ложное? На первый взгляд сообщение анализатора кажется странным. Ведь в теле цикла есть инкремент переменной nbNeighb!

Поэтому, если переменная nbNeighb будет увеличена, то цикл будет остановлен. Посмотрите ниже и вы увидите оператор break, который прерывает выполнение цикла. Таким образом, значение переменной nbNeighb никогда не достигнет значения больше 1.

Код явно содержит какую-то логическую ошибку.

Ошибка N2

void
CmpRallyPointRenderer::MergeVisibilitySegments( std::deque<SVisibilitySegment>& segments)
{ .... segments.erase(segments.end()); ....
}

Предупреждение PVS-Studio: V783 CWE-119 Dereferencing of the invalid iterator 'segments.end()' might take place. CCmpRallyPointRenderer.cpp 1290

Возможно, программист хотел удалить из контейнера последний элемент. Очень, очень странный код. В этом случае код должен быть таким:

segments.erase(segments.back());

Хотя, тогда было бы проще написать:

segments.pop_back();

Если честно, мне не совсем понятно, что именно здесь должно было быть написано.

Ошибка N3, N4

Решил рассмотреть совместно две ошибки, так как они связаны с утечкой ресурсов и требуют сначала показать, что такое макрос WARN_RETURN.

#define WARN_RETURN(status)\ do\ {\ DEBUG_WARN_ERR(status);\ return status;\ }\ while(0)

Итак, как видите, макрос WARN_RETURN приводит к выходу из тела функции. Теперь посмотрим на неаккуратные способы использования этого макроса.

Первый фрагмент.

Status sys_generate_random_bytes(u8* buf, size_t count)
{ FILE* f = fopen("/dev/urandom", "rb"); if (!f) WARN_RETURN(ERR::FAIL); while (count) { size_t numread = fread(buf, 1, count, f); if (numread == 0) WARN_RETURN(ERR::FAIL); buf += numread; count -= numread; } fclose(f); return INFO::OK;
}

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'f' handle. A resource leak is possible. unix.cpp 332

На практике такое навряд ли возможно. Если функция fread не сможет прочитать данные, то функция sys_generate_random_bytes завершит свою работу без освобождения файлового дескриптора. Тем не менее, код неаккуратный. Сомнительно, что не удастся прочитать данные из "/dev/urandom".

Второй фрагмент.

Status sys_cursor_create(....)
{ .... sys_cursor_impl* impl = new sys_cursor_impl; impl->image = image; impl->cursor = XcursorImageLoadCursor(wminfo.info.x11.display, image); if(impl->cursor == None) WARN_RETURN(ERR::FAIL); *cursor = static_cast<sys_cursor>(impl); return INFO::OK;
}

Предупреждение PVS-Studio: V773 CWE-401 The function was exited without releasing the 'impl' pointer. A memory leak is possible. x.cpp 421

Если не удаётся загрузить курсор, то происходит утечка памяти.

Ошибка N5

Status LoadHeightmapImageOs(....)
{ .... shared_ptr<u8> fileData = shared_ptr<u8>(new u8[fileSize]); ....
}

Предупреждение PVS-Studio: V554 CWE-762 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. MapIO.cpp 54

Правильный вариант:

shared_ptr<u8[]> fileData = shared_ptr<u8>(new u8[fileSize]);

Ошибка N6

FUTrackedPtr(ObjectClass* _ptr = NULL) : ptr(_ptr)
{ if (ptr != NULL) FUTracker::TrackObject((FUTrackable*) ptr); ptr = ptr;
}

Предупреждение PVS-Studio: V570 The 'ptr' variable is assigned to itself. FUTracker.h 122

Ошибка N7, N8

std::wstring TraceEntry::EncodeAsText() const
{ const wchar_t action = (wchar_t)m_action; wchar_t buf[1000]; swprintf_s(buf, ARRAY_SIZE(buf), L"%#010f: %c \"%ls\" %lu\n", m_timestamp, action, m_pathname.string().c_str(), (unsigned long)m_size); return buf;
}

Предупреждение PVS-Studio: V576 CWE-628 Incorrect format. Consider checking the fifth actual argument of the 'swprintf_s' function. The char type argument is expected. trace.cpp 93

Пересказывать её не буду, а отсылаю к документации по диагностике V576 (см. Здесь мы сталкиваемся с запутанной и невнятной историей альтернативной реализации функции swprintf в Visual C++. раздел «Широкие строки»).

В данном случае, скорее всего, этот код будет корректно работать при компиляции в Visual C++ для Windows и некорректно при сборке для Linux или macOS.

Consider checking the fourth actual argument of the 'swprintf_s' function. Аналогичная ошибка: V576 CWE-628 Incorrect format. vfs_tree.cpp 211 The char type argument is expected.

Ошибка N9, N10, N11

В начале указатель используется, а только потом проверяется.
Классика.

static void TEST_CAT2(char* dst, ....)
{ strcpy(dst, dst_val); // <= int ret = strcat_s(dst, max_dst_chars, src); TS_ASSERT_EQUALS(ret, expected_ret); if(dst != 0) // <= TS_ASSERT(!strcmp(dst, expected_dst));
}

Предупреждение PVS-Studio: V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 140, 143. test_secure_crt.h 140

Аналогичные предупреждения: Думаю, ошибка не требует пояснения.

  • V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 150, 153. test_secure_crt.h 150
  • V595 CWE-476 The 'dst' pointer was utilized before it was verified against nullptr. Check lines: 314, 317. test_secure_crt.h 314

Ошибка N12

typedef int tbool; void MikkTSpace::setTSpace(...., const tbool bIsOrientationPreserving, ....)
{ .... m_NewVertices.push_back(bIsOrientationPreserving > 0.5 ? 1.0f : (-1.0f)); ....
}

V674 CWE-682 The '0.5' literal of the 'double' type is compared to a value of the 'int' type. Consider inspecting the 'bIsOrientationPreserving > 0.5' expression. MikktspaceWrap.cpp 137

5. Нет смысла сравнивать переменную типа int с константой 0. 5 выглядит совсем странно. Более того, по смыслу это вообще переменная типа boolean, а значит сравнение её с 0. Предположу, что вместо bIsOrientationPreserving здесь должна использоваться другая переменная.

Ошибка N13

virtual Status ReplaceFile(const VfsPath& pathname, const shared_ptr<u8>& fileContents, size_t size)
{ ScopedLock s; VfsDirectory* directory; VfsFile* file; Status st; st = vfs_Lookup(pathname, &m_rootDirectory, directory, &file, VFS_LOOKUP_ADD|VFS_LOOKUP_CREATE); // There is no such file, create it. if (st == ERR::VFS_FILE_NOT_FOUND) { s.~ScopedLock(); return CreateFile(pathname, fileContents, size); } ....
}

Предупреждение PVS-Studio: V749 CWE-675 Destructor of the 's' object will be invoked a second time after leaving the object's scope. vfs.cpp 165

Для этого явно вызывают деструктор. До того, как создавать файл, нужно, чтобы объект типа ScopedLock «разлочил» некий объект. Т.е. Беда в том, что деструктор для объекта s будет вызван автоматически ещё раз при выходе из функции. Я не изучал устройство класса ScopedLock, но в любом случае так делать не стоит. деструктор будет вызван дважды. Даже если сейчас код работает нормально, очень легко всё сломать, меняя реализацию класса ScopedLock. Часто подобный двойной вызов деструктора приводит к неопределённому поведению или другим неприятным ошибкам.

Ошибка N14, N15, N16, N17

CFsmEvent* CFsm::AddEvent( unsigned int eventType )
{ .... pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; ....
}

Предупреждение PVS-Studio: V668 CWE-570 There is no sense in testing the 'pEvent' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. fsm.cpp 259

Проверка указателя не имеет смысла, так как в случае ошибки выделения памяти будет сгенерировано исключение std::bad_alloc.

Однако всё намного хуже, когда в теле оператора if выполняется какая-то логика. Итак, проверка лишняя, но ошибка не является серьезной. Рассмотрим такой случай:

CFsmTransition* CFsm::AddTransition(....)
{ .... CFsmEvent* pEvent = AddEvent( eventType ); if ( !pEvent ) return NULL; // Create new transition CFsmTransition* pNewTransition = new CFsmTransition( state ); if ( !pNewTransition ) { delete pEvent; return NULL; } ....
}

Предупреждение анализатора: V668 CWE-570 There is no sense in testing the 'pNewTransition' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. fsm.cpp 289

Естественно, этого не произойдёт и возникнет утечка памяти. Происходит попытка освободить память, адрес которой хранится в указателе pEvent.

Сейчас объясню, что не так с этим кодом. На самом деле, когда я начал разбираться с этим кодом, то выяснилось, что всё сложнее и возможно тут не одна ошибка, а две. Для этого нам понадобится ознакомиться с устройством функции AddEvent.

CFsmEvent* CFsm::AddEvent( unsigned int eventType )
{ CFsmEvent* pEvent = NULL; // Lookup event by type EventMap::iterator it = m_Events.find( eventType ); if ( it != m_Events.end() ) { pEvent = it->second; } else { pEvent = new CFsmEvent( eventType ); if ( !pEvent ) return NULL; // Store new event into internal map m_Events[ eventType ] = pEvent; } return pEvent;
}

Обратите внимание, что функция не всегда возвращает указатель на новый объект, созданный с помощью оператора new. Иногда она берёт уже существующий объект из контейнера m_Events. Да и указатель на вновь созданный объект тоже, кстати, будет помещён в m_Events.

Я не знаком с проектом, но, скорее всего, где-то есть код, который уничтожает все эти объекты. И тут возникает вопрос: а кто владеет и должен уничтожать объекты, указатели на которые хранятся в контейнере m_Events? Тогда удаление объекта внутри функции CFsm::AddTransition вообще является лишним.

У меня сложилось впечатление, что можно просто удалить следующий фрагмент кода:

if ( !pNewTransition )
{ delete pEvent; return NULL;
}

Другие ошибки:

  • V668 CWE-571 There is no sense in testing the 'ret' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. TerrainTextureEntry.cpp 120
  • V668 CWE-571 There is no sense in testing the 'answer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. SoundManager.cpp 542

Ошибка N18, N19

static void dir_scan_callback(struct de *de, void *data) { struct dir_scan_data *dsd = (struct dir_scan_data *) data; if (dsd->entries == NULL || dsd->num_entries >= dsd->arr_size) { dsd->arr_size *= 2; dsd->entries = (struct de *) realloc(dsd->entries, dsd->arr_size * sizeof(dsd->entries[0])); } if (dsd->entries == NULL) { // TODO(lsm): propagate an error to the caller dsd->num_entries = 0; } else { dsd->entries[dsd->num_entries].file_name = mg_strdup(de->file_name); dsd->entries[dsd->num_entries].st = de->st; dsd->entries[dsd->num_entries].conn = de->conn; dsd->num_entries++; }
}

Предупреждение PVS-Studio: V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'dsd->entries' is lost. Consider assigning realloc() to a temporary pointer. mongoose.cpp 2462

Ошибка в том, что значение указателя на исходный блок памяти сразу перезаписывается новым значением, которое вернула функция realloc. Если размер массива становится недостаточным, то происходит перевыделение памяти с помощью функции realloc.

После чего невозможно будет освободить блок памяти, адрес которого ранее хранился в dsd->entries. Если память выделить не удастся, то функция realloc вернёт NULL, и этот NULL будет записан в переменную dsd->entries. Произойдёт утечка памяти.

Consider assigning realloc() to a temporary pointer. Ещё одна ошибка: V701 CWE-401 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'Buffer' is lost. Preprocessor.cpp 84

Заключение

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

Для разнообразия закончу статью приглашением последовать за мной в Instagram @pvs_studio_unicorn и в Twitter @Code_Analysis. Спасибо за внимание.

Good job, authors of the game 0 A. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. D!


Оставить комментарий

Ваш email нигде не будет показан
Обязательные для заполнения поля помечены *

*

x

Ещё Hi-Tech Интересное!

Snapdragon 8cx: 7-нанометровая платформа для ПК

Теперь мы применяем эти познания и в сфере ПК. Мы занимаемся созданием инновационных технологий, которые полностью меняют то, как люди в мире используют различные устройства для вычислений и связи друг с другом или с сетью интернет. Фактически, мы стремимся к ...

[Перевод] На что способен формат Mini PCI-e

Это крохотный форм-фактор, служащий базовым для mSATA и M. Мне очень нравится формат Mini PCI-e. Но по сути это просто PCI-e, поэтому с его помощью можно сделать гораздо больше. 2, но в основном он используются для подключения карт WiFi и ...