Top 3 open-source games written in C#: searching for bugs

• Introduction
• Third place. Thrive
  • Game overview
  • Error review
• Second place. Barotrauma
  • Error review
• First place. Space Station 14
  • Error review
• Conclusion

There are a lot of fantastic games, but few of them are open source. In this article, we will examine the most curious bugs found in the source code of three best, to my mind, open-source games written in C#.

Introduction

All of the games listed are available on Steam. I selected them based on the following criteria:

• the idea is original;
• the gameplay is enjoyable;
• the game is popular.

All the games are open-source projects, so why not examine their source code? Using the PVS-Studio analyzer, I found several errors in these projects. Let's take a look at these flaws. Learning from someone else's mistakes is handy. This way you make fewer errors yourself :)

Are you ready? Then let's get started!

Third place. Thrive

Game overview

Thrive is an evolution simulation game. The gameplay is divided into 7 stages across a species' complete evolution from a prokaryotic cell to space exploration. However, the game is still in development, and at the moment only the first of all stages is available — the microbe stage.

The game's concept is similar to Spore, but Thrive emphasizes realism.

To survive and thrive in this game, you need to:

• balance various substances in the bodies of your species;
• develop the species wisely, understand their energy capabilities and the peculiarities of their habitat;
• protect your species against predators or turn them into predators.

Otherwise, you may doom your species to extinction.

Error review

I checked Thrive 0.6.2 with the PVS-Studio analyzer.

Thrive is a fairly small project, and its code is well-debugged. The analyzer detected one small error. So, let's review it.

The expression is always false

private void SpawnMicrobe(Vector3 position)
{
  ....        
  cloudSystem!.AddCloud(   random.Next(0, 1) == 1 // <= 
                         ? phosphates 
                         : ammonia,
                        ....);
}

The PVS-Studio warning:

V3022. Expression 'random.Next(0, 1) == 1' is always false. MicrobeBenchmark.cs 520.

Note the random.Next(0, 1) method. It returns a random integer in the specified range. The range is set with arguments: the first argument is the left bound of the range, and the second argument is the right. However, developers overlooked a nuance: the left bound is included in the specified range, while the right bound is not. Thus, the random.Next(0, 1) method can return only one value — 0.

For this code to work as expected, we just need to increase the right bound to 2:

cloudSystem!.AddCloud(random.Next(0, 2) == 1 ? ....);

Second place. Barotrauma

Barotrauma is a multiplayer 2D submarine simulator with survival horror and RPG elements. The game takes place in the ocean of one of Jupiter's moons.

The primary objective of the game is to navigate your submarine to the nearest settlement while tackling numerous emergencies together with your crew.

In the game, you need to choose one of the roles: captain, engineer, mechanic, security officer, doctor, and assistant.

At the beginning of the game, roles help to distribute responsibilities among players and determine the characters' talents. These roles, however, do not limit the breadth of duties you can undertake. A doctor can learn how to navigate a submarine, as well as an assistant can shoot at the ominous creatures that lurk in the ocean's depths.

Teamwork and cooperation are the keys to victory in Barotrauma; without it, the survival in this murky underwater world is impossible.

Error review

I checked Barotrauma 0.17.12.0 with the PVS-Studio analyzer.

This project has more lines of code than the previous one. Thus, searching for errors becomes far more interesting.

By the way, we have analyzed Barotrauma before. If you're interested, you can check out the related article.

The pointless null check

public class Contact
{
  ....
  public Fixture FixtureA { get; internal set; }
  public Fixture FixtureB { get; internal set; }
  ....
  internal void Update(ContactManager contactManager)
  {
    Body bodyA = FixtureA.Body;
    Body bodyB = FixtureB.Body;

    if (FixtureA == null || FixtureB == null) // <=
      return;
    ....  
    if (sensor)
    {
      Shape shapeA = FixtureA.Shape;
      Shape shapeB = FixtureB.Shape;                
      touching = Collision.Collision.TestOverlap(...., 
                                                 ref bodyA._xf, 
                                                 ref bodyB._xf);
      ....
    }
  }
}

The PVS-Studio warnings:

