# Игра с null: проверка MonoGame статическим анализатором PVS\-Studio Анализатор PVS\-Studio уже не раз был использован для анализа кода библиотек, фреймворков и движков для разработки игр\. Пришло время добавить к их списку MonoGame – низкоуровневый gamedev\-фреймворк, написанный на языке C\#\. ![0915_MonoGame_ru/image1.png](https://import.viva64.com/docx/blog/0915_MonoGame_ru/image1.png) ## Введение MonoGame – это фреймворк с открытым исходным кодом, используемый для разработки игр\. Является идейным наследником проекта [XNA](https://en.wikipedia.org/wiki/Microsoft_XNA), который разрабатывался Microsoft до 2013 года\. Думаю, нелишним будет напомнить и про то, что такое [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/) :\)\. Но если даже лишним для наших постоянных читателей, то все равно напомню :\) PVS\-Studio — статический анализатор кода, который позволяет находить различные ошибки, а также проблемы, связанные с безопасностью приложений\. В этой статье был использован анализатор версии 7\.16 и [исходники MonoGame](https://github.com/MonoGame/MonoGame) от 12\.01\.2022\. Стоит сказать, что анализатор выдал пару предупреждений для кода используемых в проекте библиотек – DotNetZip и NVorbis\. Ради интереса я привёл их в этой статье\. Вы же при желании можете легко [исключить из проверки сторонний код](https://pvs-studio.ru/ru/docs/manual/0014/)\. ## Предупреждения анализатора **Issue 1** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3106/) Possible negative index value\. The value of 'i' index could reach \-1\. MonoGame\.Framework\.DesktopGL\(netstandard2\.0\) Cue\.cs 251 Анализатор сообщает о том, что переменная _i_, используемая в качестве индекса, может принимать значение \-1\. Эта переменная_ _инициализируется возвращаемым значением метода _FindVariable_\. Посмотрим, что у него внутри: ```cpp 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_: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_ инициализируется таким выражением: ```cpp int len = rows * columns * elementSize; ``` Если вы внимательно вглядитесь в код, то вас может посетить чувство дежавю, ведь это выражение встречается в коде ещё один раз: ```cpp Buffer.BlockCopy(data as Array, 0, _buffer, offset, rows*columns*elementSize); // <= ``` Скорее всего, _len_ должна быть на месте этого выражения\. **Issue 4** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_: ```cpp public class ShaderInfo { .... public Dictionary SamplerStates = new Dictionary(); } ``` Оказывается, что это словарь,_ _а_ Add_ – стандартный метод\. Действительно, _null_ не может быть ключом словаря\. В качестве ключа словаря передаётся значение поля _sampler\.Name_\. Потенциальный _null_ может быть присвоен в этой строке: ```cpp sampler.Name = this.GetValue(tree, TokenType.Identifier, 0) as string; ``` Результатом приведения через оператор _as_ будет _null_, если метод _GetValue_ вернёт _null_ или не экземпляр типа _string_\. Может ли такое быть? Посмотрим внутрь _GetValue_: ```cpp 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_ в двух случаях: 1. Если переданное значение _index_ меньше 0; 1. Если не был найден подходящий по переданному _type_ элемент коллекции _nodes_\. Стоит добавить проверку на _null_ для возвращаемого значения оператора _as_\. **Issue 5** ```cpp internal void Update() { if (GetQueuedSampleCount() > 0) { BufferReady.Invoke(this, EventArgs.Empty); } } ``` Предупреждение PVS\-Studio: [V3083](https://pvs-studio.ru/ru/docs/warnings/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_ объявлено вот так: ```cpp public event EventHandler BufferReady; ``` Важно отметить, что модификатор доступа _public_ позволяет другим программистам использовать событие _BufferReady_ в любом коде, что повышает шанс проведения операций с событием в других потоках\. Соответственно, даже добавление проверки на _null_ в само условие не спасает, ведь состояние _BufferReady_ между проверкой и вызовом может быть изменено\. Самый простой вариант исправления – добавление Elvis operator '?\.' в вызов _Invoke_: ```cpp BufferReady?.Invoke(this, EventArgs.Empty); ``` Если такой способ по каким\-то причинам недоступен, то следует присвоить _BufferReady_ локальной переменной и работать с ней: ```cpp EventHandler bufferReadyLocal = BufferReady; if (bufferReadyLocal != null) bufferReadyLocal.Invoke(this, EventArgs.Empty); ``` Ошибки _public_ событий в многопоточном коде могут появляться редко, но являются очень коварными\. Такие ошибки сложно или даже невозможно воспроизвести\. Подробнее тема безопасной работы с событиями описана в документации к диагностике [V3083](https://pvs-studio.ru/ru/docs/warnings/v3083/)\. **Issue 6** ```cpp public override TOutput Convert( 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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'processor'\. MonoGame\.Framework\.Content\.Pipeline PipelineProcessorContext\.cs 55 Анализатор предупреждает о возможном разыменовании нулевой ссылки при вызове _processor\.Process_\. Объект класса _processor_ создаётся вызовом _\_manager\.CreateProcessor_\. Посмотрим часть его кода: ```cpp public IContentProcessor CreateProcessor( string name, OpaqueDataDictionary processorParameters) { var processorType = GetProcessorType(name); if (processorType == null) return null; .... } ``` Из кода следует, что _CreateProcessor_ вернёт _null_ в случае, если и метод _GetProcessorType_ вернёт _null_\. Что ж, давайте посмотрим и на его код: ```cpp 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_? Этот метод является реализацией контракта из абстрактного класса\. Вот сам абстрактный метод: ```cpp /// /// Converts a content item object using the specified content processor. ///.... /// Optional processor /// for this content. ///.... public abstract TOutput Convert( TInput input, string processorName, OpaqueDataDictionary processorParameters ); ``` Комментарий ко входному параметру _processorName_ уверяет программиста, что этот параметр необязательный\. Возможно, программист, увидев такой комментарий для сигнатуры, будет уверен, что в реализациях контракта были сделаны проверки на _null_ или пустоту строки\. Но в данной реализации проверок нет\. Обнаружение анализатором потенциального разыменования нулевой ссылки в одном месте позволило найти большое количество вероятных источников проблем, например: * для корректной работы требуется непустое и не _null_ значение строки вопреки комментарию к сигнатуре абстрактного метода; * большое количество возвратов _null_\-значений, обращение к которым происходит без проверки и, как следствие, может привести к _NullReferenceException_\. **Issue 7** ```cpp public MGBuildParser(object optionsObject) { .... foreach(var pair in _optionalOptions) { var fi = GetAttribute(pair.Value); if(!string.IsNullOrEmpty(fi.Flag)) _flags.Add(fi.Flag, fi.Name); } } ``` Предупреждение PVS\-Studio: [V3146](https://pvs-studio.ru/ru/docs/warnings/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_ из предупреждения анализатора находится там, поиск не занял много времени: ```cpp static T GetAttribute(ICustomAttributeProvider provider) where T : Attribute { return provider.GetCustomAttributes(typeof(T),false) .OfType() .FirstOrDefault(); } ``` Следует использовать _null_\-условный оператор для защиты от _NullReferenceException_: ```cpp if(!string.IsNullOrEmpty(fi?.Flag)) ``` Тогда, если _fi_ – _null_, то при попытке обратиться к свойству _Flag_, мы получим не исключение, а _null_\. Возвращаемым значением _IsNullOrEmpty_ для _null_\-аргумента будет обычный _false_\. **Issue 8** ```cpp public GenericCollectionHelper(IntermediateSerializer serializer, Type type) { var collectionElementType = GetCollectionElementType(type, false); _contentSerializer = serializer.GetTypeSerializer(collectionElementType); .... } ``` Предупреждение PVS\-Studio: [V3080](https://pvs-studio.ru/ru/docs/warnings/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_: ```cpp public ContentTypeSerializer GetTypeSerializer(Type type) { .... if (type.IsArray) { .... } .... } ``` Очевидно, что если параметр _type_ будет равен _null_, то обращение к свойству _IsArray_ приведёт к выбрасыванию исключения\. Переданный _collectionElementType _инициализируется возвращаемым значением метода _GetCollectionElementType_\. Посмотрим, что у метода внутри: ```cpp 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** ```cpp class Floor0 : VorbisFloor { int _rate; .... int[] SynthesizeBarkCurve(int n) { var scale = _bark_map_size / toBARK(_rate / 2); .... } } ``` Предупреждение PVS\-Studio: [V3041](https://pvs-studio.ru/ru/docs/warnings/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_ выглядит так: ```cpp static float toBARK(double lsp) ``` Поле _\_rate_ имеет тип _int_, и результат деления переменной целочисленного типа на значение этого же типа также будет целочисленным – дробная часть будет потеряна\. Если такое поведение не предполагалось, то для получения в результате деления значения типа _double_ можно, например, добавить литерал _d_ к числу или написать это число в виде с точкой: ```cpp var scale = _bark_map_size / toBARK(_rate / 2d); var scale = _bark_map_size / toBARK(_rate / 2.0); ``` **Issue 10** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_ без проверки использует\. Статический анализ, конечно же, неидеален, но всё же он позволяет обнаруживать подобные проблемы \(и не только их\!\)\. Поэтому я предлагаю вам [попробовать анализатор](https://pvs-studio.ru/ru/pvs-studio/try-free/) и также проверить интересные вам проекты – вдруг что\-нибудь найдётся? Большое спасибо за внимание, увидимся в следующих статьях\!