Pour obtenir une clé
d'essai remplissez le formulaire ci-dessous
Demandez des tariffs
Nouvelle licence
Renouvellement de licence
--Sélectionnez la devise--
USD
EUR
RUB
* En cliquant sur ce bouton, vous acceptez notre politique de confidentialité

Free PVS-Studio license for Microsoft MVP specialists
To get the licence for your open-source project, please fill out this form
** En cliquant sur ce bouton, vous acceptez notre politique de confidentialité.

I am interested to try it on the platforms:
** En cliquant sur ce bouton, vous acceptez notre politique de confidentialité.

Message submitted.

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.

>
>
>
Checking Barotrauma with the PVS-Studio…

Checking Barotrauma with the PVS-Studio static analyzer

31 Mar 2022

Barotrauma is an indie game where you can steer a submarine, hide from monsters, and even play the accordion to save your ship from going down. The Barotrauma project is developed by Undertow Games in collaboration with FakeFish. The source code is mainly written in C#. So, today, we're going to check it with the PVS-Studio static analyzer.

0930_Barotrauma/image1.png

Introduction

Barotrauma is a 2D co-op survival horror submarine simulator. You can play as a submarine captain, give orders, fix leaks and fight monsters.

Barotrauma is not an open-source project in the usual sense. The earlier version of the game is available for free, and you can find the current version on Steam. Also, the developers published the source code on GitHub so that the community can develop more complex mods and find bugs.

Analysis results

Errors in if

V3001 There are identical sub-expressions 'string.IsNullOrEmpty(EndPoint)' to the left and to the right of the '||' operator. BanList.cs 41

public bool CompareTo(string endpointCompare)
{
  if (string.IsNullOrEmpty(EndPoint) || string.IsNullOrEmpty(EndPoint)) 
  { return false; }
  ....
}

The EndPoint value is checked twice. Seems like the developer forgot to change the EndPoint parameter to endpointCompare when copying the string.IsNullOrEmpty method. Developers often make errors in comparison functions. Do read my colleague's article about this if you haven't already.

V3004 The 'then' statement is equivalent to the 'else' statement. ServerEntityEventManager.cs 314

public void Write(Client client, IWriteMessage msg, 
                  out List<NetEntityEvent> sentEvents)
{
  List<NetEntityEvent> eventsToSync = null;
  if (client.NeedsMidRoundSync)
  {
    eventsToSync = GetEventsToSync(client);
  }
  else
  {
    eventsToSync = GetEventsToSync(client);
  }
  ....
}

The if branch contains the same value as the else branch. Perhaps the developers should remove the else branch or change its behavior.

The analyzer issued two warnings for the following code fragment:

  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless DebugConsole.cs 2177
  • V3022 Expression 'args.Length < 2' is always false. DebugConsole.cs 2183