• V3095. The 'FixtureA' object was used before it was verified against null. Check lines: 252, 255. Contact.cs 252.
• V3095. The 'FixtureB' object was used before it was verified against null. Check lines: 253, 255. Contact.cs 253.

In the above code, there is the FixtureA == null || FixtureB == null check, which indicates that FixtureA and FixtureB may have a null value. In this case, the check doesn't work since the exception is thrown earlier:

Body bodyA = FixtureA.Body; // possible NullReferenceException
Body bodyB = FixtureB.Body; // possible NullReferenceException

if (FixtureA == null || FixtureB == null)
  return;

It may seem that this check is redundant and there is no risk of NullReferenceException. However, the number of similar checks in other parts of the code suggests the opposite:

So, we have grounds to suspect that the problem exists. In this case, it would be much better to check both the FixtureA and FixtureB properties, as well as the FixtureA.Body and FixtureB.Body properties for null:

Body bodyA = FixtureA?.Body;
Body bodyB = FixtureB?.Body;

if (bodyA == null || bodyB == null)
  return;

The order of calls matters

public void RecreateSprites()
{
  ....
  if (_deformSprite != null)
  {
    _deformSprite.Remove();

    var source = _deformSprite.Sprite.SourceElement;

    _deformSprite = new DeformableSprite(source, ....);            
  }
  ....
  for (int i = 0; i < DecorativeSprites.Count; i++)
  {
    var decorativeSprite = DecorativeSprites[i];

    decorativeSprite.Remove();

    var source = decorativeSprite.Sprite.SourceElement;        // <=

    DecorativeSprites[i] = new DecorativeSprite(source, ....);          
  }
}

The PVS-Studio warning:

V3080. Possible null dereference. Consider inspecting 'decorativeSprite.Sprite'. Limb.cs 462.

In this case, the order of operations is wrong, which will result in NullReferenceException. It's clear from the implementation of the _deformSprite.Remove and decorativeSprite.Remove methods:

class DecorativeSprite : ISerializableEntity
{
  ....
  public Sprite Sprite { get; private set; }
  ....
  public void Remove()
  {
    Sprite?.Remove();
    Sprite = null;
  }
}

partial class DeformableSprite
{
  ....
  public Sprite Sprite { get; private set; }
  ....

  public void Remove()
  {
    Sprite?.Remove();
    Sprite = null;
    ....
  }
}

Both methods assign null to the Sprite properties. Further dereferencing of these properties will lead to an error.

To avoid the exception, just swap the calls of the Remove method and initializations of the source variables:

if (_deformSprite != null)
{
  var source = _deformSprite.Sprite.SourceElement; // <=
  _deformSprite.Remove(); // <=

  _deformSprite = new DeformableSprite(source, ....);            
}
....
for (int i = 0; i < DecorativeSprites.Count; i++)
{
  var decorativeSprite = DecorativeSprites[i];

  var source = decorativeSprite.Sprite.SourceElement; // <=
  decorativeSprite.Remove(); // <=

  DecorativeSprites[i] = new DecorativeSprite(source, ....);          
}

The expression is always false

private void UpdateAttack(float deltaTime)
{
  ....
  if (pathSteering != null) // <= (line 1551)
  { .... }
  else
  {
    ....
    if (pathSteering != null) // <= (line 1954)
    {
      pathSteering.SteeringSeek(....);
    }
    else
    {
      SteeringManager.SteeringSeek(....);
    } 
    ....
  }
  ....
}

The PVS-Studio warning:

V3022. Expression 'pathSteering != null' is always false. EnemyAIController.cs 1954.

Behind all the '....' in the method's implementation, there are numerous lines of code. The static analyzer clearly shows that this method has some oddities. Take a look at the above warning.

The analyzer reports that the pathSteering != null condition is always false. If you scroll the code 403 lines up, you will notice that this check is nested within the else block of the same check. As a result, the code inside the if block of the nested check is unreachable.

It's also worth noting that the pathSteering value does not change between these checks. You can see it for yourself.

First place. Space Station 14

Space Station 14 is an online multiplayer role-playing game. Even though it has simple 2D graphics, the game includes many gameplay mechanics that rely on role-based interaction between players.

