Little adventure in pursuit of errors. The Battle for Wesnoth!

In this article, we'll tell you about our journey across the Irdya lands. Our adventures promise glorious battles, victories, and rare rewards of mighty artifacts! "What on earth are those artifacts?" you may ask. Well, these are bugs found in the code of a well-known, highly addictive game, The Battle for Wesnoth.

About the project

The Battle for Wesnoth is an open-source, turn-based strategy game set in a high fantasy world.

It combines stunning pixel graphics and clever gameplay with many engaging mechanics. Most importantly, the game has never crashed or thrown an error during the six months I've been playing it non-stop (just to write this article, of course).

If you're over 30 and somehow missed this amazing game but had a feature phone back in 2004, you might remember a game like Ancient Empire. The Battle for Wesnoth is its better version!

Well, it's not fair to compare this wonderful game to a 50 MB mobile game from back then. Anyway, I highly recommend it!

The game was released on June 18, 2003, and received over 70% positive reviews on Steam. The project is alive and kicking, and it's constantly evolving: developers just released update 1.19.12 on June 5. Meanwhile, the GitHub repository is updated daily with new changes.

As a hardcore programmer, of course, I built the game from source. By the way, thanks to the thoughtful developers' instructions, the process is smooth and hassle-free. It's amazing how much they've put in—despite the numerous project dependencies, the game is built just with few commands. All we need to do is start it and enjoy!

In case you're wondering, here's the commit I used to clone the project: febf638.

The inevitable spoiler

The article purpose isn't to devalue the work of programmers involved in the development of this product. Its purpose is to popularize static code analyzers that are useful even for high-quality and established projects.

Integrating PVS-Studio into a project at the analysis stage

The latest version of PVS-Studio 7.37 was recently released. The release is packed with all kinds of changes. We've worked hard to increase the number of useful diagnostic rules and minimize false positives.

However, there's an issue: even if the number of false positives is reduced, warnings can still confuse new users who haven't configured the analyzer yet. This is especially true for large projects, where some diagnostic rule groups may be irrelevant or unnecessary for users.

Some may have only recently begun exploring the realm of static analysis. To ensure a positive experience when using the analyzer for the first time, you may consider the following recommendations:

1. Complete the initial configuration process. As previously mentioned, it's recommended to disable any unnecessary diagnostic rule groups and keep only the necessary ones. For example, if the project doesn't work with MISRA or AUTOSAR standards, there's no reason to keep these diagnostic rule groups enabled.
2. Use the filtering settings to display the most interesting warnings. This will make reviewing the report quicker and easier.
3. Explore how to configure the analysis process via the *.pvsconfig file. It simplifies customization, improves accuracy, speeds up setup, and unlocks additional features.
4. Streamline your workflow with reports by using the mass suppression mechanism via suppress files. This mechanism suppresses both specific warnings for a particular file and all warnings in general, ensuring that only new warnings are received (i.e., the baseline).
5. Use the incremental analysis mode instead of performing the full analysis each time. It analyzes only modified files and issues warnings only for new code.
6. Integrate the full project analysis into CI during nightly builds. If you use the "pull/merge requests" model in your development process, you can also integrate analysis there.

Of course, we don't usually go through all these steps to check open-source projects for writing articles. Usually, we disable third-party libraries and enable only diagnostic rules for general purposes and micro-optimizations. This approach clearly reveals the state of the project codebase. After choosing the most interesting warnings, we can share the results with you.

All right, let's roll up our sleeves and explore bugs—I have some for you!

Analysis results

Fragment 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;
}

We'll start our adventure with a small but very serious issue. Can you spot it? If not, you may have never experienced situations when a user contacts technical support with an issue. Instead of providing a clear description, the user sends something like, "Everything's broken/stopped/closed," followed by, "No, it didn't show any errors."

To elaborate, the code snippet throws two exceptions without any message. This turns debugging into a guessing game.

Oh, how much it hurts... My team has experienced this pain not too long ago. To prevent this from happening again, we've created the special V1116 diagnostic rule for cases like this.

The PVS-Studio warnings:

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

The above function isn't the only one that throws an exception with an empty string. You can see a similar example in the same file:

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;
}

The PVS-Studio warnings:

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

Fragment 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;
}

The PVS-Studio warning: V1048 [CWE-1164] The 'msgval' variable was assigned the same value. gettext.cpp 440

This code fragment shows the same value being assigned to the msgval variable. First, initialization by an expression occurs:

dgettext (domainname, msgid);

The dgettext function is essentially a wrapper over the dgettext function from the boost::locale library:

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

This function instantly translates strings or messages. It takes the original message—or the msgid parameter, which is the key representing it—as an input and returns a translated string on the specified language standard.

If the function returns msgid, then no phrase translation was found. In such cases, developers check whether the original message started with the ^ character. If so, they represent the substring following that character as an output:

msgval = firsthat + 1;

So, now we know how the code works, but it isn't clear why the msgval variable is re-assigned the msgid value. Since this action is redundant, let's simplify the code a bit:

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;
}

Such strange things usually sneak in during refactoring. So, I recommend developers to take a closer look at this code.

Fragment 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;
                               })));
  ....
}

The PVS-Studio warning: V539 Consider inspecting iterators which are being passed as arguments to function 'find_if'. mp_create_game.cpp 534

This code fragment deserves a place in the hall of fame. It's a great example of how to evaluate 0 in a clever way, and I think everyone should see it.

Here's how it works. The std::find_if function is called with two identical iterators as its first two arguments, both pointing to the beginning—level_types_.begin(). The std::find_if function won't iterate at all. It won't find anything and will return the second argument, which, in this case, is the same level_types_.begin() starting iterator.

