Часть третья

В первой статье про qdEngine было рассмотрено 10 ошибок, выбранных плагином PVS-Studio. Однако есть ещё 10 багов, заслуживающих внимания. Как говорится, лучше учиться на чужих ошибках. Заодно они хорошо демонстрируют возможности PVS-Studio в выявлении разнообразнейших ошибочных паттернов.

Предыдущие статьи о проверке игрового движка qdEngine:

Без долгих предисловий приступим к рассмотрению ассорти из багов :)

Предупреждение N1: ошибка в обработчике ошибок

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

В результате обработчики ошибок сами часто содержат ошибки. Как-нибудь напишу про это отдельную статью.

Здесь на помощь приходят инструменты статического анализа кода, такие как PVS-Studio. Анализаторы проверяют код независимо от того, как часто (с какой вероятностью) он вызывается в процессе работы приложения. Поэтому анализаторы могут найти ошибки в редко используемом коде. Рассмотрим как раз такой случай:

class CAVIGenerator  
{
  ....
  _bstr_t m_sFile;
  ....
};

HRESULT CAVIGenerator::InitEngine()
{
  ....
  TCHAR szBuffer[1024];
  ....
  if (hr != AVIERR_OK)
  {
    _tprintf(szBuffer,
             _T("AVI Engine failed to initialize. Check filename %s."),m_sFile);
    m_sError=szBuffer;
  ....
};

Предупреждение PVS-Studio: V510 The 'printf' function is not expected to receive class-type variable as third actual argument. AVIGenerator.cpp 132

Дело в том, что m_sFile – это класс типа _bstr_t. Попытка распечатать содержимое класса как обыкновенную строчку приведёт к неопределённому поведению. На практике, скорее всего, вместо имени файла будут распечатаны мусорные символы. Если мусорных символов будет слишком много, то переполнится буфер szBuffer[1024]. Переполнение буфера, в свою очередь, можно рассматривать как потенциальную уязвимость.

В общем, используйте PVS-Studio для профилактики подобных классических багов в обработчиках ошибок.

Код можно исправить, используя перегруженные операторы класса _bstr_t:

// Extract the pointers to the encapsulated Unicode or multibyte BSTR object.
operator wchar_t*
operator char*

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

_tprintf(szBuffer,
         _T("AVI Engine failed to initialize. Check filename %s."),
         (LPCSTR)m_sFile);

Тип LPCSTR развернётся в зависимости от режима компиляции (UNICODE приложение или нет) в const wchar_t* или const char*.

Предупреждение N2: недостижимый код

bool qdGameScene::follow_path_seek(qdGameObjectMoving* pObj,
                                   bool lock_target)
{
  if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
    selected_object_->set_grid_zone_attributes(sGridCell::CELL_SELECTED);

  return pObj->move(selected_object_->last_move_order(), lock_target);

  if (qdGameObjectMoving::FOLLOW_UPDATE_PATH == pObj->follow_condition())
    selected_object_->drop_grid_zone_attributes(sGridCell::CELL_SELECTED);
}

Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. qd_game_scene.cpp 1245

Код после оператора return не выполняется.

Блок недостижимого кода точно такой же, как и перед оператором return. Скорее всего, повторяющийся код получился случайно в процессе неаккуратного редактирования кода или слияния веток. Думаю, нижнюю часть кода можно просто удалить.

Предупреждение N3: new [] / delete

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

bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  char* buf = new char[buf_sz];
  ....
  delete buf;
  return true;
}

Предупреждение PVS-Studio: V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] buf;' statement should be used instead. gr_font.cpp 72

Выделяется массив, поэтому для освобождения должен использоваться оператор delete[]. Правильный код:

bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  char* buf = new char[buf_sz];
  ....
  delete[] buf;
  return true;
}

Хотя нет. Это тоже неправильный код. Правильно — это использовать умные указатели:

bool grFont::load_index(XStream& fh)
{
  int buf_sz = fh.size();
  auto buf = std::make_unique<char[]>(buf_sz);
  ....
  return true;
}

