Stride Game Engine error review

Stride is a free, feature-packed and cross-platform game engine implemented in C#. Stride may certainly become an alternative to Unity, but what about the quality of its source code? Let's check Stride Game Engine with the help of the PVS-Studio static analyzer.

Introduction

Since Stride is a rapidly developing project, it not only acquires new functions and features, but inevitably gets more and more bugs. In this article we're inspecting the most curious ones. In fact, the analysis of errors in the project is an excellent way to assess the quality of its code. However, it's not that easy to check major projects of this kind, that's why we're going to use the PVS-Studio static code analyzer to spot bugs in Stride.

We checked the Stride Game Engine 4.1.0.1734 version. The source code is available on GitHub.

Stride Game Engine error review

Anonymous function is used to unsubscribe from an event

Issue 1

private void RemoveEditor(....)
{
  ....
  if (....)
  {
    multiEditor.OpenedAssets.CollectionChanged -= (_, e) => 
      MultiEditorOpenAssetsChanged(multiEditor, e);
    ....
  }
  ....
}

PVS-Studio warning: V3084. Anonymous function is used to unsubscribe from 'CollectionChanged' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. AssetEditorsManager.cs 444

In the case above, an anonymous function is used to unsubscribe from the CollectionChanged event. However, no handlers are unsubscribed, since an anonymous function is used. The declaration of an anonymous function creates a new delegate instance, different from the one previously subscribed to CollectionChanged. Even if these two functions have identical bodies, they still represent different objects. Thus, the hadler is not unsubscribed through an anonymous function and the value of CollectionChanged does not change.

Possible null dereference while accessing the property

Issue 2

private void SetIsVisible(bool isVisible)
{
  if (isVisible)
    PreviewService.OnShowPreview(); //<=
  else
    PreviewService.OnHidePreview(); //<=
}

PVS-Studio warning: V3080. Possible null dereference. Consider inspecting 'PreviewService'. PreviewViewModel.cs 109

In this case, the analyzer detected a possible null reference dereference. Pay attention to the SetIsVisible method. There's no check for null for the PreviewService property. The implementation of this property indicates the importance of the null check:

private IAssetPreviewService PreviewService
{
  get
  {
    if (previewService != null)
      return previewService;

    previewService = ServiceProvider.TryGet<IAssetPreviewService>();
    if (previewService == null)
      return null;                   

    previewService.PreviewAssetUpdated += PreviewAssetUpdated;
      return previewService;
  }
}

It's clear that the PreviewService property can actually return null. That's why the risk of null reference dereference is real. We can solve it by adding the condition:

private void SetIsVisible(bool isVisible)
{
  if (PreviewService != null)       //<=
  {
    if (isVisible)
      PreviewService.OnShowPreview(); 
    else
      PreviewService.OnHidePreview(); 
  }
}

Redundant value assignment

Issue 3

public TexAtlas(...., TexImage atlas) : base(....)
{
  ....
  Name = atlas.Name;
  ....
  Name = "";
}

PVS-Studio warning: V3008. The 'Name' variable is assigned values twice successively. Perhaps this is a mistake. TexAtlas.cs 48, 43

The Name variable is assigned values twice in the constructor. The developers may have decided to test the class functions with the empty Name property. That's why they added the second assignment but later forgot to delete it. Now, regardless of the constructor's input parameters, an empty string is always assigned for this property. This will inevitably break the logic of the program that uses the value of this property.

The null check is missed

Issue 4

public override void EndScreenDeviceChange(....)
{
  ....
  window.FullscreenIsBorderlessWindow = FullscreenIsBorderlessWindow; // <=
  if (....)
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
    }
    ....
  }
  else
  {
    ....
    if (window != null)
    {
      window.ClientSize = new Size2(clientWidth, clientHeight);
      window.Location = savedFormLocation;
      ....
      window.BringToFront();
    }

  }
  ....
}

PVS-Studio warning: V3095. The 'window' object was used before it was verified against null. GameWindowSDL.cs 64, 70.

At the beginning of the EndScreenDeviceChange method, the window object is used before it was checked for null. The analyzer's warning is relevant enough according to the rest of the code: the variable is accessed inside the conditional operator that checks for null.

When checking C# projects, the analyzer warns about potential errors that result in an exception of the NullReferenceException type far more often. It would be better to keep this in mind when coding. Let's check another similar warning*.*

Issue 5

private async Task Update()
{
  while (....)
  {
    ....
    if (....)
    {
      ....
      materialFilterRenderFeature.MaterialFilter =
        (materialFilterRenderFeature != null && ....)? ....
      ....
    }
  }
}

PVS-Studio warning: V3095. The 'materialFilterRenderFeature' object was used before it was verified against null. EditorGameRenderModeService.cs 106, 107.

In contrast to the previous case, there is a check for null, but it's... a bit odd. The MaterialFilter property of the materialFilterRenderFeature object is accessed. Then, the result of the ternary operator execution is assigned to this property. The operator's conditional expression checks materialFilterRenderFeature for null. But there's no need for this check. If materialFilterRenderFeature equals to null, the program execution results in NullReferenceException while accessing the MaterialFilter property.

When the check for null results in NullReferenceException

Issue 6

public Palette GlobalPalette
{
  get {....}
  set
  {
    SetTagValue("GlobalPalette", (value != null) ? null : value.Data);
  }
}

