PVS-Studio
Published in

PVS-Studio

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.

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

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

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

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

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

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

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

“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:

Possible NullReferenceException

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

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

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

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

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.

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

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:

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

Always false

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

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

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:

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!

--

--

--

PVS-Studio is a tool for detecting bugs and security weaknesses in the source code of programs, written in C, C++, C# and Java. It works under 64-bit systems in Windows, Linux and macOS environments, and can analyze source code intended for 32-bit, 64-bit and embedded ARM platfor

Recommended from Medium

Automation is Easy — Reading Interactive PDF using PyPDF2 : AIE2

Contributing to Open Source : Week 2

Adding Sensors to the Web of Things

[CouchDB] Hyperledger Fabric Account-based Wallet Java Chaincode

I recently enrolled for hng internship.

How to “think OOP” — The game of Classerole

Google CTF 2020 (Flag Shop)

Make a cool emoji display box

Get the Medium app

A button that says 'Download on the App Store', and if clicked it will lead you to the iOS App store
A button that says 'Get it on, Google Play', and if clicked it will lead you to the Google Play store
Unicorn Developer

Unicorn Developer

The developer, the debugger, the unicorn. I know all about static analysis and how to find bugs and errors in C, C++, C#, and Java source code.

More from Medium

Active-Passive Background Service In .Net 6

DFrame — Distributed load testing framework for writing test scenarios in C#

PVS-Studio 7.19: what’s new?

Advanced Data Modeling with Realm .NET