Владислав Столяров

14 Фев 2023

Теги:

#Cpp

Приключения капитана Блада: потонет ли Арабелла?

14 Фев 2023

Автор: Владислав Столяров

О проекте
Чертова дюжина
Заключение

Недавно в сети появилась новость о том, что был открыт исходный код игры "Приключения капитана Блада". Мы не смогли пройти мимо и проверили его качество с помощью PVS-Studio. Потонет ли легендарный корабль капитана Блада от найденных багов? Давайте узнаем!

О проекте

Игра "Приключения капитана Блада" создана студией "1C Морской волк" по мотивам книг Рафаэля Сабатини. Она повествует о приключениях главного героя этих произведений, капитана Питера Блада. Действие игры разворачивается в средневековой новой Англии. Главный герой, военный врач по образованию, был несправедливо осуждён и сослан на остров Барбадос. С острова ему удаётся бежать, после чего, захватив испанский корабль, он становится самым знаменитым пиратом того времени.

Проект был завершён в 2010 году, но так и не увидел свет. Всё потому, что компании 1С и Playlogic не смогли решить, кому принадлежат права на игру. Далее ходили новости, что финальная сборка игры безвозвратно утеряна. Однако не так давно появилось несколько новостей о том, что это не так. Появилось видео с прохождением на YouTube и работающий билд игры на одном известном трекере. Позже на GitHub выложили исходники под лицензией GPLv3.

Итак, после того как мы познакомились с проектом, предлагаю посмотреть какие интересные ошибки в нём удалось обнаружить.

Чертова дюжина

Предупреждение N1

V1075 The function expects the file to be opened for writing, but it was opened for reading. Debugger.cpp 172

Можем поздравить моего коллегу Сергея Ларина с первым трофейным срабатыванием его диагностического правила, вышедшего в релизе PVS-Studio 7.15. Данное диагностическое правило ругается на запись в файлы, открытые для чтения, и наоборот. Честно, мы даже не думали, что оно когда-нибудь сработает. Приведу проблемную функцию полностью:

void appDebuger::CopyDump(const char * src, DWORD from, const char * dst)
{
  FILE * file = fopen(src, "rb");
  if(file)
  {
    fseek(file, 0, SEEK_END);
    DWORD size = ftell(file);
    fseek(file, from, SEEK_SET);
    if(size > from)
    {
      FILE * to = fopen(dst, "a+b");
      if(to)
      {
        char buf[128];
        while(from < size)
        {          
          DWORD s = size - from;
          if(s > sizeof(buf)) s = sizeof(buf);
          memset(buf, ' ', sizeof(buf));
          fread(buf, s, 1, file);
          fwrite(buf, s, 1, file);            // <=
          from += s;
        }
        fclose(to);
      }
    }
    fclose(file);
  }
}

Итак, функция должна прочитать данные из одного файла по пути src, начиная с позиции from, и записать их в другой файл по пути dst. За исходный файл отвечает переменная file, за результирующий файл переменная to.

К сожалению, в коде происходит чтение из переменной file и запись в переменную file. Поправить код можно таким образом:

fwrite(buf, s, 1, to);

Предупреждение N2

V1065 [CWE-682] Expression can be simplified, check 'bi' and similar operands. PreviewAnimation.cpp 146

PreviewAnimation::PreviewAnimation(....)
{
  // ....
  const int bw = 30;
  const int bh = 30;
  const int bi = 3;
  const int cn = 9;

  bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + (bw + bi) + 7 - bi + 1;
  // ....
}

А это одно из моих самых любимых диагностических правил! Оно говорит нам о том, что в представленном выражении можно сократить переменную bi:

bar.r.w = 7 + (bw + bi)*cn + 7 - bi + 1 + bw + 7 + 1;

Возможно, на месте одного из bi должна быть какая-то другая переменная. Или же выражение (bw + bi) забыли на что-то умножить. Интересно, а как бы поменялись анимации в игре после этого?

Также, отмечу, что давать подобные имена константам: bw, bh, bi, cn – плохая практика. Это запутывает код для читающего его человека. Куда уж лучше было бы назвать переменные осмысленнее или хотя бы сопроводить их определения поясняющими комментариями.

Предупреждение N3

V570 [CWE-480] The 'ldMaxChars' variable is assigned to itself. StringAttr.cpp 198

void StringAttribute::LoadFromFile (....)
{
  int ldMinChars, ldMaxChars;
  // ....
  minChars = ldMinChars;
  ldMaxChars = ldMaxChars;
  // ....
}

class StringAttribute : public BaseAttribute
{
  int minChars;
  int maxChars;
}

Тут мы видим классическое присваивание переменной самой себе. Чтобы посмотреть, что должно быть на месте переменной, в которую происходит присваивание, заглянем в определение класса StringAttribute. В нём видно похожее поле maxChars. Очевидно, что программист опечатался и на самом деле должен был присвоить переменной значение ldMaxChars. Поправленный фрагмент кода:

