Vadim Kuleshov

Feb 04 2022

Tags:

#CSharp #GameDev

Playing with null: Checking MonoGame with the PVS-Studio analyzer

Feb 04 2022

Author: Vadim Kuleshov

Introduction
Analyzer warnings
Conclusion

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#.

Introduction

MonoGame is an open-source framework for game development. It's the heir of the XNA project, which was developed by Microsoft until 2013.

Let me also remind you about what 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 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 from your analysis.

Analyzer warnings

Issue 1

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 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:

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:

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 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

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 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:

int len = rows * columns * elementSize;

If you look closely at the code, you might feel deja vu, because this expression repeats one more time:

Buffer.BlockCopy(data as Array, 0,
                 _buffer,
                 offset,
                 rows*columns*elementSize);    // <=

Most likely, len was supposed to be in this place.

Issue 4

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 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:

public class ShaderInfo
{
  ....

  public Dictionary<string, SamplerStateInfo> SamplerStates =
     new Dictionary<string, SamplerStateInfo>();
}

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:

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:

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:

If the passed index value is less than 0;
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

internal void Update()
{
  if (GetQueuedSampleCount() > 0)
  {
    BufferReady.Invoke(this, EventArgs.Empty);
  }
}

PVS-Studio warning: 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:

public event EventHandler<EventArgs> 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:

BufferReady?.Invoke(this, EventArgs.Empty);

If this option is not available for some reason, assign BufferReady to a local variable and work with it:

EventHandler<EventArgs> 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 documentation.

Issue 6

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);
  ....
}

PVS-Studio warning: 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:

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:

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:

/// <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
);

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

public MGBuildParser(object optionsObject)
{
  ....
  foreach(var pair in _optionalOptions)
  {
    var fi = GetAttribute<CommandLineParameterAttribute>(pair.Value);
    if(!string.IsNullOrEmpty(fi.Flag))
      _flags.Add(fi.Flag, fi.Name);
  }
}

PVS-Studio warning: 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:

static T GetAttribute<T>(ICustomAttributeProvider provider)
                         where T : Attribute
{
  return provider.GetCustomAttributes(typeof(T),false)
                 .OfType<T>()
                 .FirstOrDefault();
}

A null conditional operator should be used to protect code from NullReferenceException.

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

public GenericCollectionHelper(IntermediateSerializer serializer,
                               Type type)
{
  var collectionElementType = GetCollectionElementType(type, false);
  _contentSerializer = 
                serializer.GetTypeSerializer(collectionElementType);
  ....
}

PVS-Studio warning: 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:

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 collectionElementTypeis initialized with the return value of the GetCollectionElementType method. Let's look at what this method has inside:

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

class Floor0 : VorbisFloor
{
  int _rate;
  ....
  int[] SynthesizeBarkCurve(int n)
  {
    var scale = _bark_map_size / toBARK(_rate / 2);
    ....
  }
}

PVS-Studio warning: 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:

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:

var scale = _bark_map_size / toBARK(_rate / 2d);
var scale = _bark_map_size / toBARK(_rate / 2.0);

Issue 10

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 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 and check your projects? Maybe you'll find some interesting things too.

Thank you and see you in next articles!

#CSharp #GameDev