Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you haven't received our response, please do the following:
check your Spam/Junk folder and click the
"Not Spam" button for our message.
This way, you won't miss messages from our team in the future.
Sometimes you might wish to feel nostalgic and play your favorite old game, but certain things in such games may seem outdated. So, to breathe new life into an old project, some enthusiasts set the goal to recreate and improve its source code. We decided to use the PVS-Studio static analyzer and check how VCMI developers cope with this task.
VCMI Project is an open-source game engine for Heroes of Might and Magic 3. The VCMI engine is cross-platform and runs on Windows, Linux, Android, macOS, and iOS. The engine got an update, resulting in improved animations, increased performance, the updated graphical interface, fixed bugs, the improved mod system, and much more. You can find detailed information about the project here.
A large team of developers is regularly updating the project, and the number of new code lines is constantly increasing. In such circumstances, it is not a miracle that errors may occur in code. No matter how experienced a programmer is, mistakes can appear because of inattention or fatigue. Moreover, VCMI developers work on a non-commercial basis in their spare time.
The purpose of the article is not to offend developers. We just want to show how useful static analysis tools can be. They can greatly simplify developers' life and save users from possible bugs. This will not only reduce time for development and testing, but also help you avoid mistakes in the final product, which can spoil the pleasure when using it.
It's easier to use a static analyzer than it seems. For example, PVS-Studio provides a free version for open-source projects, and in the documentation you can see how to quickly integrate it into the development process.
Fans of the Heroes of Might and Magic series may also be interested in our article about Free Heroes of Might and Magic II.
Let's check how effective static analysis is and examine some code snippets which triggered the analyzer.
Before we proceed with the analysis, it is worth mentioning that the code is of high quality. In some places, developers even inserted superfluous checks to be on the safe side. Nevertheless, a lot of errors got into the code.
The analysis was carried out on the 10fc6ce commit.
The issue with memory in games is acute. Of course, nowadays Heroes of Might and Magic 3 is hardly a demanding game that runs at the limit of the available RAM. However, it is extremely unpleasant to see that some game takes up a few extra gigabytes from RAM in a couple of hours. Needless to say, no one likes memory leaks. Let's consider the following code fragments and analyzer warnings.
Fragment N1
CGObjectInstance *CMapLoaderH3M::readDwellingRandom(....)
{
auto *object = new CGDwelling();
CSpecObjInfo *spec = nullptr;
switch(objectTemplate->id)
{
case Obj::RANDOM_DWELLING:
spec = new CCreGenLeveledCastleInfo();
break;
case Obj::RANDOM_DWELLING_LVL:
spec = new CCreGenAsCastleInfo();
break;
case Obj::RANDOM_DWELLING_FACTION:
spec = new CCreGenLeveledInfo();
break;
default:
throw std::runtime_error("Invalid random dwelling format");
}
spec->owner = object;
....
object->info = spec;
return object;
}PVS-Studio warning: V773 The exception was thrown without releasing the 'object' pointer. A memory leak is possible. MapFormatH3M.cpp 1173
Here we see that if we get into the default branch of the switch statement, an exception will be thrown. In this case, the memory allocated by the operator new for the object pointer will leak.
To solve this problem, you can move the declaration of the object pointer after the body of the switch statement, but it is much better to use smart pointers. Here is a possible fix:
std::unique_ptr<CGDwelling> CMapLoaderH3M::readDwellingRandom(....)
{
std::unique_ptr<CSpecObjInfo> spec{ nullptr };
switch(objectTemplate->id)
{
case Obj::RANDOM_DWELLING:
spec = std::make_unique<CCreGenLeveledCastleInfo>();
break;
case Obj::RANDOM_DWELLING_LVL:
spec = std::make_unique<CCreGenAsCastleInfo>();
break;
case Obj::RANDOM_DWELLING_FACTION:
spec = std::make_unique<CCreGenLeveledInfo>();
break;
default:
throw std::runtime_error("Invalid random dwelling format");
}
auto object = std::make_unique<CGDwelling>();
spec->owner = object.get();
....
object->info = std::move(spec);
return object;
}Fragment N2
CTownHandler::CTownHandler():
randomTown(new CTown()),
randomFaction(new CFaction())
{
randomFaction->town = randomTown;
randomTown->faction = randomFaction;
randomFaction->identifier = "random";
randomFaction->modScope = "core";
}
CTownHandler::~CTownHandler()
{
delete randomTown;
}PVS-Studio warning: V773 The 'randomFaction' pointer was not released in destructor. A memory leak is possible. CTownHandler.cpp 282
In the member initializer list of the constructor, two pointers are initialized with the operator new, but in the destructor, memory is freed only by one pointer.
In the simplest case, the fixed code looks as follows:
CTownHandler::~CTownHandler()
{
delete randomFaction;
delete randomTown;
}However, you can replace simple pointers with smart ones (for example, with std::unique_ptr) and forget about such issues forever.
Fragment N3
template <class T>
void addModificator()
{
modificators.emplace_back(new T(*this, map, generator));
}PVS-Studio warning: V1023 A pointer without owner is added to the 'modificators' container by the 'emplace_back' method. A memory leak will occur in case of an exception. Zone.h 122
Here the situation is more interesting. The modificators container is a doubly linked list of smart std::unique_ptr pointers. Developers want to insert a new smart pointer "in-place" at the end of the list with the help of the emplace_back function and pass a raw pointer to an object of the T type that the operator new created.
Such code contains a potential problem. What if the std::bad_alloc exception will be thrown when a new node is created in std::list? That's right, here you will get a memory leak.
It's easy to fix the issue — use std::make_unique and replace emplace_back with push_back:
template <class T>
void addModificator()
{
modificators.push_back(std::make_unique<T>(*this, map, generator));
}Yes, an extra move constructor will be called in this code, but we eliminate the possibility of a memory leak.
The reader may also ask: "Why was it necessary to replace emplace_back with push_back"? The push_back function has a great advantage — it creates less work for the compiler, and therefore, less time is spent on compilation.
When you call push_back, the compiler only has to select an overload. When you call emplace_back, the compiler will both instantiate the function template and select an overload. So, both these functions will do the same. But this does not mean that we should always give priority to push_back. Use emplace_back when you can pass arguments to the constructor of the container element type. Otherwise, if your object has already been created or constructor arguments cannot be passed, the push_back function is more preferable.
In this section, we will look at code fragments that lead to undefined behavior. For those who are interested in UB, I also recommend the following article. Let's move on to the analyzer's warnings.
Fragment N4
void ApplyGhNetPackVisitor::visitTradeOnMarketplace(....)
{
....
bool allyTownSkillTrade = (....
&& gh.getPlayerRelations(player, hero->tempOwner)
&& ....);
if (hero && ....)
gh.throwAndComplain(&pack, "This hero can't use this marketplace!");
....
}PVS-Studio warning: V595 The 'hero' pointer was utilized before it was verified against nullptr. Check lines: 182, 184. NetPacksServer.cpp 182
Here we see that the hero pointer is dereferenced before a check for nullptr. A similar warning:
Fragment N5
void Queries::popIfTop(QueryPtr query)
{
//LOG_TRACE_PARAMS(logGlobal, "query='%d'", query);
if(!query)
logGlobal->error("The query is nullptr! Ignoring.");
popIfTop(*query);
}PVS-Studio warning: V1004 The 'query' pointer was used unsafely after it was verified against nullptr. Check lines: 246, 249. CQuery.cpp 249
This case is more interesting. Here, developers log an error to handle the situation when query equals nullptr. However, after that, the program will continue execution, and the null pointer will be dereferenced, which leads to undefined behavior. Perhaps, it's better to interrupt the execution of the function after logging.
Fragment N6
void CCallback::trade(....)
{
....
pack.marketId = dynamic_cast<const CGObjectInstance *>(market)->id;
....
}PVS-Studio warning: V522 There might be dereferencing of a potential null pointer. CCallback.cpp 255
Here the result of the dynamic_cast operator is dereferenced. Since dynamic_cast can return nullptr, nullptr may be dereferenced.
The analyzer issued 112 warnings of the V522 diagnostic rule, and 100 of these warnings were associated with dynamic_cast. For every fifth warning, the code had a check for the resulting pointer using the assert macro, but this is not a panacea. The assert macro expands to an empty string in the release version. Therefore, we can say that no dynamic_cast result was checked.
You can say that the developers were confident in the returned result. However, we should remember that tomorrow the code may change, and suddenly dynamic_cast will start to return nullptr. The cost of an error in this case will be much more than an extra check.
Here is just a part of such warnings:
In this section, I'd like to consider unreachable code — code that will not be executed under any conditions in the program. Most often, such cases appear due to violations in the conditions, but there are cases when inattention leads to them.
So, here they are from left to right.
Fragment N7
size_t CTrueTypeFont::getGlyphWidth(const char *data) const
{
if (....)
return fallbackFont->getGlyphWidth(data);
return getStringWidth(....);
int advance;
TTF_GlyphMetrics(....);
return advance;
}PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. CTrueTypeFont.cpp 86
It's better to pay attention to this function: the code after return getStringWidth(....); will never be executed.
Fragment N8
CConnection::CConnection (....)
{
....
while (endpoint_iterator != end)
{
....
if (!error)
{
init();
return;
}
else
{
throw std::runtime_error(....);
}
endpoint_iterator++;
}
}PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. Connection.cpp 115
This fragment looks strange too. Developers try to iterate to a certain boundary, but the line with endpoint_iterator++ will never be executed. I could not figure out the author's idea, perhaps one of the readers will succeed.
Fragment N9
void CCreatureHandler::loadStackExp (....)
{
....
switch (mod[0])
{
....
case 'D':
b.type = BonusType::ADDITIONAL_ATTACK; break;
case 'f':
b.type = BonusType::FEARLESS; break;
case 'F':
b.type = BonusType::FLYING; break;
case 'm':
b.type = BonusType::MORALE; break; // <=
b.val = 1;
b.valType = BonusValueType::INDEPENDENT_MAX;
break;
....
}
}PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. CCreatureHandler.cpp 1094
In case 'm', a code part will not be executed. As we see, the code lines above are very similar. We can conclude that this error appeared as a result of copy-paste.
Fragment N10
bool Animation::loadFrame(size_t frame, size_t group)
{
....
if (....)
{
if (defFile)
{
auto frameList = defFile->getEntries();
if (....)
{
....
return true;
}
}
return false;
// still here? image is missing
printError(frame, group, "LoadFrame");
images[group][frame] = std::make_shared<QImage>("DEFAULT");
}
....
}PVS-Studio warning: V779 Unreachable code detected. It is possible that an error is present. Animation.cpp 545
In this fragment, everything that is after return false; will not be executed. Judging by the comment in the code, the developers are aware of this, and such code is only a temporary measure.
The following case is the most interesting in my opinion.
Fragment N11
std::string CComponent::getSubtitleInternal()
{
....
if (val)
return ....;
else
return val > 1 ? creature->getNamePluralTranslated()
: creature->getNameSingularTranslated();
....
}PVS-Studio warning: V547 Expression 'val > 1' is always false. CComponent.cpp 218
The control flow can get into the else branch only if val == 0. Accordingly, val > 1 will never return true, and creature->getNamePluralTranslated() is unreachable code.
It is worth noting that the developers left a comment that this function should be fixed. However, they either did not have time to do it (by the time when the article was written), or they forgot. In any case, with a static analyzer one will notice such an error when writing a function and immediately correct it.
Fragment N12
void deallocate_segment(....)
{
....
if (seg_index >= first_block)
{
segment_element_allocator_traits::deallocate(....);
}
else
if (seg_index == 0)
{
elements_to_deallocate = first_block > 0 ? this->segment_size(first_block)
: this->segment_size(0);
....
}
}PVS-Studio warning: V547 Expression 'first_block > 0' is always true. concurrent_vector.h 669
Let's figure out what's going on here. If the first check fails, then seg_index < first_block. OK, let's keep this in mind. Now, if seg_index == 0, then first_block > 0. After that, we again check that first_block > 0, and the analyzer indicates that this condition is always true. Therefore, the this->segment_size(0) expression will never be executed. It may be difficult for a person to trace such a logical chain, but this is easy for a soulless machine.
The symbolic execution technology in the analyzer detects such errors. If you are interested what it is and how it works, I suggest you refer to this article: PVS-Studio Static Code analysis technologies.
In this section, we will look at a more conceptual error when the assignment operator is incorrectly overloaded.
Fragment N13
ObjectTemplate & ObjectTemplate::operator=(const ObjectTemplate & rhs)
{
....
width = rhs.width;
height = rhs.height;
visitable = rhs.visitable;
blockedOffsets = rhs.blockedOffsets;
blockMapOffset = rhs.blockMapOffset;
visitableOffset = rhs.visitableOffset;
usedTiles.clear();
usedTiles.resize(rhs.usedTiles.size());
for(size_t i = 0; i < usedTiles.size(); i++)
std::copy(....);
return *this;
}PVS-Studio warning: V794 The assignment operator should be protected from the case of 'this == &rhs'. ObjectTemplate.cpp 88
The analyzer tells us that an object cannot be assigned to itself. When the usedTiles.clear() function is called, we will erase the usedTiles data member of our object. As a result, resize will make the container size equal to 0, and nothing will be copied (and there is nothing left to copy). It turns out that a part of the object will be lost.
You can solve the problem in one of the following ways:
This section contains warnings that cannot be called errors, because they do not lead to serious consequences. Some of the fragments may be an author's idea, which only the author can confirm; while some fragments are unnecessary actions that are better avoided.
Let's move on to situations where developers made redundant checks either because of logical expressions that were incorrectly calculated or as a safety net.
Fragment N14
void BattleStacksController::updateBattleAnimations(uint32_t msPassed)
{
....
if (hadAnimations && currentAnimations.empty())
{
//stackAmountBoxHidden.clear();
owner.executeStagedAnimations();
if (currentAnimations.empty())
owner.onAnimationsFinished();
}
....
}PVS-Studio warning: V547 Expression 'currentAnimations.empty()' is always true. BattleStacksController.cpp 384
The nested check is always true. Perhaps it was necessary to check another container or the check can even be removed.
Fragment N15
std::vector<BattleHex> CStack::meleeAttackHexes(....)
{
....
int mask = 0;
....
if (....)
{
if ((mask & 1) == 0)
{
mask |= 1;
res.push_back(defenderPos);
}
}
....
}PVS-Studio warning: V547 Expression '(mask & 1) == 0' is always true. CStack.cpp 262
The mask variable is declared equal to zero and then does not change in code till the check. The mask was probably modified earlier, but it does not change after refactoring.
Fragment N16
bool CGarrisonSlot::mustForceReselection() const
{
....
if (!creature || !selection->creature)
return false;
....
if (!owner->removableUnits)
{
if (selection->upg == EGarrisonType::UP)
return true;
else
return creature || upg == EGarrisonType::UP;
}
}PVS-Studio warning: V560 A part of conditional expression is always true: creature. CGarrisonInt.cpp 284
At the very beginning of the code fragment, there was already the if (!creature) check and the exit from the function. Accordingly, the last return will always return true.
Fragment N17
std::optional<BattleAction> CBattleAI::considerFleeingOrSurrendering()
{
....
if (!bs.canFlee || !bs.canSurrender)
{
return std::nullopt;
}
auto result = cb->makeSurrenderRetreatDecision(bs);
if (!result && bs.canFlee && bs.turnsSkippedByDefense > 30)
{
return BattleAction::makeRetreat(bs.ourSide);
}
....
}PVS-Studio warning: V560 A part of conditional expression is always true: bs.canFlee. BattleAI.cpp 837
It is similar to the previous one, although it looks more like a safety net if the bs.canFlee data member changes after the first check (although it cannot change during the cb->makeSurrenderRetreatDecision(bs) function call).
A similar warning:
Fragment N18
void ApplyOnServerNetPackVisitor::visitLobbySetCampaign(LobbySetCampaign & pack)
{
....
if ( !isCurrentMapConquerable
|| (isCurrentMapConquerable && i == *pack.ourCampaign->currentMap))
{
srv.setCampaignMap(i);
}
....
}PVS-Studio warning: V728 An excessive check can be simplified. The '||' operator is surrounded by opposite '!isCurrentMapConquerable' and 'isCurrentMapConquerable' expressions. NetPacksLobbyServer.cpp 225
Here you can notice that the check can be simplified to the following:
if (!isCurrentMapConquerable || i == *pack.ourCampaign->currentMap)This section contains the rest of the exciting analyzer warnings, which are difficult to categorize by types of errors considered above.
Fragment N19
template <typename Handler>
void serialize(Handler &h, const int version)
{
h & x;
h & y;
h & w;
h & h;
}PVS-Studio warning: V501 There are identical sub-expressions to the left and to the right of the '&' operator: h &h Rect.h 158
This is a member function of the Rect class, which contains these data members: x, y, w, h. Obviously, the operator & is overloaded. However, it is not possible to see how, since the function is a template. In this case, there is an object of the Handler type to the left and right of the & operator, but not the h data member of the Rect class. So, it is difficult to say for sure whether it is intended or not.
For example, the AIUtility.h file contains the following function on line 74:
template<typename Handler>
void serialize(Handler & h, const int version)
{
h & this->h;
h & hid;
h & name;
}It explicitly specifies the this->h data member. I reviewed a couple of such functions (there are many of them), and there is no object of the Handler type that is simultaneously used with the operator & on the left and on the right.
Fragment N20
void CArmedInstance::updateMoraleBonusFromArmy()
{
....
//-1 modifier for any Undead unit in army
const ui8 UNDEAD_MODIFIER_ID = -2;
....
}PVS-Studio warning: V569 Truncation of constant value -2. The value range of unsigned char type: [0, 255]. CArmedInstance.cpp 123
The UNDEAD_MODIFIER_ID constant has the unsigned char type. The analyzer warns that the variable will be truncated. As a result of this assignment, the UNDEAD_MODIFIER_ID variable will be equal to 254. Perhaps this is what the author of the code intended. However, it is better to replace the value with a more meaningful one for better code readability.
Fragment N21
void CGameHandler::endBattleConfirm(....)
{
....
for (int i = 0; i < cs.spells.size(); i++)
{
names << "%s";
if (i < cs.spells.size() - 2)
names << ", ";
else if (i < cs.spells.size() - 1)
names << "%s";
}
....
}PVS-Studio warning: V658 A value is being subtracted from the unsigned variable. This can result in an overflow. In such a case, the '<' comparison operation can potentially behave unexpectedly. Consider inspecting the 'i < cs.spells.size() - 2' expression. CGameHandler.cpp 760
Here cs.spells.size() returns a value of the size_t type. If the cs.spells container contains only 1 object, then an overflow will occur in the cs.spells.size() – 2 expression, and i will be compared with the maximum number that can be stored in size_t. Therefore, "%s, " will be written in the stream.
Perhaps this was intended, or developers are sure that there is always more than 1 object in the container, but the code looks peculiar.
And the last case:
Fragment N22
CGameState::CrossoverHeroesList
CGameState::getCrossoverHeroesFromPreviousScenarios()
const
{
....
crossoverHeroes.heroesFromAnyPreviousScenarios =
crossoverHeroes.heroesFromPreviousScenario = heroes;
crossoverHeroes.heroesFromPreviousScenario = heroes;
....
}PVS-Studio warning: V519 The variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1203, 1204. CGameState.cpp 1204
Here heroes are assigned to crossoverHeroes.heroesFromPreviousScenario twice in a row. Perhaps it's better to save heroes in another variable.
The article does not contain the entire list of detected errors, as some of them were in third-party libraries. Although such errors are still in VCMI, I did not consider them in the article, because VCMI developers are not directly related to them.
To prevent such warnings, PVS-Studio allows you to exclude selected directories or files from the analysis, and that's what I did. I found that the developers checked the project with the Coverity analyzer, but the information about the last check refers to April 14, 2018. Unfortunately, we don't know whether the developers are using any static analyzer now.
We can say that the PVS-Studio solution would help avoid many dangerous and dubious places in code and, thus, protect players from possible bugs in one of the best strategies of the 20th century.
0
0
0
55
0
0