Анализатор PVS-Studio часто проверяет код библиотек, фреймворков и движков для разработки игр. Сегодня мы рассмотрим еще один проект — MonoGame, низкоуровневый фреймворк для разработки игр, написанный на C#.
Введение
MonoGame — это фреймворк с открытым исходным кодом для разработки игр. Это наследник проекта XNA, который разрабатывался Microsoft до 2013 года.
Также напомню, что такое PVS-Studio :). PVS-Studio — это статический анализатор кода, который ищет различные ошибки кода и уязвимости, связанные с безопасностью. Я использовал PVS-Studio версии 7.16 и Исходники MonoGame от 12.01.2022.
Стоит отметить, что анализатор выдал пару предупреждений о некоторых библиотеках, используемых в проекте — DotNetZip и NVorbis. Я описал их ниже. При желании вы можете легко исключить сторонний код из своего анализа.
Предупреждения анализатора
Проблема 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 Возможно отрицательное значение индекса. Значение индекса i может достигать -1. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 251
Анализатор заметил, что переменная i может иметь значение -1. Эта переменная использовалась как индекс.
Переменная i инициализируется возвращаемым значением метода 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.
Проблема 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 Переменная relativeVelocity присваивается, но к концу функции не используется. MonoGame.Framework.DesktopGL(netstandard2.0) Cue.cs 266
Анализатор предупреждает, что значение было присвоено, но больше не использовалось.
Кого-то может смутить тот факт, что код находится в блоке lock, но… Для relativeVelocity это ничего не значит, потому что эта переменная объявлена локально и не участвует в межпотоковое общение.
Возможно, значение relativeVelocity следует присвоить полю.
Проблема 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 Переменная len назначена, но не используется к концу функции. 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); // <=Скорее всего, лен должен был быть в этом месте.
Проблема 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 Первый аргумент метода Добавить не должен быть нулевым. Возможное нулевое значение: sampler.Name. MonoGame.Effect.Compiler ParseTree.cs 1111
Анализатор предупреждает нас, что метод Add не предназначен для приема null в качестве первого аргумента. При этом анализатор предупреждает нас, что первый аргумент sampler.Name, переданный в Add, может быть null.
Для начала давайте посмотрим на поле shaderInfo.SamplerStates:
public class ShaderInfo { ....public Dictionary<string, SamplerStateInfo> SamplerStates = new Dictionary<string, SamplerStateInfo>(); }
Это словарь, а Добавить — стандартный метод. Действительно, null не может быть ключом словаря.
Значение поля sampler.Name передается как ключ словаря. В этой строке можно указать потенциальный null:
sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string;Метод GetValue может возвращать null или экземпляр любого типа, кроме string. Таким образом, результатом приведения с помощью оператора as является null. Может быть? Давайте посмотрим на 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;
- Если элемент коллекции nodes, соответствующий переданному типу, не найден.
Разработчик должен был добавить проверку null для возвращаемого значения оператора as.
Проблема 5
internal void Update()
{
if (GetQueuedSampleCount() > 0)
{
BufferReady.Invoke(this, EventArgs.Empty);
}
}Предупреждение PVS-Studio: V3083 Возможен небезопасный вызов события BufferReady, NullReferenceException. Рассмотрите возможность назначения события локальной переменной перед ее вызовом. MonoGame.Framework.DesktopGL(netstandard2.0) Microphone.OpenAL.cs 142
Анализатор предупреждает о небезопасном вызове события, потенциально не имеющего подписчиков.
Перед вызовом события проверяется возвращаемое значение метода GetQueuedSampleCount. Если наличие подписчиков на событие не зависит от истинности условия, то при вызове этого события может быть сгенерировано NullReferenceException.
Если истинность выражения «GetQueuedSampleCount() › 0›» гарантирует наличие подписчиков, проблема остается. Состояние может меняться между проверкой и вызовом. Событие BufferReady объявляется следующим образом:
public event EventHandler<EventArgs> BufferReady;Обратите внимание, что модификатор доступа public позволяет другим разработчикам использовать событие BufferReady в любом коде. Это увеличивает вероятность выполнения операций с событием в других потоках.
Таким образом, добавление проверки null в условие не предотвращает NullReferenceException, поскольку состояние BufferReady может измениться между проверкой и вызовом.
Самый простой способ исправить это — добавить оператор Элвиса ‘?.’ к вызову Invoke:
BufferReady?.Invoke(this, EventArgs.Empty);Если эта опция по какой-то причине недоступна, присвойте BufferReady локальной переменной и работайте с ней:
EventHandler<EventArgs> bufferReadyLocal = BufferReady;
if (bufferReadyLocal != null)
bufferReadyLocal.Invoke(this, EventArgs.Empty);Ошибки с событиями public в многопоточном коде могут появляться редко, но они очень опасны. Эти ошибки трудно или даже невозможно воспроизвести. Подробнее о безопасной работе с операторами можно прочитать в документации V3083.
Проблема 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 Возможно нулевое разыменование. Рассмотрите возможность проверки процессора. 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, который будет записан в процессор переменная. В результате будет выдано 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.
Проблема 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 Возможно нулевое разыменование fi. FirstOrDefault может возвращать нулевое значение по умолчанию. 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.
Проблема 8
public GenericCollectionHelper(IntermediateSerializer serializer,
Type type)
{
var collectionElementType = GetCollectionElementType(type, false);
_contentSerializer =
serializer.GetTypeSerializer(collectionElementType);
....
}Предупреждение PVS-Studio: V3080 Возможное нуль-разыменование внутри метода в ‘type.IsArray’. Рассмотрите возможность проверки 1-го аргумента: collectionElementType. MonoGame.Framework.Content.Pipeline GenericCollectionHelper.cs 48
PVS-Studio указывает, что collectionElementType передается в метод serializer.GetTypeSerializer. collectionElementType может иметь значение null. Этот аргумент разыменовывается внутри метода, и это еще одно потенциальное NullReferenceException.
Давайте проверим, что мы не можем передать null в ContentTypeSerializer:
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. Тем не менее, ни одной проверки.
Проблема 9
class Floor0 : VorbisFloor
{
int _rate;
....
int[] SynthesizeBarkCurve(int n)
{
var scale = _bark_map_size / toBARK(_rate / 2);
....
}
}Предупреждение PVS-Studio: V3041 Выражение было неявно приведено из типа int в тип double. Рассмотрите возможность использования явного приведения типа, чтобы избежать потери дробной части. Пример: двойной A = (двойной)(X)/Y;. MonoGame.Framework.DesktopGL(netstandard2.0) VorbisFloor.cs 113
Анализатор предупреждает, что при делении целочисленного значения _rate на два может произойти непредвиденная потеря дробной части результата. Это предупреждение из кода 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);Проблема 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 Переменной r дважды подряд присваиваются значения. Возможно, это ошибка. Проверьте строки: 1309, 1307. MonoGame.Framework.DesktopGL(netstandard2.0) Inflate.cs 1309
Анализатор обнаружил, что переменной со значением было присвоено новое значение. Предыдущий ни разу не использовался. Это предупреждение было выдано для кода DotNetZip.
Если элемент управления перемещается в ветвь else, переменной r присваивается сумма r и e. При выходе из ветви первая операция присвоит r другое значение, не используя текущее. Сумма будет потеряна, что сделает часть расчетов бессмысленными.
Заключение
Ошибки могут быть разные. Их делают даже опытные разработчики. В этой статье мы рассмотрели как простые ошибки, так и опасные фрагменты. Некоторые из них разработчики могут даже не заметить — код не всегда говорит о том, что один метод возвращает null, а другой метод использует этот null без всякой проверки.
Статический анализ не идеален, но он все же находит такие ошибки (и многие другие!). Так почему бы вам не попробовать анализатор и не проверить свои проекты? Возможно, вы тоже найдете что-то интересное.
Спасибо и до встречи в следующих статьях!