PVS-Studio warning: V3080. Possible null dereference. Consider inspecting 'value'. MetadataModels.cs 132

The ternary operator is passed as a second argument to the SetTagValue method. This operator checks the value variable for null. If value equals null, the value.Data *is passed as an argument to the method.* Accessing this variable's property will throw NullReferenceException. The developers may have mixed up the operands. Therefore, the correct implementation of the ternary operator may be as follows:

(value != null) ? value.Data : null

Same conditional statements with a different purpose

Issue 7

internal IObjectNode SetTarget(....)
{
  if (....)
  {
    var targetValue = targetNode.Retrieve();

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException(@"The type of the retrieved" +
                                           "node content does not match" +
                                           "the type of this reference");

    if (targetValue != null && !type.IsInstanceOfType(targetValue))
      throw new InvalidOperationException("TargetNode type does not " +
                                          "match the reference type.");

    ....
  }
  ....
}

PVS-Studio warning: 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.ObjectReference.cs 134

The analyzer detected two similar if statements with identical conditions. An exception is thrown in both conditional statements, thereby interrupting the program execution. It seems like there's nothing wrong here, just a bit of redundant code. However, pay attention to the exceptions' error messages, they are different. The developers probably intended to implement two different checks, that's why they have copied the check and changed the message but forgot to change the condition. Thus, one possible error remained unhandled and still poses a threat to the full and correct operation of the program.

According to the second exception's error message, the correct implementation of the second if condition is the following:

if (targetNode!= null && !type.IsInstanceOfType(targetNode))
  throw new InvalidOperationException("TargetNode type does not " +
                                      "match the reference type.");

Let's take a look at a similar error detected by the analyzer.

Issue 8

private GraphViewModel(....) : base(serviceProvider)
{
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootPresenters));
  ....
  if (rootPresenters == null) 
    throw new ArgumentNullException(nameof(rootNode));
  ....
}

PVS-Studio warning: 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. GraphViewModel.cs 40

Again, we have two if statements with identical conditions. As in the previous case, the arguments passed to the exception's constructor indicate the error. But in this case the arguments are the names of the variables checked. It's clear that the second conditional statement contains an error. Its correct implementation may look like this:

if (rootNode == null)
  throw new ArgumentNullException(nameof(rootNode));

Copy paste errors or which case operator is the third wheel?

Issue 9

public override void AddPoint(....)
{
  ....
  switch (Component)
  {
    case VectorComponent.X:
      value.X = realPoint.Y;
      break;

    case VectorComponent.Y:
      value.Y = realPoint.Y;
      break;

    case VectorComponent.Z:
      value.Z = realPoint.Y; // <=
      break;

    case VectorComponent.W:
      value.Z = realPoint.Y; // <=
      break;

    default:
      throw new NotSupportedException();
  }
  ....
}

PVS-Studio warning: V3139. Two or more case-branches perform the same actions. RotationComponentCurveViewModel.cs 53

Here is the switch statement that has two similar case operators. Both the case labels match the VectorComponent.{0} template, which can have any of the following properties instead of {0}: X, Y, Z, or W. The value.{0} = realPoint.Y assignment is executed in every case operator. Let's take a look at the last case operator. It has the VectorComponent.W label, and the value.Z = realPoint.Y operation is executed within the boundaries of the operator. This operation is identical to the assignement in the previous case operator. According to the template we identified, this statement should contain value.W instead of value.Z.

Thus, the correct implementation of the last case may be as follows:

case VectorComponent.W:
  value.W = realPoint.Y; 
  break;

Don't spare code to handle NullReferenceException

Issue 10

protected override Size ArrangeOverride(....)
{
  ....
  if (....)
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as TreeViewItem;
      ....
      if (child != null)
      {
        child.Arrange(....);
        currentY += child.ActualHeight;
      }
      ....
    }
  }
  else
  {
    for (....)
    {
      var child = itemsControl.ItemContainerGenerator
                              .ContainerFromIndex(i) as UIElement;

      child?.Arrange(....);
      currentY += child.DesiredSize.Height; //<=
    }
  }
  ....
}

PVS-Studio warning: V3125. The 'child' object was used after it was verified against null. VirtualizingTreePanel.cs 430, 429

Let's inspect then and else blocks of the conditional statement. They are somewhat similar, aren't they? In both blocks, the loop is executed, and the child variable is initialized through the as operator within the executed loop. However, there is one major difference: the child variable access is protected from NullReferenceException with the null check in the first loop, unlike the second loop. The developers may not have added the check for null for the second loop intentionally. That's why they processed the first access to the child variable with the null-conditional operator:

child?.Arrange(....);

At the same time, they probably didn't pay attention to the second access to the variable:

currentY += child.DesiredSize.Height;

This operation can still result in NullReferenceException.

Conclusion

The analyzer detected some potentially dangerous code fragments in the Stride Game Engine, that turned out to be pretty curious. But only developers can determine the actual errors in these code fragments by analyzing their project with the help of PVS-Studio.

In any case, no vulnerability is much better than having one. The static analyzer allows you to quickly find errors and thus minimize their number in the project without wasting a lot of time and effort searching for errors manually.

You can easily see it yourself and try the PVS-Studio trial version on your projects for free.

But first, we recommend you to learn the basics of using the PVS-Studio analyzer. Check the documentation for more info.

I hope you enjoyed reading this article :)

Good luck and clean code to you! Thank you and see you soon!