Этой статьёй мы начинаем трилогию об игровом движке Nau Engine. В первой части мы сосредоточимся на его функциональности, уделяя особое внимание трём ключевым блокам ошибок: проблемам с памятью, копипасте и логическим ошибкам.

О Nau Engine

Nau Engine — это игровой движок, разработанный для упрощения процесса создания и поддержки игр. Он основан на трёх принципах: универсальность, доступность и кроссплатформенность.

1. Универсальность: Nau Engine предлагает инструменты для всех этапов создания игры — от разработки до пострелизной поддержки. Это включает системы для аналитики, обработки ошибок и обновления контента.

2. Доступность: движок нацелен на разработчиков с разным уровнем опыта. Он предоставляет интуитивно понятные инструменты и шаблоны, что помогает новичкам легко начать, а более опытным пользователям — углубляться в детали.

3. Кроссплатформенность: Nau Engine позволяет адаптировать игры для различных платформ, таких как ПК, мобильные устройства и веб, что делает разработку более гибкой и удобной.

Особое внимание стоит уделить адаптированной ECS (Entity Component System) библиотеке от движка Dagor, которая интегрирована в Nau Engine. Эта библиотека не только обеспечивает высокую производительность, но и навевает приятные воспоминания о таких культовых играх, как:

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

Следует отметить, что для анализа использовалось состояние репозитория на момент коммита 020cbfe.

Результаты проверки

Копипаста

Фрагмент N1

NAU_ASSERT(irRequest.usage.access == req.usage.access 
        && irRequest.usage.type == irRequest.usage.type);

Предупреждение PVS-Studio: V501_ There are identical sub-expressions 'irRequest.usage.type' to the left and to the right of the '==' operator. irGraphBuilder.cpp 719_

В этом фрагменте нас интересует макрос NAU_ASSERT, который служит для проверки условий во время выполнения кода. Если быть точнее, то интересно нам второе сравнение внутри этого макроса, а именно: irRequest.usage.type == irRequest.usage.type. При отдельном написании сразу становится видно, что в левой и правой части сравнения используется одно и то же выражение, а следовательно, условие всегда будет истинным.

Вероятно, автор кода хотел сравнить irRequest.usage.type с req.usage.type, чтобы проверить, совпадают ли типы в двух разных объектах:

NAU_ASSERT(irRequest.usage.access == req.usage.access 
        && irRequest.usage.type   == req.usage.type);

Фрагмент N2

bool VFXModFXInstance::deserialize(const nau::DataBlock* blk)
{
  ....
  m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
  m_life.part_life_max = blk->getReal("lifeMin", 5.0f);
  ....
}

Предупреждение PVS-Studio: V656_ Variables 'm_life.part_life_min', 'm_life.part_life_max' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'blk->getReal("lifeMin", 5.0f)' expression. Check lines: 94, 95. vfx_mod_fx_instance.cpp 95_

В функции VFXModFXInstance::deserialize наблюдается проблема, связанная с инициализацией переменных m_life.part_life_min и m_life.part_life_max. Из-за копипасты обе переменные получают свои значения через вызов одной и той же функции с использованием одного и того же ключа "lifeMin". Очевидно, что для минимального и максимального времени жизни частиц должны использоваться разные значения. Поэтому можем предположить, что во втором случае вместо "lifeMin" должен использоваться ключ "lifeMax":

m_life.part_life_min = blk->getReal("lifeMin", 5.0f);
m_life.part_life_max = blk->getReal("lifeMax", 5.0f);

Фрагмент N3

Material* Material::clone() const
{
  auto material = new (std::nothrow) Material();
  if (material)
  {
    ....
    material->_textureSlots = material->_textureSlots;
    material->_textureSlotIndex = material->_textureSlotIndex;
    ....
  }
....
}

Предупреждения PVS-Studio:

V570_ The 'material->_textureSlots' variable is assigned to itself. CCMaterial.cpp 527_

V570_ The 'material->_textureSlotIndex' variable is assigned to itself. CCMaterial.cpp 528_

Здесь происходит присваивание переменных _textureSlots и _textureSlotIndex самим себе, что не имеет смысла и может быть ошибкой. Судя по названию функции-члена класса (clone), нужно скопировать состояния объекта в новый. Исходя из этого, исправление может быть таким:

