checking MonoGame with PVS-Studio static analyzer

The PVS-Studio analyzer has been used more than once to analyze the code of libraries, frameworks and engines for game development. It’s time to add MonoGame, a low-level gamedev framework written in C#, to their list.

Introduction

MonoGame is an open source framework used for game development. Is the ideological successor of the project XNAwhich was developed by Microsoft until 2013.

I think it would be useful to recall what is PVS Studio :). But even if it’s superfluous for our regular readers, I’ll remind you anyway 🙂 PVS-Studio is a static code analyzer that allows you to find various errors, as well as problems related to application security. In this article, version 7.16 analyzer was used and MonoGame sources dated 12.01.2022.

It is worth saying that the analyzer issued a couple of warnings for the code of the libraries used in the project – DotNetZip and NVorbis. For the sake of interest, I have included them in this article. If you wish, you can easily exclude third-party code from checking.

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 reports that the variable iused as an index, can take the value -1.

This variable* *is initialized with the return value of the method FindVariable. Let’s see what’s inside it:

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 the collection failed to find an element with a matching property value, then the returned value will be -1. Obviously, using a negative number as an index will result in an exception being thrown. IndexOutOfRangeException.

Issue 2

The following problem was also found in the method Apply3D:

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

This is a warning about the case when a value is assigned but not used further.

At a cursory glance, it might confuse someone that the code is in lock-block, but… for relativeVelocity it means nothing, since it is declared locally and does not participate in inter-thread communication.

Perhaps the value * relativeVelocity * should be assigned to some 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

This is a warning about another case where a value is assigned but not used further.

Variable len initialized with this expression:

int len = rows * columns * elementSize;

If you look closely at the code, then you may feel a sense of deja vu, because this expression appears in the code one more time:

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

Most likely, len should be in place of this expression.

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 warning says that the method Add not designed for transfer null as the first argument. At the same time, the parser reports that the first Add argument sampler.Name may matter null.

First, let’s take a closer look at the field shaderInfo.SamplerStates:

public class ShaderInfo
{
  ....

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

It turns out that this is a dictionary, * a Add* is the standard method. Really, null cannot be a dictionary key.

The field value is passed as the dictionary key sampler.Name. Potential null can be assigned in this line:

sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string;

The result of casting through the operator as will nullif method GetValue will return null or not an instance of the type string. Could this be? Let’s look inside 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:

  1. If the passed value index less than 0;

  2. If a suitable one was not found according to the transmitted type collection element nodes.

It is worth adding a check for null for the return value of the operator as.

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 an event to a local variable before invoking it. MonoGame.Framework.DesktopGL(netstandard2.0) Microphone.OpenAL.cs 142

The analyzer’s message speaks of an unsafe call to an event that potentially has no subscribers.

Before the event is called, the return value of the method is checked GetQueuedSampleCount. If the presence of subscribers to the event does not depend on the truth of the condition, then an exception is possible when calling NullReferenceException.

If the logic is such that the truth of the expression “GetQueuedSampleCount() > 0” guarantees the existence of subscribers, then the problem remains. The fact is that the state can change between checking and raising the event. Event BufferReady declared like this:

public event EventHandler<EventArgs> BufferReady;

It is important to note that the access modifier public allows other programmers to use the event BufferReady in any code, which increases the chance of performing operations on the event in other threads.

Accordingly, even adding a check for null does not save the condition itself, because the state BufferReady between check and call can be changed.

The easiest fix is ​​to add the Elvis operator ‘?.’ in a challenge Invoke:

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

If for some reason this method is not available, then you should assign BufferReady local variable and work with it:

EventHandler<EventArgs> bufferReadyLocal = BufferReady;
if (bufferReadyLocal != null)
  bufferReadyLocal.Invoke(this, EventArgs.Empty);

Mistakes public events in multithreaded code may appear rarely, but are very insidious. Such errors are difficult or even impossible to reproduce. The topic of safe work with events is described in more detail in the documentation for diagnostics. V3083.

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 the possible dereference of a null reference when calling processor.Process.

class object processor created by calling _manager.CreateProcessor. Let’s see some of his code:

public IContentProcessor CreateProcessor(
                    string name,
                    OpaqueDataDictionary processorParameters)
{
  var processorType = GetProcessorType(name);
  if (processorType == null)
    return null;
  ....
}

It follows from the code that CreateProcessor will return null in case and method GetProcessorType will return null. Well, let’s take a look at its 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 nullif no matching element was found in the collection. Then if GetProcessorType will return nullthen and CreateProcessor will return nullwhich will be stored in a variable processor. Ultimately, this will lead to NullReferenceException when calling a method: processor.Process.

Now back to the method Convert from the warning. Did you notice that it has a modifier override? This method is an implementation of a contract from an abstract class. Here is the abstract method itself:

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

Input parameter comment processorName assures the programmer that this parameter is optional. Perhaps a programmer, seeing such a comment for a signature, will be sure that checks have been made in the implementations of the contract for null or a blank line. But in this implementation there are no checks.

The analyzer’s detection of a potential null reference dereference in one place allowed us to find a large number of possible sources of problems, for example:

  • to work correctly requires a non-empty and non-empty null string value contrary to abstract method signature comment;

  • a large number of returns null-values ​​that are accessed without checking and, as a result, can 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

It is also a warning about a possible NullReferenceExceptionsince the return value First Or Default has not been tested for null.

Let’s find this challenge First Or Default. Variable fi initialized with the value returned by the method GetAttribute. Call First Or Default from the analyzer warning is there, the search didn’t take long:

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

Should be used null-conditional operator to protect against NullReferenceException:

if(!string.IsNullOrEmpty(fi?.Flag))

Then if finullthen when you try to access the property Flagwe will get not an exception, but null. return value IsNullOrEmpty for null-argument will be normal 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 the method serializer.GetTypeSerializer transmitted collectionElementTypewhich may matter null. Inside the method, this argument is dereferenced, and this is another potential* NullReferenceException*.

Let’s check that in ContentTypeSerializer really can’t be transferred. null:

public ContentTypeSerializer GetTypeSerializer(Type type)
{
  ....
  if (type.IsArray)
  {
    ....
  }
  ....
}

Obviously, if the parameter type will be equal to nullthen accessing the property IsArray will result in an exception being thrown.

The passed *collectionElementType *is initialized with the return value of the method GetCollectionElementType. Let’s see what the 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 passes to one of the two conditional structures, then it will be returned null. Two scenarios that could lead to NullReferenceException, vs one scenario with no return null-values, but no validation.

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 reports that when dividing an integer value _rate at 2, an unexpected loss of the fractional part of the result may occur. This is a warning from the NVorbis code.

The warning is related to the second division operator. Method signature toBARK looks like that:

static float toBARK(double lsp)

Field _rate has type int, and the result of dividing a variable of integer type by a value of the same type will also be integer – the fractional part will be lost. If this behavior was not intended, then to obtain a value of type as a result of division double you can, for example, add a literal d to a number or write this number as 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 has determined that a variable that has a value is assigned to another one, but the previous one was not used in any way. This is a warning from the DotNetZip code.

If control is transferred to else-branch, then a variable r sum result will be assigned r and e. But after leaving the branch, the first operation will assign r another value without using the existing one. The result of the sum will be lost, making part of the calculation meaningless.

Conclusion

Mistakes are very different, and they are made by developers of various levels. In this analysis, we encountered both simple flaws and really dangerous fragments. Looking at some of them, the problem cannot be noticed at all. After all, it is not always clear from the code that any method returns nulland another method is this null uses without verification.

Static analysis, of course, is not perfect, but still it allows you to detect such problems (and not only them!). Therefore I suggest to you try analyzer and also check the projects you are interested in – what if there is something?

Thank you very much for your attention, see you in the next articles!

If you want to share this article with an English-speaking audience, then please use the translation link: Vadim Kuleshov. Playing with null: Checking MonoGame with the PVS-Studio analyzer.

Similar Posts

Leave a Reply