The game is set on a space station where things are about to go wrong (maybe because of you, who knows). All workers of the station (players) are divided into 12 groups: command, security, medics, science, engineers, antagonists (bad guys), etc.

Each group consists of 2 to 14 roles, for a total of 66 roles in the game. Play your role and join forces with other players to work for the station's welfare or to make a mess.

The game has no level-ups, NPC, and in-game purchases. Each round of the game is a unique story that you and the other players create together.

Error review

I checked the latest (at the time of writing) available version of Space Station 14 with PVS-Studio.

Identical checks

private void OnSmokeSpread(....)
{
  if (.... || args.NeighborFreeTiles.Count == 0)
  {
     ....
     return;
  }
  ....
  if (args.NeighborFreeTiles.Count == 0 && ....) // <=
  {
    ....
  }
}

The PVS-Studio warning:

V3022 Expression 'args.NeighborFreeTiles.Count == 0 && args.Neighbors.Count > 0 && component.SpreadAmount > 0' is always false. SmokeSystem.cs 128

Two identical args.NeighborFreeTiles.Count == 0 checks are implemented. If the first check is true, the method returns. This way, the second check is always false when performed, and the code in the corresponding if block is unreachable.

The incorrect logical operator

private void OnStateChanged(...., MobStateChangedEvent args)
{
  if (   args.NewMobState != MobState.Dead 
      || args.NewMobState != MobState.Critical) // <=
  {
    return;
  }
  ....
}

The PVS-Studio warning:

V3022. Expression is always true. Probably the '&&' operator should be used here. ....\Content.Shared\DoAfter\SharedDoAfterSystem.cs 49

The analyzer reports that the given conditional expression is always true, so the following code is unreachable. The error occurs when the || operator is used instead of the && operator.

Here is another example:

public GasTankWindow(GasTankBoundUserInterface owner)
{
  ....
  _spbPressure = new FloatSpinBox
  {
    IsValid = f => f >= 0 || f <= 3000, // <=
    ....
  };
  ....
}

The PVS-Studio warning:

V3022. Expression 'f >= 0 || f <= 3000' is always true. Probably the '&&' operator should be used here. GasTankWindow.cs 168.

The analyzer reports that the f >= 0 || f <= 3000 expression is always true. It is, because if the value of f is greater than or equal to 0, then the first condition (f >= 0) is met, and if f is less than 0, then the second condition (f <= 3000) is met. Obviously, developers wanted to limit the value of the variable to a range of 0 to 3000. However, they needed to use the && operator instead of || in the conditional expression:

_spbPressure = new FloatSpinBox
{
  IsValid = f => f >= 0 && f <= 3000,
  ....
};

The anonymous function is used to unsubscribe from the event

protected override void Dispose(bool disposing)
{
  MenuBody.OnChildRemoved -= ctrl =>
    _uiController.OnRemoveElement(this, ctrl);
  ....
}

The PVS-Studio warning:

V3084. Anonymous function is used to unsubscribe from 'OnChildRemoved' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuPopup.xaml.cs 74.

In this case, the just-declared anonymous function is used to unsubscribe from the event. However, no handlers are unsubscribed, since the function is a separate delegate instance that is in no way connected to any of the event handlers. Even if the implementations of the function for unsubscribing and the handler are identical, they will still represent different objects.

Here are some more examples of similar errors found in this project:

public void OnRemoveElement(ContextMenuPopup menu, Control control)
{
  ....
  element.OnMouseEntered -= _ => OnMouseEntered(element);
  element.OnMouseExited -= _ => OnMouseExited(element);
  element.OnKeyBindDown -= args => OnKeyBindDown(element, args);
  ....
}

The PVS-Studio warnings:

• V3084. Anonymous function is used to unsubscribe from 'OnMouseEntered' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 205.
• V3084. Anonymous function is used to unsubscribe from 'OnMouseExited' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 206.
• V3084. Anonymous function is used to unsubscribe from 'OnKeyBindDown' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ContextMenuUIController.cs 207.

Conclusion

We reviewed the top 3 open-source games written in C# and the most interesting (but not all) bugs found by the static analyzer in their code.

By the way, you can try the PVS-Studio static analyzer for free.

That's all folks! I hope you enjoyed the article :)

Good luck and clean code to you! See you soon!