material->_textureSlots = _textureSlots;
material->_textureSlotIndex = _textureSlotIndex;

Логические ошибки

Фрагмент N4

template <typename T>
requires(std::is_enum_v<T>)
class TypedFlag
{
public:
  using EnumType = T;
  using ValueType = std::underlying_type_t<T>;
  ....
private:
  ValueType m_value = 0;
  ....
  friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags)
  {
  }
  ....
};

Предупреждение PVS-Studio: V591_ Non-void function should return a value. typed_flag.h 145_

В перегрузке оператора | для шаблона класса TypedFlag отсутствует возвращаемое значение, что может привести к ошибкам в логике работы программы. Особенно интересна эта ситуация тем, что для оператора |= логика реализована:

friend TypedFlag<T>& operator|=(TypedFlag<T>& value, T flag)
{
  return value.set(flag);
}

Есть несколько способов исправить эту проблему.

Способ N1. Реализовать функциональность операторов. Они должны возвращать результат, который соответствует логике объединения флагов. Например:

return TypedFlag<T> { value }.set(flags);

Способ N2. Удалить оператор. Если функционал оператора | не нужен, его можно удалить, чтобы избежать путаницы или, например, можно объявить оператор как удалённый:

friend TypedFlag<T> operator|(TypedFlag<T> value, TypedFlag<T> flags) = delete;

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

И вот ещё случаи:

• V591 Non-void function should return a value. typed_flag.h 149

• V591 Non-void function should return a value. typed_flag.h 163

• V591 Non-void function should return a value. string.h 511

Фрагмент N5

EntityId EntityManager::createEntitySync(....)
{
  ....
  if (EASTL_UNLIKELY(result != RequestResources::Loaded))
  {
#if NAU_DEBUG
    if (result == RequestResources::Loaded)
    {
      ....
    }
#endif
    if (result == RequestResources::Error)
    {
      ....
    }
  }
  ....
}

Предупреждение PVS-Studio: V637_ Two opposite conditions were encountered. The second condition is always false. Check lines: 711, 714. entityManager2.cpp 711_

В коде присутствует логическая ошибка, которая может ввести в заблуждение. Рассмотрим детали:

• Условие if (EASTL_UNLIKELY(result != RequestResources::Loaded)): оно проверяет, что результат не равен RequestResources::Loaded. Если это условие истинно, значит, ресурсы не загружены.

• Блок #if NAU_DEBUG: внутри него есть проверка if (result == RequestResources::Loaded), которая никогда не будет истинной, так как мы уже проверили, что result не равен RequestResources::Loaded. Это создаёт противоречие и может ввести в заблуждение.

Фрагмент N6

IGenSave* create_async_writer(....)
{
  AsyncWriterCB* ret = new AsyncWriterCB(buf_size);
  if (!ret->open(fname, mode))
  {
    if (ret)
    {
      delete ret;
      ret = nullptr;
    }
  }
  return ret;
}

Предупреждение PVS-Studio: V668_ 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. asyncWrite.cpp 363_

В этом коде происходит проверка указателя ret на nullptr после вызова new. Это может быть источником ошибок, поскольку указатель ret всегда будет указывать на выделенную память, если выделение прошло успешно. В случае если память не удастся выделить, будет выброшено исключение, и код ниже не будет выполнен.

Кроме того, указатель ret разыменовывается до проверки, что делает саму проверку if (ret) ещё более бессмысленной. Если бы ret действительно был равен nullptr, программа бы уже аварийно завершилась при попытке разыменования.

И вот ещё случаи:

• V668 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. asyncWrite.cpp 377

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontCharMap.cpp 58

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontCharMap.cpp 77

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontCharMap.cpp 89

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontFNT.cpp 523

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontFNT.cpp 545

• V668 There is no sense in testing the 'tempFont' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCFontFNT.cpp 569

• V668 There is no sense in testing the 'layerGradient' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCLayer.cpp 579

• V668 There is no sense in testing the 'layerGradient' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCLayer.cpp 592

