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.
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.
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:
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.
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);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.
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.
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;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.
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(';');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!