As you've already figured out from the title, in this article we are going to discuss suspicious fragments found in the source code of the 'Space Engineers' project. The format of this article, however, is not quite the same as the rest of our articles. This time, in addition to the project description, review of selected bugs and suspicious fragments, and ways to fix them, I've included a small section where I talk about how to use a static analyzer in a proper way. I strongly recommend reading this section because many developers don't know or simply have never pondered how to use this type of tools right - the result is that static analysis tools are used ten times less effectively than they could.
Space Engineers is a sandbox game about engineering, constructing, and maintaining creations in space. Players build space ships, space stations, planetary outposts of various sizes and uses (civil and military), pilot ships and travel through space to explore planets and gather resources to survive. Space Engineers features a realistic, volumetric-based physics engine: everything in the game can be assembled, disassembled, damaged and destroyed. Space Engineers is the first title fully utilizing VRAGE 2.0, an in-house game engine developed by Keen Software House.
The game's source code is available at the repository at GitHub.
The project is written in C# and was analyzed with PVS-Studio static code analyzer. Feel free to download and test the analyzer on your own project or a third party project.
For a complete list of projects analyzed so far and the bugs found in those projects, follow this link.
Further in this article, we will discuss some of the bugs and suspicious fragments reported by the analyzer. Again, remember that it's not a complete list of all the warnings. To see the total number of errors found and to learn why we don't discuss every issue we find, see the corresponding section.
But I know you can't wait to start, so here we go.
void DeserializeV0(XmlReader reader)
{
....
if (property.Name == "Rotation" ||
property.Name == "AxisScale" ||
property.Name == "AxisScale")
continue;
....
}PVS-Studio warning: V3001 There are identical sub-expressions 'property.Name == "AxisScale"' to the left and to the right of the '||' operator. Sandbox.Graphics MyParticleEmitter.cs 352
It's a typical error found in code written in C++, C#, and, I bet, many other programming languages. Errors like that are usually caused by mere lack of attention. The programmer was comparing the 'property.Name' property with string literals and mistakenly compared it with 'AxisScale' twice. They apparently meant to compare the property with a different literal the second time (in other methods nearby, the same property is compared with literal 'LimitAngle', so I guess it was meant in our case, too).
Another typical bug pattern found in the project has to do with identical 'then' and 'else' blocks of an 'if' statement. Such errors occur because of inattention (including careless use of copy-paste). Here are a few examples:
private void StartRespawn()
{
m_lastCountdownTime = MySandboxGame.TotalGamePlayTimeInMilliseconds;
if (m_removeAfterDeath)
m_deathCountdownMs = AgentDefinition.RemoveTimeMs;
else
m_deathCountdownMs = AgentDefinition.RemoveTimeMs;
}PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyAgentBot.cs 260
No matter what value the 'm_removeAfterDeath' variable refers to, another variable, 'm_deathCountdownMs', will be assigned with one and the same value. I can't say for sure what exactly must be fixed in this code. But there is obviously a bug in it.
Another similar example:
private static bool IsTriangleDangerous(int triIndex)
{
if (MyPerGameSettings.NavmeshPresumesDownwardGravity)
{
return triIndex == -1;
}
else
{
return triIndex == -1;
}
}PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyNavigationTriangle.cs 189
This case is similar to the previous one; the 'if' statement makes no sense here. Again, I'm not sure to how to fix this code. Perhaps the programmer wanted either operator '==' or '!=' to be used depending on the condition, but it's only my guess.
Another similar example:
public void UpdateLight()
{
....
if (((MyCubeGrid)Parent).GridSizeEnum == MyCubeSize.Large)
Light.GlareIntensity = 0.5f + length * 2;
else
Light.GlareIntensity = 0.5f + length * 2;
....
}PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. Sandbox.Game MyThrust.cs 149
Glare intensity must be changed depending on the condition, but it will stay the same because of copy-paste. What value must be set in either case is, again, something that only the code authors can know.
When analyzing projects, we sometimes come across the code where methods' return values are not used. It happens, for example, when programmers forget that the 'Replace' method of class 'String' returns a modified string while the original one remains unchanged since objects of the 'String' class are immutable. In this project, we found two errors related to loss of methods' return values:
public void Init(string cueName)
{
....
if (m_arcade.Hash == MyStringHash.NullOrEmpty &&
m_realistic.Hash == MyStringHash.NullOrEmpty)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find any sound for '{0}'", cueName));
else
{
if (m_arcade.IsNull)
string.Format(
"Could not find arcade sound for '{0}'", cueName);
if (m_realistic.IsNull)
string.Format(
"Could not find realistic sound for '{0}'", cueName);
}
}PVS-Studio warnings:
Static method 'Format' of class 'String' composes the resulting string based on the format string and arguments that form it and returns that string. Therefore, calling to this method without using its return value doesn't make sense.
As seen from this code, an error message must be written into the log if some of the elements can't be found. The last two calls to method 'string.Format' are to be passed as arguments to method 'MySandboxGame.Log.WriteLine'.
This is what a correct version of the code could look like:
if (m_arcade.IsNull)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find arcade sound for '{0}'", cueName));
if (m_realistic.IsNull)
MySandboxGame.Log.WriteLine(string.Format(
"Could not find realistic sound for '{0}'", cueName));In some of my other articles on analysis of C# projects (Sony C#/.Net component set analysis, Looking for bugs in MonoDevelop), I mentioned that I started noticing certain patterns of mistakes made by C#-programmers. Every new project I scan makes me more and more convinced, that this observation is true. One of those patterns is casting an object to a compatible type using the 'as' operator and then checking the original object, instead of the new one, for 'null'. This mistake increases the risk of getting a 'NullReferenceException'. 'Space Engineers' has this bug too.
Here are a few examples of errors of this type:
protected override void Init(MyObjectBuilder_DefinitionBase builder)
{
base.Init(builder);
var ob = builder as MyObjectBuilder_WeaponBlockDefinition;
Debug.Assert(builder != null);
WeaponDefinitionId = new MyDefinitionId(ob.WeaponDefinitionId.Type,
ob.WeaponDefinitionId.Subtype);
ResourceSinkGroup = MyStringHash.GetOrCompute(ob.ResourceSinkGroup);
InventoryMaxVolume = ob.InventoryMaxVolume;
}PVS-Studio warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'builder', 'ob'. Sandbox.Game MyWeaponBlockDefinition.cs 21
This code will execute correctly if 'builder' equals 'null': in that case, 'Assert' will execute and everyone will be happy (relatively, of course). If 'builder' is of type 'MyObjectBuilder_WeaponBlockDefinition', it's OK too. But if the value of 'builder' is other than 'null' while the 'ob' object's value becomes 'null' as a result of the cast, the 'Debug.Assert(builder != null)' check will execute successfully but then, when attempting to use the 'ob' object, an exception of 'NullReferenceException' type will be generated.
The reason why I elaborate on scenarios when the code works correctly and when it doesn't, is to avoid repeating these explanations again in the future. Anyway, it's obvious that there is a bug in this code.
Another similar error:
private void contextMenu_ItemClicked(MyGuiControlContextMenu sender,
MyGuiControlContextMenu.EventArgs args)
{
....
var actionsItem = item as MyToolbarItemActions;
if (item != null)
{
if (idx < 0 || idx >= actionsItem
.PossibleActions(ShownToolbar.ToolbarType)
.Count)
RemoveToolbarItem(slot);
....
}
....
}PVS-Studio warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'actionsItem'. Sandbox.Game MyGuiControlToolbar.cs 511
If the 'item' object can't be cast to type 'MyToolbarItemActions' and 'actionsItem', the 'item != null' check won't help since it checks a wrong object, and further execution of the code may result in a 'NullReferenceException'.
The fixed version of the check should look like this:
if (actionsItem != null)A few more similar warnings:
In the version PVS-Studio 6.01, besides adding new diagnostics, we have also improved the existing ones - some of them quite a lot. One of these is a diagnostic V3022, which detects conditions that are always true or false.
Let's discuss several such fragments, found by the analyzer:
private long SpawnInventoryContainer(MyDefinitionId bagDefinition)
{ ... }
public override void OnCharacterDead()
{
....
var bagEntityId = SpawnInventoryContainer(
Character.Definition.InventorySpawnContainerId.Value);
if (bagEntityId != null)
....
}PVS-Studio warning: V3022 Expression 'bagEntityId != null' is always true. Sandbox.Game MyCharacterInventorySpawnComponent.cs 60
Since the 'SpawnInventoryContainer' method returns an object of type 'long', the 'bagEntityId' variable will be of the same type. Primitive types like 'long' can be compared to 'null' (long_var == null), but such a comparison will always evaluate to 'false'. Therefore, the body of the 'if' statement will always execute. What's more likely is that nullable type 'long?' was expected here.
It's not the only example of this kind; there were some other fragments where primitive meaningful types were compared with 'null'. Here are the corresponding analyzer warnings:
Some of the issues are pretty interesting:
private new bool TestPlacement()
{
....
for (int i = 0; i < PreviewGrids.Count; ++i)
{
....
if (retval && i == 0)
{
....
var settings = i == 0 ?
m_settings.GetGridPlacementSettings(grid, false) :
MyPerGameSettings.BuildingSettings.SmallStaticGrid;
....
}
....
}
}PVS-Studio warning: V3022 Expression 'i == 0' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 790
There is a ternary operator in this code, but it's useless. The condition of the 'if' statement checks if 'i == 0', and then, when initializing the 'settings' object, this condition is checked once again. It would make sense if 'i' changed between the checks; but it doesn't, so the check is not needed and 'settings' will always be initialized with one and the same value.
There were two more warnings for this loop:
The analyzer output a few dozens of warnings like that for the code in general, and we won't discuss them all here. If you wish, you can download the project's source code and scan it yourself (see the download links for the source code and analyzer in the beginning). The project takes little time to build and analyze, so it won't be difficult. It will help you kill several birds with one stone: try the analyzer, feel the usefulness of such tools in practice, and get to know the project's source code better.
Despite the fact that using null references in C# is way less dangerous than null-pointers dereferencing in C++ (which leads to UB), it's still very unpleasant to get unexpected 'NullReferenceExceptions', especially if these exceptions manifest themselves at the user's side rather than during the development. So, you should be very careful whenever a null reference may get dereferenced:
new MyEntity Entity { get; }
private static bool EnergyCritWarningMethod(out MyGuiSounds cue,
out MyStringId text)
{
....
if (MySession.ControlledEntity.Entity is MyCharacter ||
MySession.ControlledEntity == null)
....
}PVS-Studio warning: V3027 The variable 'MySession.ControlledEntity' was utilized in the logical expression before it was verified against null in the same logical expression. Sandbox.Game MyHudWarning.cs 415
It does require certain actions when 'MySession.ControlledEntity == null' or 'MySession.ControlledEntity.Entity' is a type compatible with 'MyCharacter'. But since the checks of these conditions are put in a wrong order, an exception may occur. It will be raised if 'MySession.ControlledEntity == null' because 'Entity' is an instance property. The solution is to reorder the subexpressions:
if (MySession.ControlledEntity == null ||
MySession.ControlledEntity.Entity is MyCharacter)Some errors are found in loops: for example, a loop body never executes or executes exactly once or will be executing forever. There are many different reasons behind each scenario. Here's one of such loops:
internal static void
AddDivisionForCullingStructure(List<MyRenderObject> roList,
int objectCountLimit,
List<BoundingBoxD> resultDivision)
{
....
for (int axis = bestAxis; axis <= bestAxis; axis++)
....
}PVS-Studio warning: V3028 Consider inspecting the 'for' operator. Initial and final values of the iterator are the same. VRage.Render MyRender-Management.cs 1034
The loop counter ('axis') is initialized to the value of 'bestAxis', but because the same value (or less) is also used as the loop termination condition, the loop won't iterate at all. The programmer actually wanted the counter to start with 0, in which case the loop should be rewritten as follows:
for (int axis = 0; axis <= bestAxis; axis++)Here's another interesting example:
public override void Draw()
{
....
foreach (var flame in m_thrust.Flames)
{
if (m_thrust.CubeGrid.Physics == null)
continue;
....
if (m_landingEffect != null)
{
m_landingEffect.Stop(true);
m_landingEffect = null;
--m_landingEffectCount;
}
continue; // <=
....
if (m_landingEffect == null)
continue;
....
}
}PVS-Studio warning: V3020 An unconditional 'continue' within a loop. Sandbox.Game MyRenderComponentThrust.cs 109
The error here has to do with the 'continue' statement being placed outside the 'then' branch of the 'if' statement, and this mistake results in an infinite loop. It means that all the rest of the code following this statement (over 10 lines) will never execute. The solution is obvious - the 'continue' statement must be moved under the condition.
As I already said, I don't discuss every warning issued by the analyzer for the project source code; if I did, it would take just too much time and the articles would be huge and tiresome to read. But you may be wondering how many suspicious fragments in total have been found. Well, at the time I was writing this article, the figures were as follows:
Developers must examine all the first-level warnings and at least look through the second-level ones. It's not that third-level warnings are something trivial and uninteresting; it's just that they rather deal with ad-hoc diagnostics. Anyway, you should peek into the third level too because you may find some specific diagnostics there that may appear useful for your project.
Unfortunately, we often see that many developers don't know how to use a static analyzer in a right way.
They often stick to the following scenario, which they find normal: download an analyzer, run it on the project before the release, fix something, put the analyzer aside and forget about it. Oh, release is coming! Recall you have the analyzer, run it again, fix something, forget about it.
It's the worst scenario one may think up. Errors, which appeared in the code during the development, stay there instead of being caught by the static analyzer right off. Some of them are found by the compiler, others by the programmer, and others by the testers. What's left is found by the analyzer when you finally decide to use it. All this clean-up takes tons of effort of many people, and there's still a high risk that you've missed something serious. But an even worse thing is that the longer a bug stays in the code, the more expensive it becomes to fix it.
If the analyzer was used regularly, most of the bugs would be fixed as early as at the development stage, making the life of both the programmers and testers much easier.
Another possible scenario is when the tool outputs too many warnings and developers simply leave them in the way they are. There are two ways to deal with this problem:
There is a simple conclusion to draw from everything that was said above: a static analyzer is a tool that must be used regularly, not occasionally. It is the only way to get most from it and to be able to eliminate errors at the earliest stages, when the cost of bug-fixing is still low.
To sum up, I won't talk about the quality of the source code and whether the project is good or bad - these notions are subjective, and tastes differ, as you know. You can get some initial impression from the figures I gave you (the number of warnings) and code fragments we've discussed. But for a full understanding you need to scan the project and examine the warnings yourself. And this is what I strongly recommend doing: it will help you get a fuller picture of the code and its quality and get to know the analyzer better. And I hope my advice on the best practice of using the analyzer was helpful, too.