• V668 There is no sense in testing the 'pwszBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCDevice-win32.cpp 90

• V668 There is no sense in testing the 'pwszBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CCDevice-win32.cpp 326

Фрагмент N7

void Properties::skipWhiteSpace()
{
  signed char c;
  do
  {
    c = readChar();
  } while (isspace(c) && c != EOF);
  ....
  if (c != EOF)
  {
    ....
  }
}

Предупреждения PVS-Studio:

V739_ EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 464_

V739_ EOF should not be compared with a value of the 'char' type. The 'c' should be of the 'int' type. CCProperties.cpp 468_

Функция-член Properties::skipWhiteSpace использует переменную типа signed char для хранения символа, считанного с помощью функции Properties::readChar. Последняя, судя по комментарию, симулирует поведение std::getchar:

//
// Stream simulation
//
signed char Properties::readChar()
{
  if (eof())
    return EOF;
  return _data->_bytes[(*_dataIdx)++];
}

Код содержит две ошибки:

Ошибка N1. Функция std::getchar неспроста возвращает значения типа int. Функция возвращает символ (1 байт) или EOF в результате ошибки. EOF — константа, имеющая негативное значение, обычно -1. Поэтому перед тем как преобразовывать результат std::getchar к символу, нужно отсечь вариант с EOF:

if (int res = std::getchar(); res != EOF)
{
  char ch = res;
  // your logic with character
}

Что может произойти, если не обработать такую ситуацию? Пользователи, использующие Extended ASCII Codes, иногда сталкиваются с ошибкой, когда один из символов их алфавита некорректно обрабатывается программами. Например, последняя буква русского алфавита в кодировке Windows-1251 как раз имеет код 0xFF и воспринимается некоторыми программами как конец файла.

Корректная имплементация функции Properties::readChar должна выглядеть так:

//
// Stream simulation
//
int Properties::readChar()
{
  if (eof())
    return EOF;
  return _data->_bytes[(*_dataIdx)++];
}

Ошибка N2. Функция Properties::skipWhiteSpace после исправления Properties::readChar также должна работать с типом int до тех пор, пока не отсечёт вариант с EOF:

void Properties::skipWhiteSpace()
{
  int c;
  do
  {
    c = readChar();
  }
  while (c != EOF && isspace(c));
  ....
  if (c != EOF)
  {
    // Now we can cast 'c' to 'signed char'
    signed char ch = c;
    ....
  }
}

Фрагмент N8

void Sweep::EdgeEvent(....)
{
  ....
  if (o1 == COLLINEAR) {
    if( triangle->Contains(&eq, p1)) {
    ....
    } else {
      std::runtime_error("EdgeEvent - collinear points not supported");
      assert(0);
    }
    return;
  }
  ....
  if (o2 == COLLINEAR) {
    if (triangle->Contains(&eq, p2)){
      ....
    } else {
      std::runtime_error("EdgeEvent - collinear points not supported");
      assert(0);
    }
    return;
  }
  ....
}

Предупреждения PVS-Studio:

V596_ The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 123_

V596_ The object was created but it is not being used. The 'throw' keyword could be missing: throw runtime_error(FOO); sweep.cc 140_

В функции-члене Sweep::EdgeEvent создаются объекты исключений std::runtime_error при обнаружении коллинеарных точек, но они не выбрасываются. Это приводит к тому, что программа может продолжать выполнение, игнорируя возникшую ошибку, что может вызвать некорректное поведение.

Исправленный код:

throw std::runtime_error("EdgeEvent - collinear points not supported");

Фрагмент N9

std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
    return (((size_t) shaderStage) & 0xF)
           |((size_t)(location[0] << 4))
           |((size_t)(location[1] << 8));
}

Предупреждения PVS-Studio:

V1028_ Possible overflow. Consider casting operands of the 'location[0] << 4' operator to the 'size_t' type, not the result. Types.cpp 40_

V1028_ Possible overflow. Consider casting operands of the 'location[1] << 8' operator to the 'size_t' type, not the result. Types.cpp 40_

Оператор UniformLocation::operator() выполняет битовые операции над значениями, полученными из переменных shaderStage и location, чтобы вернуть уникальный идентификатор.