void StringAttribute::LoadFromFile (....)
{
  int ldMinChars, ldMaxChars;
  // ....
  minChars = ldMinChars;
  maxChars = ldMaxChars;
  // ....
}

Предупреждение N4

V781 [CWE-20, CERT-API00-C] The value of the 'start' index is checked after it was used. Perhaps there is a mistake in program logic. coreloader.h 136

__forceinline void GetBootParams(const char * bootIni)
{
  long start = 0;
  // ....
  if (bootIni[start] && start > 0)
  {
    // ....
  }
  // ....
}

В данном фрагменте кода программист сначала использовал значение start для индексации по массиву bootIni, а только затем проверил его на больше 0. Проверка может быть бессмысленна, тогда стоит убрать её. Также она может иметь смысл, тогда её стоит поменять местами с операцией взятия значения по индексу.

if (start > 0 && bootIni[start])
{
  // ....
}

В противном случае, программист рискует уронить игру. Кстати, таких срабатываний в программе было довольно много. Вот несколько, на которые стоит обратить внимание в первую очередь:

V781 [CWE-20, CERT-API00-C] The value of the 'i' index is checked after it was used. Perhaps there is a mistake in program logic. tinyxml.h 352
V781 [CWE-20, CERT-API00-C] The value of the 'start' index is checked after it was used. Perhaps there is a mistake in program logic. coreloader.h 136
V781 [CWE-20, CERT-API00-C] The value of the 'pathLen' index is checked after it was used. Perhaps there is a mistake in program logic. winworkpath.h 50
....

Предупреждение N5

V700 [CWE-480] Consider inspecting the 'mView = mView = chr.Render().GetView()' expression. It is odd that variable is initialized through itself. CharacterItems.cpp 705

void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld)
{
  // ....
  Matrix mView = mView = chr.Render().GetView();
  // ....
}

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

void CharacterItems::DrawFlares(float dltTime, const Matrix & toWorld)
{
  // ....
  Matrix mView = chr.Render().GetView();
  // ....
}

Предупреждение N6

V654 [CWE-834] The condition 'n >= 0' of loop is always true. resourceselect.cpp 42

typedef unsigned int dword;

TResourceSelectorWindow::TResourceSelectorWindow ()
{
  string PakPath;
  // ....
  miss->EditorGetPackPath(PakPath);
  //....
  for (dword n = PakPath.Size()-1; n >= 0; n--)
  {
    if (PakPath[n] == '\\')
    {
      PakPath.Delete(0, n+1);
      break;
    }
  }

  // ....
}

Здесь мы видим потенциально бесконечный цикл, если в строчке нет символа \. Переменная n беззнакового типа, а значит, она всегда будет больше либо равна 0.

Но на самом деле никакого вечного цикла не будет, так как когда возникнет переполнение переменной n произойдёт сбой. Переполнение беззнаковой переменной не приводит к неопределённому поведению, но зато к нему приведёт доступ за пределами массива. На практике скорее всего возникнет Access Violation.

Тут программист написал алгоритм, который ищет первое вхождение символа \ с конца и удаляет подстроку от начала до этого символа включительно. Самым корректным исправлением было бы использование rbegin и rend, но есть нюанс – это самописная строка (аналог std::string с дополнительным функционалом), в которой, к сожалению, нет итераторов. Тогда, чтобы поправить код мы можем взять знаковую переменную с размерностью в 2 раза больше (int64_t) и пустить цикл по ней.

for (int64_t n = PakPath.Size() - 1; n >= 0; n--)
{
  if (PakPath[n] == '\\')
  {
    PakPath.Delete(0, n+1);
    break;
  }
}

Но такой способ уже не будет работать при портировании приложения на 64-битную систему, в которой функция Size уже будет должна возвращать значение типа uint64_t. И тогда итераторы были бы хорошим способом написать корректный код. Например, так бы он выглядел на С++14, если в самописной строке по-прежнему не было бы итераторов, но хотя бы были функции доступа к нижележащему буферу:

auto r_begin = std::make_reverse_iterator(PakPath.GetBuffer() + PakPath.Size());
auto   r_end = std::make_reverse_iterator(PakPath.GetBuffer());

while (r_begin != r_end )
{
  if (*r_begin == '\\')
  {
    PakPath.Delete(0, std::distance(r_begin, r_end));
    break;
  }

  ++r_begin;
}

Предупреждение N7

V611 [CWE-762, CERT-MEM51-CPP] The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] ldName;'. ColorAttr.cpp 198

void ColorAttribute::LoadFromFile (....)
{
  DWORD slen = 0;
  // ....
  char* ldName = NEW char[slen+1];
  // ....
  delete ldName;
}

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