P.S.: Ещё несколько мест с точно такой же ошибкой:

V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] buf;' statement should be used instead. gr_font.cpp 101
V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] alpha_buffer_;' statement should be used instead. Check lines: 25, 168. gr_font.cpp 25
V611 The memory was allocated using the 'operator new[]' but was released using the 'operator delete'. The 'delete[] buffer;' statement should be used instead. qda_editor.cpp 1012

Предупреждение N4: функция не возвращает значение

class qdSound : public qdNamedObject, public qdResource
{
  ....
  //! Возвращает длительность звука в секундах.
  float length() const { 
#ifndef __QD_SYSLIB__
    return sound_.length(); 
#endif
  }
  ....
}

Предупреждение PVS-Studio: V591 Non-void function should return a value. qd_sound.h 70

Если определён макрос __QD_SYSLIB__, то функция length будет устроена так:

float length() const { 
}

Вызов такой функции приводит к неопределённому поведению.

Примечание. Некоторые программисты считают, что в таком случае неопределённое поведение сводится к тому, что функция возвращает случайное значение. Нет. Неопределённое поведение — это всё, что угодно. Посмотрите интересный пример в этой статье.

Предлагаю исправить код следующим образом:

#ifndef __QD_SYSLIB__
  float length() const { 
    return sound_.length(); 
  }
#endif

Это приведёт к тому, что часть внешнего кода перестанет компилироваться. Вот и отлично. Этот код тоже надо переписать, так как он не имеет смысла, раз вызывает "мёртвую" функцию length.

Если такой радикальный подход по каким-то причинам не подходит, то можно пойти на следующий компромисс:

#ifndef __QD_SYSLIB__
  float length() const { 
    return sound_.length(); 
  }
#else
  [[noreturn]] float length() const { 
    throw std::logic_error { "Function not implemented" };
  }
#endif

Предупреждение N5: утечка памяти

template<class FilterClass>
LineContribType* C2PassScale<FilterClass>::AllocContributions(UINT uLineLength, 
                                                              UINT uWindowSize)
{
  static LineContribType line_ct;

  LineContribType *res = new LineContribType; 

  line_ct.WindowSize = uWindowSize; 
  line_ct.LineLength = uLineLength; 

  if (contribution_buffer_.size() < uLineLength)
    contribution_buffer_.resize(uLineLength);

  line_ct.ContribRow = &*contribution_buffer_.begin();

  if (weights_buffer_.size() < uLineLength * uWindowSize)
    weights_buffer_.resize(uLineLength * uWindowSize);

  double* p = &*weights_buffer_.begin();

  for (UINT u = 0; u < uLineLength; u++) {
    line_ct.ContribRow[u].Weights = p;
    p += uWindowSize;
  }
  return &line_ct;
}

Предупреждение PVS-Studio: V773 The function was exited without releasing the 'res' pointer. A memory leak is possible. 2PassScale.h 107

Вот эта строчка в коде лишняя:

LineContribType *res = new LineContribType;

Созданный объект нигде не используется и не удаляется. Единственное, что происходит — утечка памяти.

Предупреждение N6: ошибка в логике

bool qdConditionGroup::remove_condition(int condition_id)
{
  ....
  conditions_container_t::iterator it1 = std::find(conditions_.begin(),
                                                   conditions_.end(),
                                                   condition_id);
  if (it1 != conditions_.end())
    return false;

  conditions_.erase(it1);
  return true;
}

Предупреждение PVS-Studio: V783 Dereferencing of the invalid iterator 'it1' might take place. qd_condition_group.cpp 58

Происходит попытка удаления несуществующего элемента:

conditions_.erase(conditions_.end());

Скорее всего, в условии допущена логическая ошибка, и на самом деле должно быть написано:

if (it1 == conditions_.end())
  return false;

conditions_.erase(it1);

Предупреждение N7: опасная работа с типом HRESULT

class winVideo
{
  ....
  struct IGraphBuilder*  graph_builder_;
  ....
};

