{"id":387889,"date":"2024-06-29T07:36:07","date_gmt":"2024-06-29T07:36:07","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=387889"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=387889","title":{"rendered":"Playing with null: Checking MonoGame with the PVS-Studio analyzer<\/span>"},"content":{"rendered":"
<\/div>\n
\n
\n
\n
\n

The PVS-Studio analyzer often checks code of libraries, frameworks, and engines for game development. Today we check another project \u2014 MonoGame, a low-level gamedev framework written in C#.<\/p>\n

<\/figcaption><\/figure>\n

Introduction<\/h3>\n

MonoGame is an open-source framework for game development. It’s the heir of the XNA<\/a> project, which was developed by Microsoft until 2013.<\/p>\n

Let me also remind you about what PVS-Studio<\/a> 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<\/a> from 12.01.2022.<\/p>\n

It’s worth mentioning that the analyzer issued a couple of warnings on some libraries used in the project \u2014 DotNetZip and NVorbis. I described them below. If you want, you can easily exclude third-party code<\/a> from your analysis.<\/p>\n

Analyzer warnings<\/h3>\n

Issue 1<\/strong><\/p>\n

public void Apply3D(AudioListener listener, AudioEmitter emitter)  {   ....   var i = FindVariable(\"Distance\");   _variables[i].SetValue(distance);   ....   var j = FindVariable(\"OrientationAngle\");   _variables[j].SetValue(angle);   .... } <\/code><\/pre>\n
PVS-Studio warning: V3106<\/a> Possible negative index value. The value of ‘i’ index could reach -1. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 251<\/p>\n
The analyzer noticed that the i<\/em> variable can have value -1. This variable was used as an index.<\/p>\n
The i<\/em> variable is initialized by the return value of the FindVariable<\/em> method. Let’s look inside this method:<\/p>\n
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; } <\/code><\/pre>\n
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<\/em>.<\/p>\n
Issue 2<\/strong><\/p>\n
The next problem was also found in the Apply3D<\/em> method:<\/p>\n
public void Apply3D(AudioListener listener, AudioEmitter emitter) {   ....   lock (_engine.UpdateLock)   {     ....     \/\/ Calculate doppler effect.     var relativeVelocity = emitter.Velocity - listener.Velocity;     relativeVelocity *= emitter.DopplerScale;   } } <\/code><\/pre>\n
PVS-Studio warning: V3137<\/a> The ‘relativeVelocity’ variable is assigned but is not used by the end of the function. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 266<\/p>\n
The analyzer warns us that the value was assigned, but never used further.<\/p>\n
Someone might get confused by the fact that the code is in the lock<\/em> block, but… It means nothing for relativeVelocity<\/em> because this variable is declared locally and doesn’t participate in the inter-thread communication.<\/p>\n
Maybe the value of relativeVelocity<\/em> should be assigned to a field.<\/p>\n
Issue 3<\/strong><\/p>\n
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);   }   .... } <\/code><\/pre>\n
PVS-Studio warning: V3137<\/a> The ‘len’ variable is assigned but is not used by the end of the function. MonoGame.Framework.DesktopGL(netstandard2.0) ConstantBuffer.cs 91<\/p>\n
Another warning about a value assigned but never used.<\/p>\n
The len<\/em> variable is initialized with the following expression:<\/p>\n
int len = rows * columns * elementSize; <\/code><\/pre>\n
If you look closely at the code, you might feel deja vu, because this expression repeats one more time:<\/p>\n
Buffer.BlockCopy(data as Array, 0,                  _buffer,                  offset,                  rows*columns*elementSize);    \/\/ <= <\/code><\/pre>\n
Most likely, len<\/em> was supposed to be in this place.<\/p>\n
Issue 4<\/strong><\/p>\n
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; } <\/code><\/pre>\n
PVS-Studio warning: V3156<\/a> The first argument of the ‘Add’ method is not expected to be null. Potential null value: sampler.Name<\/a>. MonoGame.Effect.Compiler ParseTree.cs 1111<\/p>\n
The analyzer warns us that the Add<\/em> method is not designed to take null<\/em> as a first argument. At the same time the analyzer warns us that the first argument sampler.Name<\/em><\/a>, passed to Add<\/em>, can be null<\/em>.<\/p>\n
To begin with, let’s look at the shaderInfo.SamplerStates<\/em> field:<\/p>\n
public class ShaderInfo {   ....    public Dictionary<string, SamplerStateInfo> SamplerStates =      new Dictionary<string, SamplerStateInfo>(); } <\/code><\/pre>\n
It’s a dictionary and Add<\/em> is a standard method. Indeed, null<\/em> cannot be a dictionary key.<\/p>\n
The value of the sampler.Name<\/em><\/a> field is passed as the dictionary key. A potential null<\/em> can be assigned in this line:<\/p>\n
sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string; <\/code><\/pre>\n
The GetValue<\/em> method can return null<\/em> or an instance of any type other than string<\/em>. Thus, the result of casting via the as<\/em> operator is null<\/em>.  Could it be? Let’s look at getValue<\/em>:<\/p>\n
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; } <\/code><\/pre>\n
So, this method can return null<\/em> in two cases:<\/p>\n
    \n
  1. \n
    If the passed index<\/em> value is less than 0;<\/p>\n<\/li>\n
  2. \n
    If an element of the nodes<\/em> collection that matches the passed type<\/em> was not found.<\/p>\n<\/li>\n<\/ol>\n
    The developer should have added null<\/em> check for the return value of the as<\/em> operator.<\/p>\n
    Issue 5<\/strong><\/p>\n
    internal void Update() {   if (GetQueuedSampleCount() > 0)   {     BufferReady.Invoke(this, EventArgs.Empty);   } } <\/code><\/pre>\n
    PVS-Studio warning: V3083<\/a> 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<\/p>\n
    The analyzer warns about an unsafe invocation of event that potentially has no subscribers.<\/p>\n
    Before the event invocation, the return value of the GetQueuedSampleCount<\/em> method is checked. If the presence of subscribers to the event does not depend on the truth of the condition, then a NullReferenceException<\/em> may be thrown when this event is called.<\/p>\n
    If the truth of the expression «GetQueuedSampleCount() > 0><\/em>» guarantees the presence of subscribers, the problem still remains. The state can change between the check and the invocation. The BufferReady<\/em> event is declared like this:<\/p>\n
    public event EventHandler<EventArgs> BufferReady; <\/code><\/pre>\n
    Note that the public<\/em> access modifier allows other developers to use the BufferReady<\/em> event in any code. This increases the chance of performing operations with the event in other threads.<\/p>\n
    Thus, adding null<\/em> check in the condition does not prevent from NullReferenceException<\/em>, because the BufferReady<\/em> state can change between the check and the invocation.<\/p>\n
    The easiest way to fix it is to add Elvis operator ‘?.’ to the Invoke<\/em> call:<\/p>\n
    BufferReady?.Invoke(this, EventArgs.Empty); <\/code><\/pre>\n
    If this option is not available for some reason, assign BufferReady<\/em> to a local variable and work with it:<\/p>\n
    EventHandler<EventArgs> bufferReadyLocal = BufferReady; if (bufferReadyLocal != null)   bufferReadyLocal.Invoke(this, EventArgs.Empty); <\/code><\/pre>\n
    Errors with public<\/em> 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<\/a> documentation.<\/p>\n
    Issue 6<\/strong><\/p>\n
    public override TOutput Convert<TInput, TOutput>(   TInput input,   string processorName,   OpaqueDataDictionary processorParameters) {   var processor = _manager.CreateProcessor(processorName,                                                  processorParameters);   var processContext = new PipelineProcessorContext(....);   var processedObject = processor.Process(input, processContext);   .... } <\/code><\/pre>\n
    PVS-Studio warning: V3080<\/a> Possible null dereference. Consider inspecting ‘processor’. MonoGame.Framework.Content.Pipeline PipelineProcessorContext.cs 55<\/p>\n
    The analyzer warns about possible dereference of the null reference when processor.Process<\/em> is called.<\/p>\n
    An object of the processor<\/em> class is created via the _manager.CreateProcessor<\/em> call. Let’s look at its code fragment:<\/p>\n
    public IContentProcessor CreateProcessor(                     string name,                     OpaqueDataDictionary processorParameters) {   var processorType = GetProcessorType(name);   if (processorType == null)     return null;   .... } <\/code><\/pre>\n
    We see that CreateProcessor<\/em> returns null<\/em> if GetProcessorType<\/em> also returns null<\/em>. Well, let’s look at the method’s code:<\/p>\n
    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; } <\/code><\/pre>\n
    This method can return null<\/em> if no matching element was found in the collection. If GetProcessorType<\/em> returns null<\/em>, then CreateProcessor<\/em> also returns null<\/em>, which will be written to the processor<\/em> variable. As a result, NullReferenceException<\/em> will be thrown if we call the processor.Process<\/em> method.<\/p>\n
    Let’s go back to the Convert<\/em> method from the warning. Have you noticed that it has the override<\/em> modifier? This method is an implementation of a contract from an abstract class. Here’s this abstract method:<\/p>\n
    \/\/\/ <summary> \/\/\/ Converts a content item object using the specified content processor. \/\/\/.... \/\/\/ <param name=\"processorName\">Optional processor  \/\/\/ for this content.<\/param> \/\/\/.... public abstract TOutput Convert<TInput,TOutput>(   TInput input,   string processorName,   OpaqueDataDictionary processorParameters ); <\/code><\/pre>\n
    The comment to the processorName<\/em> 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<\/em> or empty strings were made in the contract implementations. But this implementation does not have any check.<\/p>\n
    Detection of potential dereference of a null reference allows us to find a number of possible sources of problem. For example:<\/p>\n