void ColorAttribute::LoadFromFile (....)
{
  DWORD slen = 0;
  // ....
  char* ldName = NEW char[slen+1];
  // ....
  delete ldName[];
}

Подробнее проблема подобных ошибок освещена в статье моего коллеги "Почему в С++ массивы нужно удалять через delete[]".

Предупреждение N8

V607 Ownerless expression 'isSetPause ? 1 : 0'. SoundsEngine.cpp 741

void SoundsEngine::SetPause(bool isSetPause)
{
  //....
  isSetPause ? 1 : 0;
  //....
}

Анализатор указывает на мёртвый код. Как я заметил, в классе SoundsEngine содержится похожее поле isPause, однако, оно имеет тип long. Вполне вероятно, что ранее код делал преобразование параметра isSetPause из типа bool в long, но затем его отрефакторили. Строка безобидна, и её можно вполне удалить.

Предупреждение N9

V583 [CWE-783] The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0xffff0000. NoFatality.cpp 63

void _cdecl NoFatality::Draw(float dltTime, long level)
{
  //....
  for (....)
  {
    Render().DrawBox(...., IsActive() ? 0xffff0000 : 0xffff0000);
  }
  //....
}

Здесь мы видим, что второй и третий операнды тернарного оператора равны. Получается, что вторым аргументом функции DrawBox всегда будет константа 0xffff0000. Наиболее вероятно, что в одной из веток следовало передавать другое значение, иначе код можно упростить.

Предупреждение N10

V523 [CWE-691] The 'then' statement is equivalent to the 'else' statement. TaskViewer.cpp 298

void TaskViewer::SetText(const char * rawText)
{
  // ....
  if (rawText[0] == '#')
  {
    // Подменим идентификатор на реальную строку
    // потом
    str = rawText;
  }
  else
  {
    str = rawText;
  }
  // ....
}

В данном фрагменте кода анализатор отметил, что у оператора if одинаковые ветки then и else. Получается, что вне зависимости от условия будет выполняться один и тот же фрагмент кода. Возможно, это не то поведение, на которое рассчитывал программист.

Предупреждения N11

V614 [CWE-457, CERT-EXP53-CPP] Uninitialized variable 'color.c' used. Color.h 1268

mathinline dword mathcall Color::GetDword() const
{
  DColor color;
  color.r = (byte)(r * 255.0f);
  color.g = (byte)(g * 255.0f);
  color.b = (byte)(b * 255.0f);
  color.a = (byte)(a * 255.0f);

  return color.c;
}

Посмотрим, что такое DColor:

class DColor
{
public:

  union
  {
#ifndef _XBOX
  struct
  {
    ///Синий
    unsigned char b;
    ///Зелённый
    unsigned char g;
    ///Красный
    unsigned char r;
    ///Прозрачность
    unsigned char a;
  };
#else
  struct
    {
    ///Прозрачность
    unsigned char a;
    ///Красный
    unsigned char r;
    ///Зелённый
    unsigned char g;
    ///Синий
    unsigned char b;
  };
#endif
  union
  {
    ///Упакованный цвет
    dword c;
    ///Упакованный цвет
    dword color;
  };
};

В коде четыре компоненты RGBA записывают побайтово в нужные поля, а затем возвращают из функции в виде значения типа dword (4-байтовый беззнаковый тип). В языке C подобный код безобиден и программист получил бы то, чего хотел. В C++ чтение из неактивного поля union ведёт к неопределённому поведению. Причина тому – C++ оперирует объектами, у которых должно стартовать время жизни. В любой момент времени может быть активно лишь одно поле. У неактивного же поля (последний union) оно не началось.

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

Как написать код на C++, который будет работать везде и всегда одинаково? Подходов может быть несколько. Можно по-прежнему оставить четыре компоненты RGBA в виде четырех полей, а при необходимости получать их комбинацию в виде dword при помощи побитовых операций:

class DColor
{
public:
  ///Синий
  uint8_t b;
  ///Зелённый
  uint8_t g;
  ///Красный
  uint8_t r;
  ///Прозрачность
  uint8_t a;

  dword getAsDword() const noexcept
  {
#ifndef _XBOX
    return (static_cast<uint32_t>(b) << 0)
         | (static_cast<uint32_t>(g) << 8)
         | (static_cast<uint32_t>(r) << 16)
         | (static_cast<uint32_t>(a) << 24);
#else
    return (static_cast<uint32_t>(a) << 0)
         | (static_cast<uint32_t>(r) << 8)
         | (static_cast<uint32_t>(g) << 16)
         | (static_cast<uint32_t>(b) << 24);
#endif
  }
};

Другим способом может быть использование массива из 4 байт, а для его преобразования в dword можно воспользоваться функцией memcpy (или std::bit_cast, начиная с C++20):

class DColor
{
private:
  uint8_t m_components[4];

public:
  DColor(uint8_t r, uint8_t g, uint8_t b, uint8_t a) noexcept; 
  DColor(dword comp) noexcept; // To construct from dword