bool winVideo::open_file(const char *fname)
{
  ....
  if (graph_builder_ -> RenderFile(wPath,NULL))
  {
    close_file(); return false;
  }
  ....
}

На первый взгляд кажется, что с кодом всё хорошо. Но дело в том, что функция RenderFile возвращает тип HRESULT:

HRESULT RenderFile(
  [in] LPCWSTR lpcwstrFile,
  [in] LPCWSTR lpcwstrPlayList
);

Поэтому проверка является ненадёжной.

Предупреждение PVS-Studio: V545 Such conditional expression of 'if' statement is incorrect for the HRESULT type value 'graph_builder_->RenderFile(wPath, 0)'. The SUCCEEDED or FAILED macro should be used instead. WinVideo.cpp 139

HRESULT — это 32-разрядное значение, разделённое на три различных поля: код серьёзности ошибки, код устройства и код ошибки. Для проверки значений типа HRESULT предназначены такие макросы, как SUCCEEDED, FAILED.

Тип HRESULT имеет множество состояний. Это может быть 0L (S_OK), 0x80000002L (Ran out of memory), 0x80004005L (unspecified failure) и т. д. Обратите внимание, что состояние S_OK кодируется как 0.

Соответственно, любой статус, кроме S_OK, считается ошибочным, и функция досрочно завершает работу. В целом, видимо, этот код сейчас работает правильно, но написан небезопасно. Небольшой рефакторинг в случае, если кто-то подумает, что функция RenderFile возвращает тип bool, приведёт к багу.

Правильная проверка:

if (FAILED(graph_builder_ -> RenderFile(wPath,NULL)))
{
  close_file(); return false;
}

Предупреждение N8: false вместо nullptr

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

const char* qdInterfaceDispatcher::get_save_title() const
{
  if (!cur_screen_)
    return false;
  ....
  return false;
}

Предупреждения PVS-Studio на строчки "return false":

V601 The 'false' value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 772
V601 The 'false' value is implicitly cast to the pointer. qd_interface_dispatcher.cpp 783

Значение false неявно преобразуется к нулевому указателю, и поэтому код работает, как и задумано. Тем не менее хотя бы во имя красоты и приличия надо использовать nullptr:

const char* qdInterfaceDispatcher::get_save_title() const
{
  if (!cur_screen_)
    return nullptr;
  ....
  return nullptr;
}

Предупреждение N9: что-то не так, но не знаю что

Прошу прощения за длинный фрагмент кода. Я не придумал, как его красиво сократить, не сделав ещё более непонятным.

Vect2s
qdGameObjectMoving::get_nearest_walkable_point(const Vect2s& target) const
{
  ....
  // Идем с конца. Если натыкаемся на проходимую точку, отличную от начальной
  bool fir_step = true;
  if (abs(x2 - x1) > abs(y2 - y1)) {
    int dx = int(float(x2 - x1)/dr.x);
    do {
      if (false == is_walkable(Vect2s(r.xi(),r.yi())))
      {
        // Если только первый шаг, то неудача
        if (fir_step) return Vect2s(-1, -1);
        r -= dr;
        return Vect2s(r.xi(),r.yi());
      }

      fir_step = false;
      r += dr;
    } while (--dx >= 0);
  }
  else {
    int dy = int(float(y2 - y1)/dr.y);
    do {
      if (false == is_walkable(Vect2s(r.xi(),r.yi())))
      {
        if (fir_step) return Vect2s(-1, -1);
        r -= dr;
        return Vect2s(r.xi(),r.yi());
      }

      fir_step = false;
      r += dr;
    } while (--dy >= 0);
  }

  // Если шаг так и не был сделан
  if (fir_step) return trg;
  ....
}

Прошу изучить циклы. Если они не заканчиваются выходом из функции, то переменная fir_step по завершению циклов всегда равна false.

Соответственно, этот код не имеет смысла:

// Если шаг так и не был сделан
if (fir_step) return trg;

Об этом и предупреждает анализатор PVS-Studio: V547 Expression 'fir_step' is always false. qd_game_object_moving.cpp 1724

