В этой статье мы расскажем вам о том, как путешествовали по землям Ирдии. Нас ждали приключения, полные славных сражений, побед и редких наград в виде могущественных артефактов! "Что же это за артефакты такие?" — спросите вы. Конечно же, это ошибки, найденные в коде довольно известной и крайне увлекательной игры "The Battle for Wesnoth".

О проекте

"The Battle for Wesnoth" — пошаговая стратегия в жанре эпического фэнтези с открытым исходным кодом.

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

Если вам тоже за 30, и вы по каким-то совершенно неведомым для меня причинам обошли эту потрясающую игру стороной, но в далёком 2004 году у вас уже был мобильный кнопочный телефон, наверняка вспомните мобильный проект Ancient Empire. Так вот, The Battle for Wesnoth — это как Ancient Empire, только на максималках во всём!

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

Игра вышла 18 июня 2003 года и набрала более 70% положительных отзывов в Steam за всё время существования там. Проект как никогда жив и развивается: буквально недавно, 5 июня, вышло обновление 1.19.12. Тем временем, каждый день на странице репозитория GitHub появляются всё новые изменения.

Разумеется, как тру программист, игру я собирал из исходного кода. И, кстати, собирается она по инструкции от заботливых разработчиков довольно просто. Удивительно, насколько они заморочились, ведь проект имеет множество всяких зависимостей, однако после ввода пары команд мы уже получаем готовую игру. Всё, что нужно, это запустить её и наслаждаться!

На всякий случай, коммит, на котором я клонировал проект: febf638.

Вездесущий спойлер

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

Немного об интеграции PVS-Studio в проект на этапе анализа

Совсем недавно вышла новая версия PVS-Studio — 7.37. Релиз пестрит множеством всяких изменений, но больше всего радует, что была проведена огромная работа по увеличению количества выдаваемых полезных предупреждений и, соответственно, уменьшению ложных.

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

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

1. Сделать минимальную предварительную настройку. Как упоминалось ранее, желательно отключить лишние группы диагностических правил и оставить только необходимые. Например, если стандарты MISRA/AUTOSAR не используются в разработке проекта, то и оставлять включёнными эти группы диагностик не имеет смысла.

2. Использовать настройку фильтрации для отображения наиболее интересных предупреждений. Это позволит ускорить и упростить ознакомление с отчётом.

3. Очень советую обратить внимание на возможность настройки анализа через файл *.pvsconfig. Благодаря ему настройка будет ещё более удобной, точной и быстрой, а также появятся новые возможности.

4. Дополнительно для повышения удобства работы с отчётом можно использовать механизм массового подавления предупреждений при помощи suppress-файлов. Благодаря этому механизму можно подавить как предупреждения определённого диагностического правила для конкретного файла, так и вообще все предупреждения, чтобы далее получать только новые (так называемый baseline).

5. Вместо того, чтобы каждый раз выполнять полный анализ проекта, можно использовать инкрементальный режим. Анализироваться будут только изменённые файлы, и предупреждения будут выданы только на новый код.

6. Полный анализ проекта можно интегрировать в CI во время ночных сборок. Если вы применяете модель Pull/Merge Requests при работе с проектом, то можно интегрировать анализ и туда.

Мы, разумеется, для проверки кода open source проектов и последующего написания статей редко проделываем все эти шаги. Самые основные: исключаем third-party библиотеки, а также включаем только группы диагностических правил общего назначения и микрооптимизаций. Так мы однозначно видим состояние кодовой базы проекта и, выделив интересные предупреждения, уже можем поделиться результатами с вами.

Что ж, перейдём к просмотру ошибок, ведь их есть у меня для вас!

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

Фрагмент N1

inline double stod(std::string_view str) {
  trim_for_from_chars(str);
  double res;
  auto [ptr, ec] = utils::charconv::from_chars(str.data(), 
                                   str.data() + str.size(), res);

  if(ec == std::errc::invalid_argument) {
    throw std::invalid_argument("");                // <=
  } else  if(ec == std::errc::result_out_of_range) {
    throw std::out_of_range("");                    // <=
  }
  return res;
}

Начать повествование хотелось бы с такой небольшой, но очень серьёзной проблемы. Видите ли вы её? Если нет, то, похоже, вам не приходилось разбираться с ситуациями, когда в техническую поддержку обращается пользователь с проблемой, а вместо понятного описания пишет что-то в духе "всё сломалось/упало/закрылось" и "нет, ничего не написало".

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

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

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

V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 146

V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 148

И функция, обозначенная выше, не единственная бросает исключение с пустой строкой. Аналогичный пример в этом же файле:

