# Playing with null: Checking MonoGame with the PVS\-Studio analyzer The PVS\-Studio analyzer often checks code of libraries, frameworks, and engines for game development\. Today we check another project — MonoGame, a low\-level gamedev framework written in C\#\. ![0915_MonoGame/image1.png](https://import.viva64.com/docx/blog/0915_MonoGame/image1.png) ## Introduction MonoGame is an open\-source framework for game development\. It's the heir of the [XNA](https://en.wikipedia.org/wiki/Microsoft_XNA) project, which was developed by Microsoft until 2013\. Let me also remind you about what [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/) is :\)\. PVS\-Studio is a static code analyzer that searches for various code errors and security\-related vulnerabilities\. I used PVS\-Studio version 7\.16 and [MonoGame sources](https://github.com/MonoGame/MonoGame) from 12\.01\.2022\. It's worth mentioning that the analyzer issued a couple of warnings on some libraries used in the project — DotNetZip and NVorbis\. I described them below\. If you want, you can easily [exclude third\-party code](https://pvs-studio.com/en/docs/manual/0014/) from your analysis\. ## Analyzer warnings **Issue 1** ```cpp public void Apply3D(AudioListener listener, AudioEmitter emitter) { .... var i = FindVariable("Distance"); _variables[i].SetValue(distance); .... var j = FindVariable("OrientationAngle"); _variables[j].SetValue(angle); .... } ``` PVS\-Studio warning: [V3106](https://pvs-studio.com/en/docs/warnings/v3106/) Possible negative index value\. The value of 'i' index could reach \-1\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) Cue\.cs 251 The analyzer noticed that the _i_ variable can have value \-1\. This variable was used as an index\. The _i_ variable is initialized by the return value of the _FindVariable_ method\. Let's look inside this method: ```cpp private int FindVariable(string name) { // Do a simple linear search... which is fast // for as little variables as most cues have. for (var i = 0; i < _variables.Length; i++) { if (_variables[i].Name == name) return i; } return -1; } ``` If no element with the corresponding value in the collection is found, then the return value is \-1\. Obviously, using a negative number as an index will lead to _IndexOutOfRangeException_\. **Issue 2** The next problem was also found in the _Apply3D_ method: ```cpp public void Apply3D(AudioListener listener, AudioEmitter emitter) { .... lock (_engine.UpdateLock) { .... // Calculate doppler effect. var relativeVelocity = emitter.Velocity - listener.Velocity; relativeVelocity *= emitter.DopplerScale; } } ``` PVS\-Studio warning: [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'relativeVelocity' variable is assigned but is not used by the end of the function\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) Cue\.cs 266 The analyzer warns us that the value was assigned, but never used further\. Someone might get confused by the fact that the code is in the _lock_ block, but\.\.\. It means nothing for _relativeVelocity_ because this variable is declared locally and doesn't participate in the inter\-thread communication\. Maybe the value of _relativeVelocity_ should be assigned to a field\. **Issue 3** ```cpp private void SetData(int offset, int rows, int columns, object data) { .... if(....) { .... } else if (rows == 1 || (rows == 4 && columns == 4)) { // take care of shader compiler optimization int len = rows * columns * elementSize; if (_buffer.Length - offset > len) len = _buffer.Length - offset; // <= Buffer.BlockCopy(data as Array, 0, _buffer, offset, rows*columns*elementSize); } .... } ``` PVS\-Studio warning: [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'len' variable is assigned but is not used by the end of the function\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) ConstantBuffer\.cs 91 Another warning about a value assigned but never used\. The _len_ variable is initialized with the following expression: ```cpp int len = rows * columns * elementSize; ``` If you look closely at the code, you might feel deja vu, because this expression repeats one more time: ```cpp Buffer.BlockCopy(data as Array, 0, _buffer, offset, rows*columns*elementSize); // <= ``` Most likely, _len_ was supposed to be in this place\. **Issue 4** ```cpp protected virtual object EvalSampler_Declaration(....) { if (this.GetValue(tree, TokenType.Semicolon, 0) == null) return null; var sampler = new SamplerStateInfo(); sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string; foreach (ParseNode node in nodes) node.Eval(tree, sampler); var shaderInfo = paramlist[0] as ShaderInfo; shaderInfo.SamplerStates.Add(sampler.Name, sampler); // <= return null; } ``` PVS\-Studio warning: [V3156](https://pvs-studio.com/en/docs/warnings/v3156/) The first argument of the 'Add' method is not expected to be null\. Potential null value: sampler\.Name\. MonoGame\.Effect\.Compiler ParseTree\.cs 1111 The analyzer warns us that the _Add_ method is not designed to take _null_ as a first argument\. At the same time the analyzer warns us that the first argument _sampler\.Name_, passed to _Add_, can be _null_\. To begin with, let's look at the _shaderInfo\.SamplerStates_ field: ```cpp public class ShaderInfo { .... public Dictionary SamplerStates = new Dictionary(); } ``` It's a dictionary and _Add_ is a standard method\. Indeed, _null_ cannot be a dictionary key\. The value of the _sampler\.Name_ field is passed as the dictionary key\. A potential _null_ can be assigned in this line: ```cpp sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string; ``` The _GetValue_ method can return _null_ or an instance of any type other than _string_\. Thus, the result of casting via the _as_ operator is _null_\. Could it be? Let's look at _getValue_: ```cpp protected object GetValue(ParseTree tree, TokenType type, ref int index) { object o = null; if (index < 0) return o; // left to right foreach (ParseNode node in nodes) { if (node.Token.Type == type) { index--; if (index < 0) { o = node.Eval(tree); break; } } } return o; } ``` So, this method can return _null_ in two cases: 1. If the passed _index_ value is less than 0; 1. If an element of the _nodes_ collection that matches the passed _type_ was not found\. The developer should have added _null_ check for the return value of the _as_ operator\. **Issue 5** ```cpp internal void Update() { if (GetQueuedSampleCount() > 0) { BufferReady.Invoke(this, EventArgs.Empty); } } ``` PVS\-Studio warning: [V3083](https://pvs-studio.com/en/docs/warnings/v3083/) Unsafe invocation of event 'BufferReady', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) Microphone\.OpenAL\.cs 142 The analyzer warns about an unsafe invocation of event that potentially has no subscribers\. Before the event invocation, the return value of the _GetQueuedSampleCount_ method is checked\. If the presence of subscribers to the event does not depend on the truth of the condition, then a _NullReferenceException_ may be thrown when this event is called\. If the truth of the expression "_GetQueuedSampleCount\(\) \> 0\>_" guarantees the presence of subscribers, the problem still remains\. The state can change between the check and the invocation\. The _BufferReady_ event is declared like this: ```cpp public event EventHandler BufferReady; ``` Note that the _public_ access modifier allows other developers to use the _BufferReady_ event in any code\. This increases the chance of performing operations with the event in other threads\. Thus, adding _null_ check in the condition does not prevent from _NullReferenceException_, because the _BufferReady_ state can change between the check and the invocation\. The easiest way to fix it is to add Elvis operator '?\.' to the _Invoke_ call: ```cpp BufferReady?.Invoke(this, EventArgs.Empty); ``` If this option is not available for some reason, assign _BufferReady_ to a local variable and work with it: ```cpp EventHandler bufferReadyLocal = BufferReady; if (bufferReadyLocal != null) bufferReadyLocal.Invoke(this, EventArgs.Empty); ``` Errors with _public_ events in multi\-threaded code may appear rarely, but they are very malicious\. These errors are hard or even impossible to reproduce\. You can read more about safer work with operators in the [V3083](https://pvs-studio.com/en/docs/warnings/v3083/) documentation\. **Issue 6** ```cpp public override TOutput Convert( TInput input, string processorName, OpaqueDataDictionary processorParameters) { var processor = _manager.CreateProcessor(processorName, processorParameters); var processContext = new PipelineProcessorContext(....); var processedObject = processor.Process(input, processContext); .... } ``` PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'processor'\. MonoGame\.Framework\.Content\.Pipeline PipelineProcessorContext\.cs 55 The analyzer warns about possible dereference of the null reference when _processor\.Process_ is called\. An object of the _processor_ class is created via the _\_manager\.CreateProcessor_ call\. Let's look at its code fragment: ```cpp public IContentProcessor CreateProcessor( string name, OpaqueDataDictionary processorParameters) { var processorType = GetProcessorType(name); if (processorType == null) return null; .... } ``` We see that _CreateProcessor_ returns _null_ if _GetProcessorType_ also returns _null_\. Well, let's look at the method's code: ```cpp public Type GetProcessorType(string name) { if (_processors == null) ResolveAssemblies(); // Search for the processor type. foreach (var info in _processors) { if (info.type.Name.Equals(name)) return info.type; } return null; } ``` This method can return _null_ if no matching element was found in the collection\. If _GetProcessorType_ returns _null_, then _CreateProcessor_ also returns _null_, which will be written to the _processor_ variable\. As a result, _NullReferenceException_ will be thrown if we call the _processor\.Process_ method\. Let's go back to the _Convert_ method from the warning\. Have you noticed that it has the _override_ modifier? This method is an implementation of a contract from an abstract class\. Here's this abstract method: ```cpp /// /// Converts a content item object using the specified content processor. ///.... /// Optional processor /// for this content. ///.... public abstract TOutput Convert( TInput input, string processorName, OpaqueDataDictionary processorParameters ); ``` The comment to the _processorName_ input parameter implies that this parameter is optional\. Perhaps the developer, seeing such a comment for the signature, will be sure that checks for _null_ or empty strings were made in the contract implementations\. But this implementation does not have any check\. Detection of potential dereference of a null reference allows us to find a number of possible sources of problem\. For example: * the correct work requires a non\-empty and non\-_null_ string value, contrary to the comment to the abstract method signature\. * a large number of _null_\-value returns, which are accessed without check\. As a result, this may lead to _NullReferenceException_\. **Issue 7** ```cpp public MGBuildParser(object optionsObject) { .... foreach(var pair in _optionalOptions) { var fi = GetAttribute(pair.Value); if(!string.IsNullOrEmpty(fi.Flag)) _flags.Add(fi.Flag, fi.Name); } } ``` PVS\-Studio warning: [V3146](https://pvs-studio.com/en/docs/warnings/v3146/) Possible null dereference of 'fi'\. The 'FirstOrDefault' can return default null value\. MonoGame\.Content\.Builder CommandLineParser\.cs 125 This warning is also about possible _NullReferenceException_, since the return value of _FirstOrDefault_ wasn't checked for _null_\. Let's find this _FirstOrDefault_ call\. The _fi_ variable is initialized with the value returned by the _GetAttribute_ method\. The _FirstOrDefault_ call from the analyzer's warning is there\. The search didn't take too much time: ```cpp static T GetAttribute(ICustomAttributeProvider provider) where T : Attribute { return provider.GetCustomAttributes(typeof(T),false) .OfType() .FirstOrDefault(); } ``` A _null_ conditional operator should be used to protect code from _NullReferenceException_\. ```cpp if(!string.IsNullOrEmpty(fi?.Flag)) ``` Consequently, if _fi_ is _null_, then when we try to access the _Flag_ property, we'll get _null_ instead of an exception\. The return value of _IsNullOrEmpty_ for _null_ argument is _false_\. **Issue 8** ```cpp public GenericCollectionHelper(IntermediateSerializer serializer, Type type) { var collectionElementType = GetCollectionElementType(type, false); _contentSerializer = serializer.GetTypeSerializer(collectionElementType); .... } ``` PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference inside method at 'type\.IsArray'\. Consider inspecting the 1st argument: collectionElementType\. MonoGame\.Framework\.Content\.Pipeline GenericCollectionHelper\.cs 48 PVS\-Studio indicates that _collectionElementType_ is passed to the _serializer\.GetTypeSerializer_ method\. _collectionElementType_ may be _null_\. This argument is dereferenced inside of the method, and this is another potential _NullReferenceException_\. Let's check that we cannot pass _null_ to _ContentTypeSerializer:_ ```cpp public ContentTypeSerializer GetTypeSerializer(Type type) { .... if (type.IsArray) { .... } .... } ``` Note that if the _type_ parameter is _null_, then accessing _IsArray_ property will throw an exception\. Passed _collectionElementType_is initialized with the return value of the _GetCollectionElementType_ method\. Let's look at what this method has inside: ```cpp private static Type GetCollectionElementType(Type type, bool checkAncestors) { if (!checkAncestors && type.BaseType != null && FindCollectionInterface(type.BaseType) != null) return null; var collectionInterface = FindCollectionInterface(type); if (collectionInterface == null) return null; return collectionInterface.GetGenericArguments()[0]; } ``` If the control switches to one of the two conditional constructions, _null_ will be returned\. Two scenarios that lead to _NullReferenceException_ versus one scenario that leads to non\-_null_ value returned\. Still, not a single check\. **Issue 9** ```cpp class Floor0 : VorbisFloor { int _rate; .... int[] SynthesizeBarkCurve(int n) { var scale = _bark_map_size / toBARK(_rate / 2); .... } } ``` PVS\-Studio warning: [V3041](https://pvs-studio.com/en/docs/warnings/v3041/) The expression was implicitly cast from 'int' type to 'double' type\. Consider utilizing an explicit type cast to avoid the loss of a fractional part\. An example: double A \= \(double\)\(X\) / Y;\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) VorbisFloor\.cs 113 The analyzer warns that when the integer value of _\_rate_ is divided by two, an unexpected loss of the fractional part of the result may occur\. This is a warning from the NVorbis code\. The warning relates to the second division operator\. The _toBARK_ method signature looks like this: ```cpp static float toBARK(double lsp) ``` The _\_rate_ field has the _int_ type\. The result of division an integer type variable by a same\-type variable is also an integer – the fractional part will be lost\. If this behavior was not intended, then to get a _double_ value as a result of division, you can, for example, add the _d_ literal to a number or write this number with a dot: ```cpp var scale = _bark_map_size / toBARK(_rate / 2d); var scale = _bark_map_size / toBARK(_rate / 2.0); ``` **Issue 10** ```cpp internal int InflateFast(....) { .... if (c > e) { // if source crosses, c -= e; // wrapped copy if (q - r > 0 && e > (q - r)) { do { s.window[q++] = s.window[r++]; } while (--e != 0); } else { Array.Copy(s.window, r, s.window, q, e); q += e; r += e; e = 0; // <= } r = 0; // copy rest from start of window // <= } .... } ``` PVS\-Studio warning: [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'r' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1309, 1307\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) Inflate\.cs 1309 The analyzer detected that a variable with a value was assigned a new value\. The previous one was never used\. This warning was issued on the DotNetZip code\. If the control moves to the _else_ branch, the _r_ variable is assigned the sum of _r_ and _e_\. When the branch exits, the first operation will assign another value to _r_, without using the current one\. The sum will be lost, making part of the calculations meaningless\. ## Conclusion Errors can be different\. Even skilled developers make them\. In this article we inspected both simple mistakes and dangerous fragments\. The developers may not even notice some of them — code doesn't always say that one method returns _null_ and the other method uses this _null_ without any check\. Static analysis isn't perfect, but it still finds errors like these \(and many more\!\)\. So why don't you [try the analyzer](https://pvs-studio.com/en/pvs-studio/try-free/) and check your projects? Maybe you'll find some interesting things too\. Thank you and see you in next articles\!