Проблема заключается в том, что операции сдвига (<<) могут привести к переполнению, если значения location[0] или location[1] слишком велики. Следует привести location[0] и location[1] к типу size_t перед выполнением операции сдвига:

std::size_t UniformLocation::operator()(const UniformLocation &uniform) const
{
    return (((size_t) shaderStage) & 0xF)
           |((size_t)(location[0]) << 4)
           |((size_t)(location[1]) << 8);
}

Проблемы с памятью

Фрагмент N10

char** m_memBlock = nullptr;

template<class Type>
class ThreadLocalValue final
{
  ....
private:
  void resizeLines(size_t sizeReq)
  {
    ....
    auto newSize = sizeReq + 1;
    ....
    m_memBlock = static_cast<char**>(realloc(m_memBlock, 
                                             sizeof(char*) * newSize));
    ....
  }
  ....
};

Предупреждение PVS-Studio: V701_ realloc() possible leak: when realloc() fails in allocating memory, original pointer 'm_memBlock' is lost. Consider assigning realloc() to a temporary pointer. thread_local_value.h 235_

В данном коде m_memBlock представляет собой массив указателей на char, а метод resizeLines изменяет его размер с помощью realloc.

Если realloc успешно выделяет память, он может вернуть либо тот же указатель, либо новый, переместив данные. Однако, если выделение памяти не удаётся, realloc возвращает nullptr, а прежнее значение m_memBlock теряется.

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

И вот ещё случаи:

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_buffer' is lost. Consider assigning realloc() to a temporary pointer. CCDrawNode.cpp 102

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_bufferGLPoint' is lost. Consider assigning realloc() to a temporary pointer. CCDrawNode.cpp 116

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_bufferGLLine' is lost. Consider assigning realloc() to a temporary pointer. CCDrawNode.cpp 130

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '_triBatchesToDraw' is lost. Consider assigning realloc() to a temporary pointer. CCRenderer.cpp 629

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer '* out' is lost. Consider assigning realloc() to a temporary pointer. ZipUtils.cpp 185

• V701 realloc() possible leak: when realloc() fails in allocating memory, original pointer 'arr->arr' is lost. Consider assigning realloc() to a temporary pointer. ccCArray.cpp 100

Фрагмент N11

IAssetContainerLoader* loader = nullptr;
const auto importSettingsProviders = ....;
RuntimeReadonlyDictionary::Ptr importSettings;
for (const auto& importSettingsProvider : importSettingsProviders)
{
  if (importSettings = 
      importSettingsProvider->getAssetImportSettings(containerPath, 
                                                     *loader); 
      importSettings)
  {
    break;
  }
}

Предупреждение PVS-Studio: V522_ Dereferencing of the null pointer 'loader' might take place. asset_file_content_provider.cpp 34_

В начале кода указатель loader инициализируется значением nullptr. Затем в цикле вызывается функция-член getAssetImportSettings, при этом аргументом передаётся разыменованный указатель loader. Странно, что между этими строками он никаким образом не модифицируется. Получаем гарантированное неопределённое поведение.

И вот ещё случай:

• V522 Dereferencing of the null pointer 'object' might take place. The null pointer is passed into 'replace' function. Inspect the second argument. Check lines: 'CCVector.h:481', 'CCLayer.cpp:976'. CCVector.h 481

Фрагмент N12

struct DelayedEntityCreationChunk
{
  ....
  DelayedEntityCreationChunk(uint16_t cap) : capacity(cap)
  {
    queue = (DelayedEntityCreation*)
      malloc(capacity * sizeof(DelayedEntityCreation));
  }
  ....
};

Предупреждение PVS-Studio: V630_ The 'malloc' function is used to allocate memory for an array of objects which are classes containing constructors and destructors. entityManager.h 1391_

В этом фрагменте анализатор ругается на использование malloc для выделения памяти под массив объектов классов с конструкторами и деструкторами. В общем случае так делать не стоит, лучше применять другие способы создания объектов на куче. Однако в данной ситуации это ложное срабатывание, так как создание объектов происходит позже через метод emplace_back (примерно то же самое происходит и в векторе).