private static void InitProjectSpecific()
{
  ....
  AssignOnClientRequestExecute(
    "setclientcharacter",
    (Client senderClient, Vector2 cursorWorldPos, string[] args) =>
    {
      if (args.Length < 2)
      {
        GameMain.Server.SendConsoleMessage("....", senderClient);
        return;
      }

      if (args.Length < 2)
      {
        ThrowError("....");
        return;
      }
    );
  ....
}

This code fragment contains two identical checks. If the condition of the first if is met, the method terminates. Otherwise, both then branches will not be executed.

Thus, the GameMain.Server.SendConsoleMessage method will send the message, but the ThrowError method won't work. It's better to merge two if bodies or change the condition of the second one.

V3022 Expression 'newPrice > 0' is always true. DebugConsole.cs 3310

private static void PrintItemCosts(....)
{
  if (newPrice < 1)
  {
    NewMessage(depth + materialPrefab.Name + 
    " cannot be adjusted to this price, because it would become less than 1.");
    return;
  }

  ....

  if (newPrice > 0)
  {
    newPrices.TryAdd(materialPrefab, newPrice);
  }
  ....
}

If newPrice is less than or equal to 0, the body of the first if is executed. After that, the execution of the method is completed. So, the condition of the second if will always be true. That's why the developers can add the body of the second if to the else branch of the first one or just remove it.

Typos

V3005 The 'arrowIcon.PressedColor' variable is assigned to itself. ChatBox.cs 164

public ChatBox(GUIComponent parent, bool isSinglePlayer)
{
  ....
  arrowIcon = new GUIImage(....)
  {
    Color = new Color(51, 59, 46)
  };
  arrowIcon.HoverColor = arrowIcon.PressedColor = 
  arrowIcon.PressedColor = arrowIcon.Color;
  ....  
}

The arrowIcon.PressedColor value is assigned to itself. At the same time, the GUIIMage class contains the SelectedColor property. It looks like the developer wanted to use it but made a typo.

V3005 The 'Penetration' variable is assigned to itself. Attack.cs 324

public Attack(float damage, 
              float bleedingDamage, 
              float burnDamage, 
              float structureDamage,
              float itemDamage, 
              float range = 0.0f, 
              float penetration = 0f)
{
   ....
   Range = range;
   DamageRange = range;
   StructureDamage = LevelWallDamage = structureDamage;
   ItemDamage = itemDamage;     
   Penetration = Penetration;                // <=
}

Another similar error. Here the developers wanted to initialize the properties of the object. However, instead of the penetration value, the Penetration variable gets the Penetration value.

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: t.Character.Name. DebugConsole.cs 1123

private static void InitProjectSpecific()
{
  AssignOnClientRequestExecute("traitorlist", 
      (Client client, Vector2 cursorPos, string[] args) =>
  {
    ....
    GameMain.Server.SendTraitorMessage(
     client, 
     string.Format("- Traitor {0} has no current objective.",            // <=
                   "",                                                   // <=
                   t.Character.Name),                                    // <=
     "",
     TraitorMessageType.Console);   
  });
}

"Traitor {0} has no current objective" suggests that {0} — the format specifier — should have contained t.Character.Name. However, the format specifier will contain an empty string.

The error seems like the result of an unsuccessful GameMain.Server.SendTraitorMessage copy-paste:

GameMain.Server.SendTraitorMessage(client, 
"There are no traitors at the moment.", "", TraitorMessageType.Console);

Possible NullReferenceException

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Voting.cs 181

public void ClientRead(IReadMessage inc)
{
  ....
  foreach (GUIComponent item in
           GameMain.NetLobbyScreen?.SubList?.Content?.Children)    // <=
  {
    if (item.UserData != null && item.UserData is SubmarineInfo) 
    {
      serversubs.Add(item.UserData as SubmarineInfo); 
    }
  }
  ....
}

If at least one component from GameMain.NetLobbyScreen?.SubList?.Content?.Children is null, the result of the entire expression will also be null. In this case, NullReferenceException will be thrown when elements are iterated in foreach.

You can read more about the ?. operator in foreach in this article.

V3027 The variable 'spawnPosition' was utilized in the logical expression before it was verified against null in the same logical expression. LevelObjectManager.cs 274

private void PlaceObject(LevelObjectPrefab prefab, 
                         SpawnPosition spawnPosition, 
                         Level level, Level.Cave parentCave = null)
{
  float rotation = 0.0f;
  if (   prefab.AlignWithSurface 
      && spawnPosition.Normal.LengthSquared() > 0.001f          // <=
      && spawnPosition != null)                                 // <=
  {
    rotation = MathUtils.VectorToAngle(new Vector2(spawnPosition.Normal.Y, 
                                                   spawnPosition.Normal.X));
  }
  ....
}

At first the LengthSquared method call for the Normal field of the spawnPosition variable happens. Then, it is compared with the specified value, and then the variable is checked for null. If spawnPosition is null, NullReferenceException occurs.

The simplest solution is to use a null check at the beginning of the condition.

V3095 The 'level' object was used before it was verified against null. Check lines: 107, 115. BeaconMission.cs 107

public override void End()
{
  completed = level.CheckBeaconActive();                        // <=
  if (completed)
  {
    if (Prefab.LocationTypeChangeOnCompleted != null)
    {
      ChangeLocationType(Prefab.LocationTypeChangeOnCompleted);
    }
    GiveReward();
    if (level?.LevelData != null)                               // <=
    {
      level.LevelData.IsBeaconActive = true;
    }
  }
}

At first, the completed variable gets the level.CheckBeaconActive value. Then, the ?. operator is used in level?.LevelData. In this case, we have two possible outcomes: if level is null — a NullReferenceException will be thrown; if level is not null — the check is redundant.

Out of bounds

V3106 Possibly index is out of bound. The '0' index is pointing beyond 'Sprites' bound. ParticlePrefab.cs 303

public ParticlePrefab(XElement element, ContentFile file)
{
  ....
  if (CollisionRadius <= 0.0f) 
    CollisionRadius = Sprites.Count > 0 ? 1 : 
                                          Sprites[0].SourceRect.Width / 2.0f;
}

When the condition of the ternary operator is met, the value of the CollisionRadius variable becomes equal to 1. Otherwise, the Sprites.Count value equals to 0. And IndexOutOfRangeException occurs when the first element of the collection is called.

Earlier in the code, the collection is checked for being empty.

if (Sprites.Count == 0)
{
  DebugConsole.ThrowError($"Particle prefab \"{Name}\" in the file \"{file}\"
                            has no sprites defined!");
}

However, the DebugConsole.ThrowError method does not block the execution of further code. The developer should change the condition of the ternary operator.

Unnecessary actions

V3107 Identical expression 'power' to the left and to the right of compound assignment. RelayComponent.cs 150

public override void ReceivePowerProbeSignal(Connection connection, 
                                             Item source, float power)
{
  ....
  if (power < 0.0f)
  {
    ....
  }
  else
  {
    if (connection.IsOutput || powerOut == null) { return; }

    if (currPowerConsumption - power < -MaxPower)
    {
      power += MaxPower + (currPowerConsumption - power);
    }
  }
}

The programmer is trying to add MaxPower, power and the difference between currPowerConsumption and power. The expanded version of the expression will look as follows:

power = power + MaxPower + (currPowerConsumption - power);

There's no need to subtract the power variable from itself. The simplified code will look like this:

power = MaxPower + currPowerConsumption;

Always false

V3009 It's odd that this method always returns one and the same value of 'false'. FileSelection.cs 395

public static bool MoveToParentDirectory(GUIButton button, object userdata)
{
  string dir = CurrentDirectory;
  if (dir.EndsWith("/")) { dir = dir.Substring(0, dir.Length - 1); }
  int index = dir.LastIndexOf("/");
  if (index < 0) { return false; }
  CurrentDirectory = CurrentDirectory.Substring(0, index+1);

  return false;
}

Quite a strange method that always returns false. If the developers intended to write so, there's no error here. Otherwise, one of returns should return true.

Lost value

V3010 The return value of function 'Trim' is required to be utilized. GameServer.cs 1589

private void ClientWriteInitial(Client c, IWriteMessage outmsg)
{
  ....

  if (gameStarted)
  {
    ....

    if (ownedSubmarineIndexes.Length > 0)
    {
      ownedSubmarineIndexes.Trim(';');
    }
    outmsg.Write(ownedSubmarineIndexes);
  }
}

The Trim method does not change the ownedSubmarineIndexes value. That's why, it's useless to call it without saving the result. The correct code looks as follows:

ownedSubmarineIndexes = ownedSubmarineIndexes.Trim(';');

Conclusion

PVS-Studio found several errors, typos, and flaws in the Baratrauma source code. It's quite difficult to find them during code-review at the development stage.

Static analysis can help developers save the time they would have spent on finding and fixing bugs. And developers can devote this time to create new content. However, it's not enough to check the code once. Developers should regularly use analyzers to maximize the effect of static analysis.

If you want to learn about other projects checked by the PVS-Studio static analyzer — welcome to our blog!

Popular related articles
Why should Unity game developers use static analysis?

Date: 11 Mai 2022

Author: Artem Rovenskii

The cost of making a video game has increased over the years. Game development is becoming increasingly complex, the code base is getting larger as well. It's getting harder for developers to track b…
PVS-Studio static analyzer to recheck Unity

Date: 08 Avr 2022

Author: Artem Rovenskii

Unity is one of the most popular game engines. It helps create many excellent cross-platform projects. It's been 4 years since the last time we checked Unity's source code. Time has come again to see…
Playing with null: Checking MonoGame with the PVS-Studio analyzer

Date: 04 Fév 2022

Author: Vadim Kuleshov

The PVS-Studio analyzer often checks code of libraries, frameworks, and engines for game development. Today we check another project — MonoGame, a low-level gamedev framework written in C#.
Nintendo Switch: drop test of the Ryujinx emulator's source code

Date: 30 Jui 2021

Author: Danila Karpov

Each generation, companies like Sony, Microsoft and Nintendo delight their consumers with new consoles and different games for them. Yet there is a caveat - some games exclusively run on their platfo…
Unity projects analysis: the solution file has two projects named "UnityEngine.UI"

Date: 09 Jui 2021

Author: Sergey Vasiliev

While PVS-Studio analyses a Unity project, one may stumble upon such an error: Error was encountered while trying to open solution file '...': The solution file has two projects named "UnityEngine.UI…

Comments (0)

Next comments
Unicorn with delicious cookie
Nous utilisons des cookies pour améliorer votre expérience de navigation. En savoir plus
Accepter