inline int stoi(std::string_view str) {
  trim_for_from_chars(str);
  int res;
  auto [ptr, ec] = utils::charconv::from_chars(
                       str.data(), str.data() + str.size(), res);

  if(ec == std::errc::invalid_argument) {
    throw std::invalid_argument("");               // <=
  } else if(ec == std::errc::result_out_of_range) {
    throw std::out_of_range("");                   // <=
  }
  return res;
}

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

V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 159

V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 161

Фрагмент N2

std::string dsgettext (const char * domainname, const char *msgid)
{
  std::string msgval = dgettext (domainname, msgid);
  if (msgval == msgid) {
    const char* firsthat = std::strchr (msgid, '^');
    if (firsthat == nullptr)
      msgval = msgid;
    else
      msgval = firsthat + 1;
  }
  return msgval;
}

Предупреждение PVS-Studio: V1048 [CWE-1164] The 'msgval' variable was assigned the same value. gettext.cpp 440

В этом фрагменте кода мы наблюдаем присвоение переменной msgval одного и того же значения. Сначала происходит инициализация выражением:

dgettext (domainname, msgid);

Функция dgettext, по сути, является обёрткой над функцией dgettext из библиотеки :

Namespace bl = boost::locale;
....
std::string dgettext(const char* domain, const char* msgid)
{
  return bl::dgettext(domain, msgid, get_manager().get_locale());
}

Эта функция предназначена для немедленного перевода строк или сообщений. Она принимает исходное сообщение (или ключ, представляющий его — параметр msgid) в качестве входных данных и возвращает переведённую строку в указанном языковом стандарте.

Если функция возвращает этот самый msgid, то перевод для фразы не найден. В таком случае разработчики проверяют, начиналось ли исходное сообщение с символа ^, и, если это так, оставляют в качестве итога подстроку, начинающуюся после этого символа:

msgval = firsthat + 1;

Итак, мы поняли, как работает код, но вот зачем переменной msgval повторно присваивается значение msgid, понятно так и не стало. А раз уж это действие лишнее, то код стоит чуть упростить:

std::string dsgettext (const char * domainname, const char *msgid)
{
  std::string msgval = dgettext (domainname, msgid);
  if (msgval == msgid) {
    const char* firsthat = std::strchr (msgid, '^');
    if (firsthat != nullptr)
      msgval = firsthat + 1;
  }
  return msgval;
}

Обычно подобные странности появляются вследствие рефакторинга и являются ошибками, поэтому рекомендую разработчикам внимательнее присмотреться к этому коду.

Фрагмент N3

void mp_create_game::sync_with_depcheck()
{
  ....
  auto& game_types_list = find_widget<menu_button>("game_types");
  game_types_list.set_value(
    std::distance(level_types_.begin(),
                  std::find_if(level_types_.begin(), 
                               level_types_.begin(), // <=
                               [&](const level_type_info& info)
                               {
                                 return info.first == new_level_index.first;
                               })));
  ....
}

Предупреждение PVS-Studio: V539 Consider inspecting iterators which are being passed as arguments to function 'find_if'. mp_create_game.cpp 534

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

Вот как это работает. В функцию std::find_if первыми двумя аргументами были переданы одинаковые итераторы, указывающие на начало — level_types_.begin(). В свою очередь, функция std::find_if честно выполнит ноль итераций, и, так и не успев найти что-либо, вернёт второй аргумент, т.е. начальный итератор level_types_.begin().

После этого функция std::distance так же честно выполнит свою работу, посчитав расстояние от своего первого аргумента, итератора level_types_.begin(), до итератора level_types_.begin(), который вернула функция std::find_if, и вернёт 0.

Исправленный код будет выглядеть следующим образом:

void mp_create_game::sync_with_depcheck()
{
  ....
  auto& game_types_list = find_widget<menu_button>("game_types");
  game_types_list.set_value(
    std::distance(level_types_.begin(),
                  std::find_if(level_types_.begin(), 
                               level_types_.end(),
                               [&](const level_type_info& info)
                               {
                                 return info.first == new_level_index.first;
                               })));
  ....
}

Фрагмент N4

template<typename T>
class enable_lua_ptr
{
public:
  ....
  enable_lua_ptr& operator=(enable_lua_ptr&& o)
  {
    self_ = std::move(o.self_);
    *self_ = static_cast<T*>(this);
  }                                  // <=
....
}

Предупреждение PVS-Studio: V591 [CWE-393] Non-void function should return a value. lua_ptr.hpp 34

Как вы уже могли заметить, оператор перемещения не возвращает значение. Получаем неопределённое поведение на ровном месте.

Фрагмент N5

