# 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\. ![0994_Stride/image1.png](https://import.viva64.com/docx/blog/0994_Stride/image1.png) ## 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](https://github.com/stride3d/stride/releases/tag/releases%2F4.1.0.1734)\. ## Stride Game Engine error review ### Anonymous function is used to unsubscribe from an event **Issue 1** ```cpp private void RemoveEditor(....) { .... if (....) { multiEditor.OpenedAssets.CollectionChanged -= (_, e) => MultiEditorOpenAssetsChanged(multiEditor, e); .... } .... } ``` PVS\-Studio warning: [V3084](https://pvs-studio.com/en/docs/warnings/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** ```cpp private void SetIsVisible(bool isVisible) { if (isVisible) PreviewService.OnShowPreview(); // <= else PreviewService.OnHidePreview(); // <= } ``` PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/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: ```cpp private IAssetPreviewService PreviewService { get { if (previewService != null) return previewService; previewService = ServiceProvider.TryGet(); 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: ```cpp private void SetIsVisible(bool isVisible) { if (PreviewService != null) // <= { if (isVisible) PreviewService.OnShowPreview(); else PreviewService.OnHidePreview(); } } ``` ### Redundant value assignment **Issue 3** ```cpp public TexAtlas(...., TexImage atlas) : base(....) { .... Name = atlas.Name; .... Name = ""; } ``` PVS\-Studio warning: [V3008](https://pvs-studio.com/en/docs/warnings/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** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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** ```cpp private async Task Update() { while (....) { .... if (....) { .... materialFilterRenderFeature.MaterialFilter = (materialFilterRenderFeature != null && ....)? .... .... } } } ``` PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/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** ```cpp public Palette GlobalPalette { get {....} set { SetTagValue("GlobalPalette", (value != null) ? null : value.Data); } } ``` PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/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: ```cpp (value != null) ? value.Data : null ``` ### Same conditional statements with a different purpose **Issue 7** ```cpp 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](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\.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: ```cpp 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** ```cpp private GraphViewModel(....) : base(serviceProvider) { .... if (rootPresenters == null) throw new ArgumentNullException(nameof(rootPresenters)); .... if (rootPresenters == null) throw new ArgumentNullException(nameof(rootNode)); .... } ``` PVS\-Studio warning: [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\. 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: ```cpp if (rootNode == null) throw new ArgumentNullException(nameof(rootNode)); ``` ### Copy paste errors or which case operator is the third wheel? **Issue 9** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ```cpp case VectorComponent.W: value.W = realPoint.Y; break; ``` ### Don't spare code to handle NullReferenceException **Issue 10** ```cpp 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](https://pvs-studio.com/en/docs/warnings/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: ```cpp child?.Arrange(....); ``` At the same time, they probably didn't pay attention to the second access to the variable: ```cpp 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](https://pvs-studio.com/en/pvs-studio/try-free/)\. But first, we recommend you to learn the basics of using the PVS\-Studio analyzer\. Check the [documentation](https://pvs-studio.com/en/docs/) for more info\. I hope you enjoyed reading this article :\) Good luck and clean code to you\! Thank you and see you soon\!