Then, the std::distance function will calculate the distance between the first argument, the level_types_.begin() iterator, and the level_types_.begin() iterator returned by the std::find_if function. As a result, we get 0.

The fixed code looks like this:

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;
                               })));
  ....
}

Fragment 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);
  }                                  // <=
....
}

The PVS-Studio warning: V591 [CWE-393] Non-void function should return a value. lua_ptr.hpp 34

As you may have already noticed, the move operator doesn't return a value, and boom! We catch undefined behavior.

Fragment 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;
}

The PVS-Studio warnings:

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

According to the developer's idea, most likely, if there's no logo or title picture for a particular add-on in the add-on manager, the default picture will be set.

The analyzer prompts us to look at the issue: the second part of the !ret.empty() will always return true. This happens due to the previous opposite check with assigning a string literal to the ret variable.

In addition, the analyzer hints that the programmer may have accidentally omitted the else keyword between the first and the second conditions. It looks like it should be there: if icon is an empty string, the function will return misc/blank-hex.png. The same should happen if icon is a non-empty string, but the file doesn't exist at that path. However, the !ret.empty() check is unnecessary, so we can simply remove it.

Fragment 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;
....
}

The PVS-Studio warning: 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

Thanks to this code snippet, I can tell you more about undefined behavior.

As we can see above, the install_dependencies class is derived from the modal_dialog class. In the install_dependencies constructor, the base class is initialized with the value returned by (wait for it...) the non-static window_id function. So, this will happen:

1. Executing the initialization list:
   • a call to the install_dependencies::window_id method;
   • a constructor call to the modal_dialog class;
   • an initialization of the addons_ data member;
2. Executing the constructor body of the install_dependencies class.

This means that a function of an object whose class hasn't been initialized yet will be called! This violates the following rule of the standard:

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.

But wait, there's more! As you may have noticed, the window_id member function is virtual and overridden in the install_dependencies class. Issues may arise later when a programmer writes a derived class where window_id will be overridden.

When an object of this derived class is created and the installed_dependencies constructor is executed, there's no information about the existence of the new override yet. So, the installed_dependencies::window_id function will always be called in the initialization list. This may differ from the developers' original intention. You can read more about it here.

Fragment 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;
....
}

The PVS-Studio warning: V575 [CWE-628] The 'fill_n' function processes '0' elements. Inspect the second argument. contexts.cpp 987

Most likely, the developers wanted to fill the ratings array with zeros, but something went wrong. So, when the std::fill_n function is called, the array won't be initialized.

The issue is that the second and third arguments were mixed up when std::fill_n was called. As we know—or have just read in the documentation—the second parameter specifies how many times the value should be written to the array, while the third represents the value. In other words, the current code outputs 6 into the ratings array exactly zero times.

We can fix this by either swapping the 0 and 6 arguments or by adding explicit initialization when creating an array.

int ratings[6]{}; // explicitly initializes the array with zeros

Fragment 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>();
}

The PVS-Studio warning: 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

The analyzer has detected that, at the end of the function, the local best_target variable is assigned a value but never used afterward. Most likely, this indicates an error: judging by the variable name, it appears to be an important part of the local AI logic.

For now, it may be intentional, and the world isn't ready for a Jarvis-level combat AI, like the one in the Iron Man movies. However, I can neither confirm nor deny it. So, my advice to the developers is to take a closer look at this code snippet.

Fragment N9

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

The PVS-Studio warning: V728 [CWE-571] An excessive check can be simplified. The '||' operator is surrounded by opposite expressions '!action_result' and 'action_result'. recruitment.cpp 439

The shown code fragment doesn't contain an error, but the analyzer recommends us simplify the expression in the loop condition as follows:

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

The code is more concise now. I find warnings from diagnostic rules, such as V728, very useful. They improve code readability, enabling developers to spend less time figuring out the code logic.

Fragment 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"));
  ....
  }
....
}

Once upon a time—only GitHub knows when—an array of the std::array type named buttons was created. Things were going smoothly, but a check was written before the array was used. It was supposed to limit the index when accessing the buttons array, but something obviously went wrong. This is due to a common error known as the "off-by-one" error.

Well, what's done is done... Legend says the day will come when the second parameter of the mouse_button_event function, button, will have a value of 6.

The PVS-Studio warning: V557 Array overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 624

When this happens and the check is passed, an attempt will be made to retrieve a string from the buttons array using the 6 index. Then, something terrible and relentlessly devastating will happen that has never been seen before in this code fragment.

However, there's always hope that a hero will come and save this code from possible crazy consequences by changing the > operator in the button > buttons.size() expression to >=!

The same analyzer warning: V557 Array overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 633

Conclusion

It's time to wrap up the article. There were a few warnings, so kudos to all the developers—past and present—who have contributed to the project! The code is easy to read and well-organized. It's clear that the developers deeply love their game. They've been developing and enhancing it for almost 20 years, always coming up with new features.

In fact, the project reminds me of Heroes of Might and Magic, a marvelous fantasy world and a magical turn-based adventure—I'm hooked on this game, too. Well, fight fire with fire, you know: I'm going to play World of Warcraft.

I'm also happy that PVS-Studio analyzer showed decent results and detected some interesting errors that I—and hopefully you too—found exciting. I hope you picked up something interesting or new and were able to learn more about C++ and its nuances!

As always, we'll send the bug report to the developers and hope that all the errors will get fixed, making the codebase even stronger.

And as a tradition, I recommend you to try our PVS-Studio analyzer. We offer a free license for open-source projects.

Take care, and have a good day!