std::string addon_info::display_icon() const
{
  std::string ret = icon;

  // make sure it's set to something when there are issues
  // otherwise display errors will spam the log while the 
  // add-ons manager is open
  if(ret.empty()){
    ret = "misc/blank-hex.png";
  } if(!image::exists(image::locator{ret}) && !ret.empty()) { // <=
    ERR_AC << "add-on '" << id << 
           "' has an icon which cannot be found: '" << ret << "'";
    ret = "misc/blank-hex.png";
  } else if(ret.find("units/") != std::string::npos 
         && ret.find_first_of('~') == std::string::npos) {
    // HACK: prevent magenta icons, because they look awful
    LOG_AC << "add-on '" << id << 
           "' uses a unit baseframe as icon without TC/RC specifications";
    ret += "~RC(magenta>red)";
  }
  return ret;
}

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

V560 [CWE-571] A part of conditional expression is always true: !ret.empty(). info.cpp 240

V646 [CWE-670] Consider inspecting the application's logic. It's possible that 'else' keyword is missing. info.cpp 240

По задумке автора, скорее всего, если в диспетчере аддонов не нашлось логотипа или титульной картинки для какого-то конкретного аддона, картинка выставляется в дефолтную.

Проблема в том, что вторая часть проверки !ret.empty(), как нам подсказывает анализатор, и правда всегда будет возвращать true из-за предыдущей противоположной проверки с присвоением в переменную ret строкового литерала.

Дополнительно анализатор подсказывает, что, возможно, программист случайно пропустил ключевое слово else между первым и вторым условием. Похоже, оно там действительно должно быть: если icon — это пустая строка, функция вернёт misc/blank-hex.png. Это же должно произойти, если icon — не пустая строка, но файла по этому пути не существует. При этом проверку !ret.empty() можно просто удалить за ненадобностью.

Фрагмент N6

class install_dependencies : public modal_dialog
{
public:
  explicit install_dependencies(const addons_list& addons)
    : modal_dialog(window_id()), addons_(addons)   // <=
  {}
....
private:
  virtual const std::string& window_id() const override;
....
}

Предупреждение PVS-Studio: V1099 [CWE-908] Using the 'window_id' function of uninitialized derived class while initializing the 'modal_dialog' base class will lead to undefined behavior. install_dependencies.hpp 29

Благодаря этому фрагменту кода я расскажу вам чуточку больше о неопределённом поведении.

Как мы видим выше, класс install_dependencies является производным от класса modal_dialog. В конструкторе install_dependencies происходит инициализация родительского класса значением, возвращаемым (внимание!) нестатической функцией window_id. То есть произойдёт следующее:

1. Исполнение списка инициализации:

   • вызов метода install_dependencies::window_id;

   • вызов конструктора класса modal_dialog;

   • инициализация поля addons_;

2. Исполнение тела конструктора класса install_dependencies.

А это значит, что произойдёт вызов функции у объекта класса, который ещё не был инициализирован! А это, в свою очередь, нарушает правило стандарта, согласно которому:

Member functions (including virtual member functions, [class.virtual]) can be called for an object under construction

Similarly, an object under construction can be the operand of the typeid operator ([expr.typeid]) or of a dynamic_cast ([expr.dynamic.cast]).

However, if these operations are performed in a ctor-initializer (or in a function called directly or indirectly from a ctor-initializer) before all the mem-initializers for base classes have completed, the program has undefined behavior.

И это ещё не всё. Как вы могли заметить, функция-член window_id является виртуальной и переопределена в классе install_dependencies. Проблемы могут появиться в дальнейшем, когда программист напишет производный от него класс, в котором будет переопределён window_id.

В момент создания объекта этого производного класса, когда будет отрабатывать конструктор installed_dependencies, информации о существовании нового переопределения ещё нет. Поэтому в списке инициализации всегда будет вызываться именно функция installed_dependencies::window_id. Это может отличаться от того, что задумывал изначально программист. Подробнее об этом можно почитать здесь.

Фрагмент N7

double readonly_context_impl::power_projection(const map_location& loc, 
                                               const move_map& dstsrc) const
{
  map_location used_locs[6];
  int ratings[6];
  std::fill_n(ratings, 0, 6);   // <=
  int num_used_locs = 0;
....
}

Предупреждение PVS-Studio: V575 [CWE-628] The 'fill_n' function processes '0' elements. Inspect the second argument. contexts.cpp 987

Скорее всего, разработчик хотел заполнить нулями массив ratings, но что-то пошло не так, и при вызове функции std::fill_n массив не будет проинициализирован вовсе.

Проблема заключается в том, что были перепутаны 2-й и 3-й аргументы при вызове std::fill_n. Ведь, насколько мы знаем или прочитали только что в документации, 2-й параметр отвечает за то, сколько раз в массив будет записано значение, а 3-й обозначает само это значение. То есть код сейчас записывает в массив ratings число 6 целых ноль раз.