Тут у читателя может возникнуть вопрос: "А зачем тогда вообще включать этот фрагмент в статью?". Я собирался убрать его, но тут моё внимание привлекло ещё кое-что в этом классе:

struct DelayedEntityCreationChunk
{
  ....
  DelayedEntityCreation* queue = nullptr;
  uint16_t readFrom = 0, writeTo = 0, capacity;

  DelayedEntityCreationChunk(uint16_t cap) :
    capacity(cap)
  {
    queue = (DelayedEntityCreation*)
         malloc(capacity * sizeof(DelayedEntityCreation));
  }

  ~DelayedEntityCreationChunk()
  {
    for(auto i = begin(), e = end(); i != e; ++i)
      i->~DelayedEntityCreation();
    free(queue);
  }

  DelayedEntityCreationChunk(const DelayedEntityCreationChunk&) = delete;
  DelayedEntityCreationChunk&
    operator=(const DelayedEntityCreationChunk&) = delete;

  DelayedEntityCreationChunk(DelayedEntityCreationChunk&& a)
  {
    memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
    memset(&a, 0, sizeof(DelayedEntityCreationChunk));
  }

  DelayedEntityCreationChunk& operator=(DelayedEntityCreationChunk&& a)
  {
    alignas(DelayedEntityCreationChunk) 
      char buf[sizeof(DelayedEntityCreationChunk)];

    memcpy(buf, this, sizeof(DelayedEntityCreationChunk));
    memcpy(this, &a, sizeof(DelayedEntityCreationChunk));
    memcpy(&a, buf, sizeof(DelayedEntityCreationChunk));
    return *this;
  }

  ....

  template <typename... Args>
  bool emplace_back(EntityId eid, Args &&...args)
  {
    DAECS_EXT_ASSERT(!full());
    new (queue + (writeTo++)) DelayedEntityCreation(eid,
                                             eastl::forward<Args>(args)...);
    return full();
  }
  ....
};

Какими свойствами обладает этот класс:

• Нетривиально разрушаемый, т.к. определён пользовательский деструктор.

• Нетривиально перемещаемый, т.к. определены пользовательские конструктор и оператор перемещения. При этом операции перемещения не помечены как noexcept, что в некоторых ситуациях не есть хорошо.

• Move-only, т.к. удалены конструктор и оператор копирования.

При этом можно заметить, что при работе с объектами этого класса в конструкторе и операторе перемещения активно используются memcpy и memset. Стандарт чётко регламентирует их поведение только для тривиально копируемых типов, в ином случае оно может быть не определено.

Рекомендую переделать код на примерно следующий:

struct DelayedEntityCreationChunk
{
  ....
  DelayedEntityCreation* queue = nullptr;
  uint16_t readFrom = 0, writeTo = 0, capacity;
  ....
public:
  DelayedEntityCreationChunk(DelayedEntityCreationChunk &&a) noexcept
    : queue { std::exchange(a.queue, {}) }
    , readFrom { std::exchange(a.readFrom, {}) }
    , writeTo { std::exchange(a.writeTo, {}) }
    , capacity { std::exchange(a.capacity, {}) }
  {
  }

  void swap(DelayedEntityCreationChunk &other) noexcept
  {
    auto lhs = std::tie(queue, readFrom, writeTo, capacity);
    auto rhs = std::tie(other.queue, other.readFrom,
                        other.writeTo, other.capacity);

    std::swap(lhs, rhs);
  }

  DelayedEntityCreationChunk&
    operator=(DelayedEntityCreationChunk &&a) noexcept
  {
    if (this == std::addressof(a)) return *this;

    auto tmp = std::move(a);
    swap(tmp);
    return *this;
  }
  ....
};

Заключение

В первой части нашего исследования игрового движка Nau Engine мы погрузились в его функциональность, уделив внимание проблемам с памятью, копипасте и логическим ошибкам. Эти проблемы могут стать настоящими камнями преткновения для разработчиков, стремящихся создавать увлекательные игры. Хотя Nau Engine предлагает множество заманчивых возможностей, обнаруженные недостатки могут существенно повлиять на производительность и общее впечатление от игр.

В следующей части нашей трилогии мы будем искать пути оптимизации и улучшения работы движка.

Благодарю за внимание!