Авторы игры 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].

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

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

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

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

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

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

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

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

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

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

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

Ошибки

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

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

Ошибка N1

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

void WaterManager::CreateWaveMeshes()
{
  ....
  int nbNeighb = 0;
  ....
  bool found = false;
  nbNeighb = 0;
  for (int p = 0; p < 8; ++p)
  {
    if (CoastalPointsSet.count(xx+around[p][0] +
                               (yy + around[p][1])*SideSize))
    {
      if (nbNeighb >= 2)
      {
        CoastalPointsSet.erase(xx + yy*SideSize);
        continue;
      }
      ++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!

Посмотрите ниже и вы увидите оператор break, который прерывает выполнение цикла. Поэтому, если переменная nbNeighb будет увеличена, то цикл будет остановлен. Таким образом, значение переменной 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" %lun",
             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

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

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

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

Ошибка 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

Нет смысла сравнивать переменную типа int с константой 0.5. Более того, по смыслу это вообще переменная типа boolean, а значит сравнение её с 0.5 выглядит совсем странно. Предположу, что вместо 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.

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

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

Заключение

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

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

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

Автор: Андрей Карпов

Источник