Можно исправить ситуацию, поменяв аргументы 0 и 6 местами, или просто добавив явную инициализацию при создании массива:

int ratings[6]{}; // явно инициализирует массив нулями

Фрагмент N8

std::pair<map_location,map_location> move_to_targets_phase::
                     choose_move(std::vector<target>& targets)
{
  std::vector<target>::iterator best_target = best_rated_target->tg;
  ....
  if(best != units_.end()) {
    LOG_AI << "Could not make good move, staying still";

    //this sounds like the road ahead might be dangerous,
    //and that's why we don't advance.
    //create this as a target, attempting to rally units around
    targets.emplace_back(best->get_location(), best_target->value);
    best_target = targets.end() - 1;                               // <=
    return std::pair(best->get_location(), best->get_location());
  }
  LOG_AI << "Could not find anywhere to move!";
  return std::pair<map_location,map_location>();
}

Предупреждение PVS-Studio: V1001 [CWE-563] The 'best_target' variable is assigned but is not used by the end of the function. ca_move_to_targets.cpp 604

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

Возможно, пока что так сделано специально, и мир ещё не готов к боевому AI уровня Джарвиса из фильма "Железный человек", но не могу ни подтвердить, ни опровергнуть. Поэтому просто советую разработчикам обратить внимание на этот фрагмент кода.

Фрагмент N9

do{
  ....
} while((action_result && action_result->is_ok()) || !action_result);

Предупреждение PVS-Studio: V728 [CWE-571] An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!action_result' and 'action_result'. recruitment.cpp 439

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

while(!action_result || action_result->is_ok());

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

Фрагмент N10

bool mouse_handler::mouse_button_event(const SDL_MouseButtonEvent& event, 
                                                          uint8_t button,
                                                        map_location loc, 
                                                              bool click)
{
  static const std::array<const std::string, 6> buttons = {       // <=
    "",
    "left",         // SDL_BUTTON_LEFT
    "middle",       // SDL_BUTTON_MIDDLE
    "right",        // SDL_BUTTON_RIGHT
    "mouse4",       // SDL_BUTTON_X1
    "mouse5"        // SDL_BUTTON_X2
  };

  if (gui().view_locked() 
    || button < SDL_BUTTON_LEFT || button > buttons.size()) {     // <=
    return false;
  } else if (event.state > SDL_PRESSED || !pc_.get_map().on_board(loc)) {
    return false;
  }

  if(game_lua_kernel* lk = pc_.gamestate().lua_kernel_.get()) {
    lk->mouse_button_callback(loc, buttons[button],               // <=
              (event.state == SDL_RELEASED ? "up" : "down"));
  ....
  }
....
}

Когда-то, лишь GitHub знает когда, был создан массив типа std::array, названный buttons. И не было беды, да только перед тем, как использовать массив, была написана проверка. Она должна была ограничить индекс при доступе к массиву buttons, но что-то явно пошло не так. Всему виной довольно частая ошибка, допускаемая программистами, которая известна как проблема неучтённой единицы.

Однако что сделано, то сделано... Ибо теперь ходит легенда, которая гласит, что придёт день, когда 2-й параметр button функции mouse_button_event будет иметь значение 6.

Предупреждение PVS-Studio: V557 Array overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 624

А когда это случится, и проверка будет пройдена, будет предпринята попытка из массива buttons получить строку по индексу 6, и тут же случится что-то очень страшное и неумолимо разрушительное, доселе невиданное в этом фрагменте кода...

Но всегда есть надежда, что появится славный герой, исправит проверку, изменив в выражении button > buttons.size() оператор > на >= и спасёт от возможных безумных последствий этот код!

Аналогичное предупреждение анализатора: V557 Аrray overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 633

Заключение

Вот и закончились предупреждения: их было немного, за что моё уважение всем разработчикам, принимавшим и принимающим участие в проекте! Код приятный и читаемый, всё на своём месте. Видно, что разработчики любят свою игру, ведь уже почти 20 лет продолжают разрабатывать и улучшать её, придумывать новые фишки.

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

Также я очень рад, что анализатор PVS-Studio смог показать достойный результат и нашёл прикольные ошибки, которые мне и, надеюсь, вам было интересно разбирать. А ещё надеюсь, что вы смогли найти для себя что-то интересное или новое и смогли продвинуться хоть чуточку дальше в изучении С++ и нюансов его работы!

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

Иии... как у нас уже исторически сложилось, предлагаю попробовать анализатор PVS-Studio. Для open source проектов предоставляется бесплатная лицензия.

Берегите себя, и всего доброго!