Why Even Legendary Games Like Wesnoth Hide Bugs in Plain Sight

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. 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: 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. Use the filtering settings to display the most interesting warnings. This will make reviewing the report quicker and easier. Explore how to configure the analysis process via the *.pvsconfig file. It simplifies customization, improves accuracy, speeds up setup, and unlocks additional features. 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). 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. 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. 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. initial configuration process Use the filtering settings to display the most interesting warnings. This will make reviewing the report quicker and easier. most interesting warnings Explore how to configure the analysis process via the *.pvsconfig file. It simplifies customization, improves accuracy, speeds up setup, and unlocks additional features. *.pvsconfig additional features 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). the mass suppression mechanism 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. incremental analysis mode 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. integrate 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 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; } 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. V1116 The PVS-Studio warnings: V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 146 V1116 charconv.hpp V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 148 V1116 charconv.hpp 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; } 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 charconv.hpp V1116 [CWE-778] Creating an exception object without an explanatory message may result in insufficient logging. charconv.hpp 161 V1116 charconv.hpp Fragment N2 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; } 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 V1048 gettext.cpp This code fragment shows the same value being assigned to the msgval variable. First, initialization by an expression occurs: msgval dgettext (domainname, msgid); dgettext (domainname, msgid); The dgettext function is essentially a wrapper over the dgettext function from the boost::locale library: dgettext dgettext boost::locale Namespace bl = boost::locale; .... std::string dgettext(const char* domain, const char* msgid) { return bl::dgettext(domain, msgid, get_manager().get_locale()); } 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. msgid 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: msgid ^ msgval = firsthat + 1; 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: 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; } 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 Fragment N3 void mp_create_game::sync_with_depcheck() { .... auto& game_types_list = find_widget ("game_types"); game_types_list.set_value( std::distance(level_types_.begin(), std::find_if(level_types_.begin(), level_types_.begin(), // ("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 V539 mp_create_game.cpp 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. 0 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. std::find_if level_types_.begin() std::find_if level_types_.begin() 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. std::distance level_types_.begin() level_types_.begin() std::find_if 0 The fixed code looks like this: void mp_create_game::sync_with_depcheck() { .... auto& game_types_list = find_widget ("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; }))); .... } void mp_create_game::sync_with_depcheck() { .... auto& game_types_list = find_widget ("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 Fragment N4 template class enable_lua_ptr { public: .... enable_lua_ptr& operator=(enable_lua_ptr&& o) { self_ = std::move(o.self_); *self_ = static_cast (this); } // class enable_lua_ptr { public: .... enable_lua_ptr& operator=(enable_lua_ptr&& o) { self_ = std::move(o.self_); *self_ = static_cast (this); } // <= .... } The PVS-Studio warning: V591 [CWE-393] Non-void function should return a value. lua_ptr.hpp 34 V591 lua_ptr.hpp As you may have already noticed, the move operator doesn't return a value, and boom! We catch undefined behavior. Fragment N5 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: '" red)"; } return ret; } 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: '" red)"; } return ret; } The PVS-Studio warnings: V560 [CWE-571] A part of conditional expression is always true: !ret.empty(). info.cpp 240 V560 info.cpp V646 [CWE-670] Consider inspecting the application's logic. It's possible that 'else' keyword is missing. info.cpp 240 V646 info.cpp 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. !ret.empty() true ret 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. else icon misc/blank-hex.png icon !ret.empty() Fragment N6 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; .... } 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 V1099 install_dependencies.hpp 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: install_dependencies modal_dialog install_dependencies window_id 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; Executing the constructor body of the install_dependencies class. 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; a call to the install_dependencies::window_id method; a constructor call to the modal_dialog class; an initialization of the addons_ data member; a call to the install_dependencies::window_id method; install_dependencies::window_id a constructor call to the modal_dialog class; modal_dialog an initialization of the addons_ data member; addons_ Executing the constructor body of the install_dependencies class. install_dependencies 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: 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. 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. 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. window_id install_dependencies window_id 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. installed_dependencies installed_dependencies::window_id here Fragment N7 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; .... } 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 V575 contexts.cpp 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. ratings std::fill_n 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. std::fill_n read in the documentation 6 ratings We can fix this by either swapping the 0 and 6 arguments or by adding explicit initialization when creating an array. 0 6 int ratings[6]{}; // explicitly initializes the array with zeros int ratings[6]{}; // explicitly initializes the array with zeros Fragment N8 Fragment N8 std::pair move_to_targets_phase:: choose_move(std::vector & targets) { std::vector ::iterator best_target = best_rated_target->tg; .... if(best != units_.end()) { LOG_AI get_location(), best_target->value); best_target = targets.end() - 1; // get_location(), best->get_location()); } LOG_AI (); } std::pair move_to_targets_phase:: choose_move(std::vector & targets) { std::vector ::iterator best_target = best_rated_target->tg; .... if(best != units_.end()) { LOG_AI get_location(), best_target->value); best_target = targets.end() - 1; // get_location(), best->get_location()); } LOG_AI (); } 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 V1001 ca_move_to_targets.cpp 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. best_target 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 Fragment N9 do{ .... } while((action_result && action_result->is_ok()) || !action_result); 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 V728 recruitment.cpp 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); 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. V728 Fragment N10 Fragment N10 bool mouse_handler::mouse_button_event(const SDL_MouseButtonEvent& event, uint8_t button, map_location loc, bool click) { static const std::array buttons = { // buttons.size()) { // 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")); .... } .... } bool mouse_handler::mouse_button_event(const SDL_MouseButtonEvent& event, uint8_t button, map_location loc, bool click) { static const std::array buttons = { // buttons.size()) { // 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. std::array buttons buttons "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. mouse_button_event button 6 The PVS-Studio warning: V557 Array overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 624 V557 mouse_events.cpp 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. buttons 6 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 >=! > button > buttons.size() >= The same analyzer warning: V557 Array overrun is possible. The 'button' index is pointing beyond array bound. mouse_events.cpp 633 V557 mouse_events.cpp 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. Heroes of Might and Magic 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. try our PVS-Studio analyzer free license Take care, and have a good day!