  DColor& operator=(dword) noexcept; // To be assigned to dword

  uint8_t& R() noexcept;
  uint8_t& G() noexcept;
  uint8_t& B() noexcept;
  uint8_t& A() noexcept;

  dword getAsDword() const noexcept
  {
#if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast)
    return std::bit_cast<dword>(m_components);
#else
    dword result;
    memcpy(&result, m_components, sizeof(m_components));
    return result;
#endif
  }
};

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

Следует писать код не так, чтобы он был максимально короткий, а чтобы его было легко читать и понимать. Я бы сделал здесь две разные реализации функции:

#if __cplusplus >= 202002L && __has_cpp_attribute(__cpp_lib_bit_cast)
  dword getAsDword() const noexcept
  {
    return std::bit_cast<dword>(m_components);
  }
#else
  dword getAsDword() const noexcept
  {
    dword result;
    memcpy(&result, m_components, sizeof(m_components));
    return result;
  }
#endif

Суть длиннее, но читать код намного приятнее.

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

Предупреждения N12

V579 [CWE-687, CERT-ARR01-C] The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. Debugger.cpp 282

void appDebuger::debuggerMakeThreadDump(....)
{
  CONTEXT ct;
  memset (&ct, 0, sizeof(&ct));
  // ....
}
typedef struct DECLSPEC_NOINITALL _CONTEXT 
{
  DWORD ContextFlags;
  // ....
  DWORD   SegCs;              // MUST BE SANITIZED
  DWORD   EFlags;             // MUST BE SANITIZED
  DWORD   Esp;
  DWORD   SegSs;
  BYTE   ExtendedRegisters[MAXIMUM_SUPPORTED_EXTENSION];

} CONTEXT;

Программист хотел инициализировать все поля структуры нулями. Размер объекта ct – 716 байт, а передали в неё размер указателя – 4 байта. Поправить вызов memset можно было так:

memset (&ct, 0, sizeof(ct));

P.S. Вообще странно, почему программисты так упорно используют этот паттерн инициализации структур с помощью memset. Можно ведь написать:

CONTEXT ct {};

Предупреждения N13

Ну и закончить перечень ошибок хочется парой прикольных опечаток:

V501 [CWE-571] There are identical sub-expressions 'lleg.to >= 0' to the left and to the right of the '&&' operator. CharacterLegs.cpp 51
V501 [CWE-571] There are identical sub-expressions 'lleg.fo >= 0' to the left and to the right of the '&&' operator. CharacterLegs.cpp 51

void CharacterLegs::Invalidate()
{
  lleg.th = ani->FindBone("Bedro_left_joint"  ,true);
  lleg.kn = ani->FindBone("Golen_left_joint"  ,true);
  lleg.fo = ani->FindBone("Foolt_left_joint"  ,true);
  lleg.to = ani->FindBone("Foolt_left_joint_2",true);

  rleg.th = ani->FindBone("Bedro_right_joint"  ,true);
  rleg.kn = ani->FindBone("Golen_right_joint"  ,true);
  rleg.fo = ani->FindBone( "Foot_right_joint"  ,true);
  rleg.to = ani->FindBone( "Foot_right_joint_2",true);

  if (   lleg.th >= 0 && lleg.kn >= 0 
      && lleg.fo >= 0 && lleg.to >= 0 
      && rleg.th >= 0 && rleg.kn >= 0 
      && lleg.fo >= 0 && lleg.to >= 0 )
  {
    // ....
  }

  // ....
}

Здесь, в условии оператора if анализатор заметил два одинаковых подвыражения lleg.to >= 0 и lleg.fo >= 0. Видно, что разработчик хотел проверить все поля и опечатался. На месте двух одинаковых подвыражений должны быть проверки других переменных:

if (   lleg.th >= 0 && lleg.kn >= 0 
    && rleg.th >= 0 && rleg.kn >= 0 
    && lleg.fo >= 0 && lleg.to >= 0
    && rleg.fo >= 0 && rleg.to >= 0)
{
  // ....
}

Заключение

В результате проверки анализатор нашёл в проекте ошибки разного рода. Чёртову дюжину из них я описал в статье. Оставшиеся – стимул для вас скачать анализатор и попробовать его (если вы ещё, конечно, этого не сделали). А ещё интереснее будет посмотреть, что найдётся в вашем собственном коде или коде коллег :).

Больше спасибо людям, благодаря которым игра всё-таки смогла увидеть свет, а также развивающим и поддерживающим её обзорами, статьями и обсуждениями. Ну а потонет ли "Арабелла", пускай каждый читатель решит сам!

#Cpp