Строчка с проверкой является лишней, или в циклах имеется логическая ошибка. К сожалению, я не могу сказать точнее, так как не знаком с кодом проекта.

Предупреждение N10: strictly aligned

bool zipContainer::find_index()
{
  const int buf_sz = 1024;
  char buf[buf_sz];

  stream_.seek(0,XS_END);

  while (stream_.tell() >= buf_sz) {
    stream_.seek(-buf_sz,XS_CUR);
    stream_.read(buf,buf_sz);

    const unsigned long idx_signature = 0x06054b50L;

    for (int i = 0; i < buf_sz - sizeof(long) * 3; i++) {
      if (*((unsigned long*)(buf + i)) == idx_signature) {
        XBuffer xbuf(buf + i + sizeof(long) + sizeof(unsigned short) * 4,
                     sizeof(long) * 2);
        xbuf > index_size_ > index_offset_;
        return true;
      }
    }
  }

  return false;
}

Предупреждение PVS-Studio: V1032 The pointer 'buf' is cast to a more strictly aligned pointer type. zip_container.cpp 237

В байтовом буфере происходит поиск сигнатуры 0x06054b50L. Это сделано непереносимым, опасным образом.

Начнём с того, что непонятно, какая именно сигнатура ищется. Дело в том, что тип long имеет разный размер на различных платформах. Чаще всего long бывает 32-битным и 64-битным. В зависимости от размера в памяти будет искаться:

Сигнатура из 4 байт 0x06054b50;
Или сигнатура из 8 байт 0x0000000006054b50.

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

Однако предупреждение анализатора про иное. Дело в том, что для поиска сигнатуры используется указатель типа char*. Чтобы произвести проверку, на каждой итерации адрес указателя (char *) интерпретируется как блок из 4/8 байт (unsigned long *). Картинка для пояснения:

Поиск сигнатуры в памяти

Проблема в том, что не учитывается выравнивание. Значения типа unsigned long должны лежать по адресам, кратным 4 или 8 байтам (или другому значению, в зависимости от архитектуры).

Доступ по невыровненному адресу приводит к неопределённому поведению. Как оно себя проявит, предсказать невозможно, и зависит это от архитектуры микропроцессора и компилятора. Наиболее вероятные сценарии:

Произойдёт аварийное завершение программы.
Будет прочитано неверное значение. Например, могут не учитываться два младших бита указателя и чтение всё равно будет выполнятся по границам, кратным 4 байтам.
Чтение данных будет выполняться медленно, так как процессору придётся выполнить дополнительные операции.
Всё работает корректно, по крайней мере пока :)

Исправить проблему можно разными вариантами. Самый простой вариант — использовать функцию memcmp, которая работает на уровне массива байт.

if (memcmp(buf + i, &idx_signature, sizeof(idx_signature)) == 0)

А можно ли написать код на современном C++, не используя функцию memcmp из мира C?

Можно, но, если честно, получается немного длиннее. Поэтому приведу такой вариант скорее из спортивного интереса:

const uint32_t idx_sig = 0x06054b50L;

const std::ranges::subrange
  haystack { buf, buf + buf_sz - sizeof(idx_sig) * 3 };
const auto needle = std::bit_cast<std::array<char, sizeof(idx_sig)>>(idx_sig);

if (auto res = std::ranges::search(haystack, needle); !std::empty(res))
{
  XBuffer xbuf(it + sizeof(uint32_t) + sizeof(uint16_t) * 4,
               sizeof(uint32_t) * 2);
  xbuf > index_size_ > index_offset_;
  return true;
}

Ещё что-то осталось?

Есть ещё одна ошибка, заслуживающая рассмотрения. Она связанна с объектно-ориентированным программированием и потребует подробного рассмотрения, поэтому я вынесу её в отдельную, последнюю статью.

Спасибо за внимание. Подписывайтесь на ежемесячный дайджест статей, чтобы не пропустить что-то интересное.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Let's check the qdEngine game engine, part three: 10 more bugs.

Проверка игрового движка qdEngine, часть третья: дополнительная десятка багов