# Checking Barotrauma with the PVS\-Studio static analyzer

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](https://import.viva64.com/docx/blog/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](https://store.steampowered.com/app/602960/Barotrauma/)\. Also, the developers published the source code on [GitHub](https://github.com/Regalis11/Barotrauma) so that the community can develop more complex mods and find bugs\.

## Analysis results

### Errors in if

[V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'string\.IsNullOrEmpty\(EndPoint\)' to the left and to the right of the '\|\|' operator\. BanList\.cs 41

```cpp
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](https://pvs-studio.com/en/blog/posts/cpp/0509/) about this if you haven't already\.

[V3004](https://pvs-studio.com/en/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. ServerEntityEventManager\.cs 314

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'args\.Length < 2' is always false\. DebugConsole\.cs 2183

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'newPrice \> 0' is always true\. DebugConsole\.cs 3310

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3005/) The 'arrowIcon\.PressedColor' variable is assigned to itself\. ChatBox\.cs 164

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3005/) The 'Penetration' variable is assigned to itself\. Attack\.cs 324

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3025/) Incorrect format\. A different number of format items is expected while calling 'Format' function\. Arguments not used: t\.Character\.Name\. DebugConsole\.cs 1123

```cpp
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:

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

### Possible NullReferenceException

[V3153](https://pvs-studio.com/en/docs/warnings/v3153/) Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Voting\.cs 181

```cpp
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](https://pvs-studio.com/en/blog/posts/csharp/0832/)\.

[V3027](https://pvs-studio.com/en/docs/warnings/v3027/) The variable 'spawnPosition' was utilized in the logical expression before it was verified against null in the same logical expression\. LevelObjectManager\.cs 274

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'level' object was used before it was verified against null\. Check lines: 107, 115\. BeaconMission\.cs 107

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3106/) Possibly index is out of bound\. The '0' index is pointing beyond 'Sprites' bound\. ParticlePrefab\.cs 303

```cpp
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\. 

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3107/) Identical expression 'power' to the left and to the right of compound assignment\. RelayComponent\.cs 150

```cpp
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:

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

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



```cpp
power = MaxPower + currPowerConsumption;
```

### Always false

[V3009](https://pvs-studio.com/en/docs/warnings/v3009/) It's odd that this method always returns one and the same value of 'false'\. FileSelection\.cs 395

```cpp
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 _return_s should return _true_\.

### Lost value 

[V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Trim' is required to be utilized\. GameServer\.cs 1589

```cpp
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:

```cpp
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](https://pvs-studio.com/en/blog/)\!