Unicorn in space: checking the source code for 'Space Engineers'



    As you already understood from the title, the article will focus on suspicious places found in the source code for 'Space Engineers'. But the format of the article is slightly different from the rest. In addition to information about the project, an overview of some of the suspicious places and errors found, as well as ways to fix them, I included a small section in the text about the correct scenario for using a static analyzer. I strongly recommend that you familiarize yourself with it, as many developers do not know or simply do not think about how to properly use the tools of this class. As a result, static analysis tools are used an order of magnitude less efficiently than they could.

    A little bit about the game




    Space Engineers is a sandbox about the design and maintenance of space objects. Players can build spaceships and stations of various sizes and destinations (civilian and military), control them and extract resources. The game uses a realistic physics engine: any object can be assembled, disassembled, damaged and completely destroyed. The game is based on the VRage 2.0 graphics engine of its own development company-creator Keen Software House .

    The source code of the game is available in the repository on GitHub .

    The source code is written in C # and checked using the PVS-Studio static code analyzer . You can check the project you are interested in by downloadingand trying the analyzer on your own or someone else's code.

    A general list of errors found and the projects in which they were identified can be found at the appropriate link .

    Erroneous and suspicious places


    Below are some warnings that the analyzer issued, as well as code fragments that it considered suspicious. I repeat that these are not all warnings. More details about the total number of errors and why not all of them were considered are written in the corresponding section.

    But you already want to take a look at the code, right? Well, let's get started.

    Typos and inattentive 'copy-paste'


    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

    A typical error that occurs in both C ++ and C # code and, probably, in many other programming languages. The reason for such errors, as a rule, is a simple carelessness. Compare the property 'property.Name' with string literals and compare twice with 'AxisScale'. Most likely, a comparison with another literal was implied (in neighboring methods, the same property is compared with the 'LimitAngle' literal, so I think it was implied here).

    No less typical errors of identical bodies of the operator 'if' were encountered in 'then' and 'else'-blocks. Such errors arise due to inattention (including due to inattentive copy-paste ). Consider several similar 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

    Regardless of the value of the variable 'm_removeAfterDeath', another variable - 'm_deathCountdownMs' - will be assigned the same value. In this case, it is difficult to say what needs to be changed. But the fact that the code contains an error is obvious.

    The following similar snippet:

    
    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

    The situation is similar to the previous one, using the 'if' operator in this code does not make sense. And again, it’s hard to say how to fix this code. Perhaps, depending on the condition, it is necessary to use either the operator '==' or '! ='. But this is only an assumption.

    And one more similar situation:

    
    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

    Depending on the condition, the intensity of the glare needs to be changed. But due to copy-paste, it will always be the same. What value you need to set in this or that case again you will not say, it is up to the authors of the code.

    Loss of return values


    There is code in projects that does not use the return value of a method. This may be due, for example, to the fact that the programmer forgot that the 'Replace' method of the 'String' class returns the changed string, and the original one remains untouched, since the objects of the 'String' class are immutable. In this project, we also encountered 2 errors related to the loss of the value returned by the method:

    
    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 :
    • V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 72
    • V3010 The return value of function 'Format' is required to be utilized. Sandbox.Game MyEntity3DSoundEmitter.cs 74

    The static method 'Format' of the class 'String' composes the resulting string based on the format string and the arguments that form it, and then returns the resulting string. Therefore, calling this method without using the return value does not make sense.

    It can be seen from this code that if some elements could not be found, it is necessary to write error information to the log. And the last two calls to the 'string.Format' method should be arguments to the 'MySandboxGame.Log.WriteLine' method.

    The correct code might look like this:

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

    Incorrect check after using the 'as' operator


    In my other articles on checking C # projects (' Checking the source code for a set of C # /. NET components from Sony ', ' Looking for errors in MonoDevelop ') I mentioned that among some errors that C # programmers make, certain patterns. And with each new proven project, I am more and more convinced of this. One of them is casting the object using the 'as' operator to a compatible type, and after that, checking for the 'null' is not the received object, but the original one, which raises the probability of throws of the type 'NullReferenceException'. 'Space engineers' did not stand aside.

    Let's look at a few examples with similar errors:

    
    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 work correctly if 'builder' is equal to 'null'. Then 'Assert' will work and everyone will be happy (relatively, of course). If 'builder' is of type 'MyObjectBuilder_WeaponBlockDefinition' - again, everything is fine. But if 'builder' does not have a value of 'null', but at the same time, as a result of the cast, the object 'ob' will have a value of 'null' - the test 'Debug.Assert (builder! = Null)' will succeed, and below, when using the object 'ob' will throw an exception of type '

    I chewed in some detail when the code worked correctly, and when it didn’t, so as not to repeat these explanations later. But, I think, it is obvious that such code contains an error.

    A 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 it was not possible to cast the 'item' object to the 'MyToolbarItemActions' and' actionsItem 'types, checking the' item! = Null 'will not help, because the wrong object is being checked, and an exception of the' NullReferenceException type is possible below '.
    Then it would be correct to fix the check for this:

    
    if (actionsItem != null)

    A few warnings with similar cases:
    • V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'ob', 'objectBuilder'. Sandbox.Game MyBlockNavigationDefinition.cs 172
    • V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'Owner', 'character'. Sandbox.Game MyWelder.cs 232

    Suspicious comparisons


    In the PVS-Studio 6.01 version, in addition to the newly added ones, existing diagnostics were improved, and some of them were very significant. An example of one of these diagnostics is V3022 , which determines that a condition will always be true or false.
    I propose to consider several fragments of such code marked 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 variable 'bagEntityId' will have the same type. Primitive types like 'long' can be compared with 'null' (long_var == null), but the result of this comparison will always be 'false'. As a result, the body of the 'if' statement will always be executed. Most likely, the nullable type was expected here - 'long?'.

    This is not the only such case; there were still places where primitive significant types were compared with 'null'. Relevant analyzer warning:

    • V3022 Expression 'info.WorkshopId == null' is always false. Sandbox.Game MyGuiBlueprintScreen.cs 326
    • V3022 Expression 'info.SteamIDOwner == null' is always false. Sandbox.Game MyGuiBlueprintScreen.cs 328
    • V3022 Expression 'result! = Null' is always true. Sandbox.Game MyGpsCollection.cs 293

    There are other interesting code snippets:

    
    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 This is

    also a very interesting case. There is a ternary operator in the code, but there is no sense in it. In the condition of the if statement, it is checked that 'i == 0', and then, when the 'settings' object is initialized, this condition is checked again. It would be logical, but between these sections of the code, the 'i' does not change in any way, as a result - there is no point in checking, the 'settings' will be constantly initialized with the same value.

    In the same cycle, there were more warnings:

    • V3022 Expression 'i == 0? true: grid.IsStatic 'is always true. Sandbox.Game MyGridClipboardAdvanced.cs 808
    • V3022 Expression 'i == 0' is always true. Sandbox.Game MyGridClipboardAdvanced.cs 808

    The code met several dozen such warnings. It makes no sense to consider all of them here, if you wish, you can download the project source code and the analyzer and check the project yourself (there are links to the code and analyzer at the beginning of the article). Fortunately, the project is collected and analyzed simply and quickly. You will kill several birds with one stone - you will try the analyzer, you will see the benefits of using such tools, and at the same time get acquainted with the source code of the project.

    And again about dereferencing null references


    Despite the fact that C # is much safer in terms of accessing null references than dereferencing null pointers in C ++ (leading to UB ), unexpectedly throwing exceptions like 'NullReferenceException' is still extremely unpleasant, especially if these exceptions appear in users and do not show yourself at the development stage. Therefore, if it is possible to dereference a null reference, you need to be extremely careful:

    
    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

    There is a need to take some action if 'MySession.ControlledEntity == null' or 'MySession.ControlledEntity.Entity' is a type compatible with 'MyCharacter'. But since the verification of these conditions is in the wrong order, an exception may occur. It will be generated if 'MySession.ControlledEntity == null', since 'Entity' is an instance property. The solution is to reorder the subexpressions:

    
    if (MySession.ControlledEntity == null ||    
        MySession.ControlledEntity.Entity is MyCharacter)


    Strange cycles


    Errors with cycles also occur in the code, for example, when the body of the cycle is never executed, it will always be executed strictly once, or it will be executed endlessly. There are many reasons, and they are all different. I suggest taking a look at one of these cycles:

    
    internal static void 
    AddDivisionForCullingStructure(List roList, 
                                   int objectCountLimit, 
                                   List 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 with the value 'bestAxis', however, the output condition is the same or a smaller value, because of which no loop iteration will be performed. It was understood that the counter should be initialized to 0, then the correct loop would look like this:

    
    for (int axis = 0; axis <= bestAxis; axis++)
    

    No less interesting example with a loop:

    
    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 is that the 'continue' statement is outside the 'then' branch of the 'if' statement, because of which it will always be executed. This means that all code following this statement (more than 10 lines) will never be executed. The solution to the problem is obvious - you need to add the 'continue' operator to the condition.

    Other warnings


    I repeat that in the articles I do not write out all the warnings that the analyzer gives on the project source code. It would take too much time to analyze all the warnings, the article in terms of volume would become excessively large, and reading it would be tedious. But you are probably wondering how many suspicious places in the code the analyzer found? At the time of writing, the statistics were as follows:

    • 75 high level alerts;
    • 92 warnings of medium importance;
    • 817 warnings of low importance;

    It is necessary to study all warnings of a high level of importance and at least look at the average level. You should not think that warnings of a low level of importance do not contain anything interesting, just more specific diagnostics are located at this level. In any case, it’s worth to familiarize yourself with the list of these messages, since it is possible that it contains exactly the specific diagnostics that your project needs.

    About the benefits of static analysis and how to use a static analyzer correctly




    Unfortunately, we have to face the fact that many people do not know how to use a static analyzer correctly.

    It is often believed that the following option is acceptable: they downloaded the analyzer, checked the project before release, corrected something, forgot about the analyzer. Oh, release soon! They remembered about the analyzer, checked the project, corrected something, forgot.

    This is the most incorrect use case. Errors made at the time of development are not immediately detected by the static analyzer, but lie in the code. Some of them are found by the compiler, some by the programmer himself, and some by testers. And only what remains after all this is found by the static analyzer, when they nevertheless decide to resort to his help. This requires a lot of efforts of different people, and still there is a high probability of missing something. Worse still, the longer the error is in the code, the more expensive it will eventually be.

    If the analyzer were used regularly, most errors would be corrected at the development stage, which would facilitate the task for both the programmer and testers.

    It is possible that the static analyzer will find too many errors, and then they simply will not be corrected. This could be avoided in 2 ways:

    • All the same regular launches of the analyzer and timely error correction. If there are few errors initially and they can be corrected, this should be done, as well as timely monitoring of the correction of new ones. If many errors were immediately found, and there is no way to fix them all, you should consider the second use case;
    • Hiding (fixing) existing errors and correcting only newly appearing ones. This will prevent new errors from appearing and at least not increase the number of existing errors. The fixed errors are corrected gradually, during which the total number of errors is gradually reduced from zero to the fixed value. More: practice of using the PVS-Studio analyzer .

    From the foregoing, a simple conclusion follows - a static analyzer is a tool for regular use, and not a one-time use. It is in this scenario of work that it fully reveals its potential, allowing you to eliminate errors at the earliest stages of their appearance, when the cost of correction is still low.

    Conclusion




    To summarize, I will not talk about the quality of the source code, whether a good project or a bad one is subjective, all views may differ. You can make some impression on the basis of the numbers (number of warnings) and code fragments given in the article. You will get a more complete picture by independently checking the project and looking at the messages, which I highly recommend doing. This will allow you to get a more detailed picture of the quality of the code, as well as get to know the static analyzer closer, the information about the use of which I tried to convey to you in this article.



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Unicorn in Space: Analyzing the Source Code of 'Space Engineers' .

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

    Also popular now: