Вадим Кулешов

04 Фев 2022

Теги:

#CSharp #GameDev

Игра с null: проверка MonoGame статическим анализатором PVS-Studio

04 Фев 2022

Автор: Вадим Кулешов

Введение
Предупреждения анализатора
Заключение

Анализатор PVS-Studio уже не раз был использован для анализа кода библиотек, фреймворков и движков для разработки игр. Пришло время добавить к их списку MonoGame – низкоуровневый gamedev-фреймворк, написанный на языке C#.

Введение

MonoGame – это фреймворк с открытым исходным кодом, используемый для разработки игр. Является идейным наследником проекта XNA, который разрабатывался Microsoft до 2013 года.

Думаю, нелишним будет напомнить и про то, что такое PVS-Studio :). Но если даже лишним для наших постоянных читателей, то все равно напомню :) PVS-Studio — статический анализатор кода, который позволяет находить различные ошибки, а также проблемы, связанные с безопасностью приложений. В этой статье был использован анализатор версии 7.16 и исходники MonoGame от 12.01.2022.

Стоит сказать, что анализатор выдал пару предупреждений для кода используемых в проекте библиотек – DotNetZip и NVorbis. Ради интереса я привёл их в этой статье. Вы же при желании можете легко исключить из проверки сторонний код.

Предупреждения анализатора

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: V3106 Possible negative index value. The value of 'i' index could reach -1. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 251

Анализатор сообщает о том, что переменная i, используемая в качестве индекса, может принимать значение -1.

Эта переменная инициализируется возвращаемым значением метода FindVariable. Посмотрим, что у него внутри:

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

Если в коллекции не удалось найти элемент с соответствующим значением свойства, то возвращённое значение будет равно -1. Очевидно, что использование отрицательного числа в качестве индекса приведёт к выбрасыванию исключения IndexOutOfRangeException.

Issue 2

Следующая проблема также была найдена в методе 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: V3137 The 'relativeVelocity' variable is assigned but is not used by the end of the function. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 266

Это предупреждение о случае, когда значение присваивается, но никак не используется далее.

При беглом просмотре кого-то могло смутить, что код находится в lock-блоке, но... для relativeVelocity это ничего не значит, так как она объявлена локально и не участвует в межпоточном взаимодействии.

Возможно, значение relativeVelocity должно быть присвоено какому-нибудь полю.

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: V3137 The 'len' variable is assigned but is not used by the end of the function. MonoGame.Framework.DesktopGL(netstandard2.0) ConstantBuffer.cs 91

Это предупреждение о ещё одном случае, когда значение присваивается, но никак не используется далее.

Переменная len инициализируется таким выражением:

int len = rows * columns * elementSize;

Если вы внимательно вглядитесь в код, то вас может посетить чувство дежавю, ведь это выражение встречается в коде ещё один раз:

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

Скорее всего, len должна быть на месте этого выражения.

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

Предупреждение говорит о том, что метод Add не рассчитан на передачу в него null в качестве первого аргумента. В то же время анализатор сообщает, что первый передаваемый в Add аргумент sampler.Name может иметь значение null.

Для начала взглянем подробнее на поле shaderInfo.SamplerStates:

public class ShaderInfo
{
  ....

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

Оказывается, что это словарь, а Add – стандартный метод. Действительно, null не может быть ключом словаря.

В качестве ключа словаря передаётся значение поля sampler.Name. Потенциальный null может быть присвоен в этой строке:

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

Результатом приведения через оператор as будет null, если метод GetValue вернёт null или не экземпляр типа string. Может ли такое быть? Посмотрим внутрь 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;
}

Итак, этот метод может вернуть null в двух случаях:

Если переданное значение index меньше 0;
Если не был найден подходящий по переданному type элемент коллекции nodes.

Стоит добавить проверку на null для возвращаемого значения оператора as.

Issue 5

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

Предупреждение PVS-Studio: 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

Сообщение анализатора говорит о небезопасном вызове события, потенциально не имеющего подписчиков.

Перед вызовом события производится проверка возвращаемого значения метода GetQueuedSampleCount. Если наличие подписчиков у события не зависит от истинности условия, то при вызове возможно исключение NullReferenceException.

Если же логика такова, что истинность выражения "GetQueuedSampleCount() > 0" гарантирует наличие подписчиков, то проблема остаётся. Дело в том, что состояние может измениться между проверкой и вызовом события. Событие BufferReady объявлено вот так:

public event EventHandler<EventArgs> BufferReady;

Важно отметить, что модификатор доступа public позволяет другим программистам использовать событие BufferReady в любом коде, что повышает шанс проведения операций с событием в других потоках.

Соответственно, даже добавление проверки на null в само условие не спасает, ведь состояние BufferReady между проверкой и вызовом может быть изменено.

Самый простой вариант исправления – добавление Elvis operator '?.' в вызов Invoke:

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

Если такой способ по каким-то причинам недоступен, то следует присвоить BufferReady локальной переменной и работать с ней:

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

Ошибки public событий в многопоточном коде могут появляться редко, но являются очень коварными. Такие ошибки сложно или даже невозможно воспроизвести. Подробнее тема безопасной работы с событиями описана в документации к диагностике 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: V3080 Possible null dereference. Consider inspecting 'processor'. MonoGame.Framework.Content.Pipeline PipelineProcessorContext.cs 55

Анализатор предупреждает о возможном разыменовании нулевой ссылки при вызове processor.Process.

Объект класса processor создаётся вызовом _manager.CreateProcessor. Посмотрим часть его кода:

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

Из кода следует, что CreateProcessor вернёт null в случае, если и метод GetProcessorType вернёт null. Что ж, давайте посмотрим и на его код:

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

Этот метод может вернуть null, если в коллекции не был найден подходящий элемент. Тогда если GetProcessorType вернёт null, то и CreateProcessor вернёт null, который будет записан в переменную processor. В итоге это приведёт к NullReferenceException при вызове метода: processor.Process.

А теперь вернёмся к методу Convert из предупреждения. Вы заметили, что он имеет модификатор override? Этот метод является реализацией контракта из абстрактного класса. Вот сам абстрактный метод:

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

Комментарий ко входному параметру processorName уверяет программиста, что этот параметр необязательный. Возможно, программист, увидев такой комментарий для сигнатуры, будет уверен, что в реализациях контракта были сделаны проверки на null или пустоту строки. Но в данной реализации проверок нет.

Обнаружение анализатором потенциального разыменования нулевой ссылки в одном месте позволило найти большое количество вероятных источников проблем, например:

для корректной работы требуется непустое и не null значение строки вопреки комментарию к сигнатуре абстрактного метода;
большое количество возвратов null-значений, обращение к которым происходит без проверки и, как следствие, может привести к 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: V3146 Possible null dereference of 'fi'. The 'FirstOrDefault' can return default null value. MonoGame.Content.Builder CommandLineParser.cs 125

Это также предупреждение о вероятном NullReferenceException, так как возвращаемое значение FirstOrDefault не было проверено на null.

Давайте найдём этот вызов FirstOrDefault. Переменная fi инициализируется значением, возвращаемым методом GetAttribute. Вызов FirstOrDefault из предупреждения анализатора находится там, поиск не занял много времени:

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

Следует использовать null-условный оператор для защиты от NullReferenceException:

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

Тогда, если fi – null, то при попытке обратиться к свойству Flag, мы получим не исключение, а null. Возвращаемым значением IsNullOrEmpty для null-аргумента будет обычный false.

Issue 8

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

Предупреждение PVS-Studio: V3080 Possible null dereference inside method at 'type.IsArray'. Consider inspecting the 1st argument: collectionElementType. MonoGame.Framework.Content.Pipeline GenericCollectionHelper.cs 48

PVS-Studio указывает, что в метод serializer.GetTypeSerializer передаётся collectionElementType, который может иметь значение null. Внутри метода происходит разыменование этого аргумента, и это ещё один потенциальный NullReferenceException.

Проверим, что в ContentTypeSerializer действительно нельзя передавать null:

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

Очевидно, что если параметр type будет равен null, то обращение к свойству IsArray приведёт к выбрасыванию исключения.

Переданный collectionElementType инициализируется возвращаемым значением метода GetCollectionElementType. Посмотрим, что у метода внутри:

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];
}

Если управление перейдёт в одну из двух условных конструкций, то будет возвращён null. Два сценария, которые могут привести к NullReferenceException, против одного сценария с возвратом не null-значения, но ни одной проверки нет.

Issue 9

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

Предупреждение PVS-Studio: 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

Анализатор сообщает, что при делении целочисленного значения _rate на 2 может произойти неожидаемая потеря дробной части результата. Это предупреждение из кода NVorbis.

Предупреждение связно со вторым оператором деления. Сигнатура метода toBARK выглядит так:

static float toBARK(double lsp)

Поле _rate имеет тип int, и результат деления переменной целочисленного типа на значение этого же типа также будет целочисленным – дробная часть будет потеряна. Если такое поведение не предполагалось, то для получения в результате деления значения типа double можно, например, добавить литерал d к числу или написать это число в виде с точкой:

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

Анализатор определил, что переменной, имеющей значение, присваивается другое, но предыдущее не было никак использовано. Это предупреждение из кода DotNetZip.

Если управление перейдёт в else-ветку, то переменной r будет присвоен результат суммы r и e. Но после выхода из ветки первая же операция присвоит r другое значение, не используя существующее. Результат суммы будет потерян, делая часть вычислений бессмысленными.

Заключение

Ошибки бывают самые разные, и совершают их разработчики самых разных уровней. В данном разборе мы сталкивались и с простыми недочётами, и с действительно опасными фрагментами. Глядя на некоторые из них, проблему вообще нельзя заметить. Ведь по коду не всегда видно, что какой-либо метод возвращает null, а другой метод этот null без проверки использует.

Статический анализ, конечно же, неидеален, но всё же он позволяет обнаруживать подобные проблемы (и не только их!). Поэтому я предлагаю вам попробовать анализатор и также проверить интересные вам проекты – вдруг что-нибудь найдётся?

Большое спасибо за внимание, увидимся в следующих статьях!

#CSharp #GameDev