# Топ\-10 ошибок, найденных в C\#\-проектах за 2023 год За 2023 год разработчиками PVS\-Studio было написано немало статей о проверке Open Source C\#\-проектов\. По традиции мы делимся с вами 10\-ю самыми интересными ошибками, найденными за этот год\. Приятного чтения\! ![1094_CSharpTop_2023_ru/image1.png](https://import.viva64.com/docx/blog/1094_CSharpTop_2023_ru/image1.png) ## Как попасть в топ? Для попадания в топ нужно соответствовать нескольким критериям: * быть кодом из Open Source проекта; * быть проанализированным с помощью PVS\-Studio; * с большой вероятностью содержать в себе ошибку; * быть интересным для рассмотрения\. Как можно заметить, критериев не так много\. Однако отобрать 10 лучших срабатываний оказалось не совсем тривиальной задачей\. Да, хотелось бы рассмотреть больше ошибок, но мы решили не изменять традициям и составили топ\-10\. Предыдущие подборки C\# багов можно найти здесь: * [топ\-10 ошибок за 2022 год](https://pvs-studio.ru/ru/blog/posts/csharp/1020/); * [топ\-10 ошибок за 2021 год](https://pvs-studio.ru/ru/blog/posts/csharp/0904/); * [топ\-10 ошибок за 2020 год](https://pvs-studio.ru/ru/blog/posts/csharp/0787/); * [топ\-10 ошибок за 2019 год](https://pvs-studio.ru/ru/blog/posts/csharp/0698/)\. Не буду томить, приступим к просмотру новых срабатываний\! P\.S\. Не стоит слишком серьёзно относиться к распределению позиций в топе\. Порядок во многом формировался исходя из субъективного мнения автора\. ## 10 место\. Неожиданный NullReferenceException Наш топ открывает срабатывание анализатора из [статьи о проверке MudBlazor](https://pvs-studio.ru/ru/blog/posts/csharp/1032/): ```cpp public static bool operator ==(ResizeOptions l, ResizeOptions r) => l.Equals(r); public static bool operator !=(ResizeOptions l, ResizeOptions r) => !l.Equals(r); ``` Предупреждения PVS\-Studio: * [V3115](https://pvs-studio.ru/ru/docs/warnings/v3115/) Passing 'null' to '\=\=' operator should not result in 'NullReferenceException'\. ResizeOptions\.cs 34 * [V3115](https://pvs-studio.ru/ru/docs/warnings/v3115/) Passing 'null' to '\!\=' operator should not result in 'NullReferenceException'\. ResizeOptions\.cs 35 В реализациях перегрузки операторов '\=\=' и '\!\=' левый операнд не проверяется на _null_\. Если значение параметра _l_ – _null_, то при вызове метода _Equals_ будет выброшено исключение типа _NullReferenceException_\. Параметр метода _Equals_, кстати, так же разыменовывается без проверки: ```cpp public bool Equals(ResizeOptions other) { if (ReportRate != other.ReportRate || ....) // <= { return false; } .... } ``` Такое поведение может стать весьма неприятным сюрпризом для пользователя, если класс публичный\. Именно таким и является _ResizeOptions_\. Чтобы убедиться в наличии проблемы, попробуем сравнить объект типа _ResizeOptions_ с _null_\. ![1094_CSharpTop_2023_ru/image2.png](https://import.viva64.com/docx/blog/1094_CSharpTop_2023_ru/image2.png) Вполне ожидаемо получаем исключение типа _NullReferenceException_\. Если правый операнд — _null_, поведение будет аналогичным\. ![1094_CSharpTop_2023_ru/image3.png](https://import.viva64.com/docx/blog/1094_CSharpTop_2023_ru/image3.png) Кстати о необходимости учитывать такие случаи говорится в [документации Microsoft](https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/statements-expressions-operators/how-to-define-value-equality-for-a-type)\. ## 9 место\. Передвинули На 9\-ом месте расположилось срабатывание из [статьи о проверке проекта Microsoft PowerToys](https://pvs-studio.ru/ru/blog/posts/cpp/1078/): ```cpp public static List AllPlugins { get { .... try { // Return a comparable produce version. var fileVersion = FileVersionInfo.GetVersionInfo(x.ExecuteFilePath); return ((uint)fileVersion.ProductMajorPart << 48) | ((uint)fileVersion.ProductMinorPart << 32) | ((uint)fileVersion.ProductBuildPart << 16) | ((uint)fileVersion.ProductPrivatePart); } catch (System.IO.FileNotFoundException) { return 0U; } .... } } ``` Предупреждения PVS\-Studio: * [V3134](https://pvs-studio.ru/ru/docs/warnings/v3134/) Shift by 48 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMajorPart'\. PluginManager\.cs 62 * [V3134](https://pvs-studio.ru/ru/docs/warnings/v3134/) Shift by 32 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMinorPart'\. PluginManager\.cs 63 Не обращайте внимания на тип свойства и на тип фактически возвращаемого значения\. Код пришлось сильно сократить, поэтому показанный отрывок относится к лямбда выражению\. Анализатор выдал на него два предупреждения\. Тип _uint_ имеет размер 32 бита\. Получается, что первый сдвиг на 48 бит эквивалентен сдвигу на 16\. Второй сдвиг на 32 бита эквивалентен сдвигу на 0 бит\. Сложно сказать, что здесь хотели сделать\. Но, возможно, вместо _uint_ стоит использовать _ulong_\. ## 8 место\. Сомнительный вызов На 8\-ом месте ещё одно срабатывание из ранее упомянутой [статьи о MudBlazor](https://pvs-studio.ru/ru/blog/posts/csharp/1032/): ```cpp internal void DateValueChanged(DateTime? value) { _valueDate = value; if (value != null) { var date = value.Value.Date; // get the time component and add it to the date. if (_valueTime != null) { date.Add(_valueTime.Value); // <= } _filterDefinition.Value = date; } else _filterDefinition.Value = value; _dataGrid.GroupItems(); } ``` Предупреждение PVS\-Studio: [V3010](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Add' is required to be utilized\. Filter\.cs 140 При вызове метода _Add_ у переменной типа _DateTime_ не было использовано возвращаемое значение\. Скорее всего, разработчик хотел добавить значение _\_valueTime\.Value_ к _date_\. Однако он не учёл, что метод _Add_ для объекта типа _DateTime_ возвращает результат добавления, а не изменяет исходный объект\. В итоге имеем бесполезный вызов, который по задумке наверняка должен был на что\-то влиять\. ## 7 место\. Побитовая путаница Следующее срабатывание было взято из [статьи о проверке Ryujinx](https://pvs-studio.ru/ru/blog/posts/csharp/1059/): ```cpp private void YesButton_Clicked(object sender, EventArgs args) { .... Window.Functions = _mainWindow.Window.Functions = WMFunction.All & WMFunction.Close; .... } ``` Предупреждение PVS\-Studio: [V3182](https://pvs-studio.ru/ru/docs/warnings/v3182/) The result of 'WMFunction\.All & WMFunction\.Close' expression is '0'\. It is possible that the '\|' operator should be used instead\. UpdateDialog\.cs 69 Как видно из предупреждения, анализатор ругается на оператор '&'\. Чтобы понять причину, давайте взглянем на перечисление _WMFunction_: ```cpp [Flags] public enum WMFunction { All = 0x1, Resize = 0x2, Move = 0x4, Minimize = 0x8, Maximize = 0x10, Close = 0x20 } ``` Мы имеем дело с битовыми флагами, но в данном случае реализация объединения не будет работать: результатом операции побитового И \(&\) для значений _WMFunction\.All_ и _WMFunction\.Close_ будет ноль\. Скорее всего, для корректной работы метода следует заменить '&' на '\|'\. ## 6 место\. Подозрительный String\.Format Движемся дальше\. Сейчас перед нами предстанет срабатывание из [статьи о проверке AWS SDK для \.NET](https://pvs-studio.ru/ru/blog/posts/csharp/1057/): ```cpp private static string GetXamarinInformation() { var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core"); if (xamarinDevice == null) { return null; } var runtime = xamarinDevice.GetProperty("RuntimePlatform") ?.GetValue(null) ?.ToString() ?? ""; var idiom = xamarinDevice.GetProperty("Idiom") ?.GetValue(null) ?.ToString() ?? ""; var platform = runtime + idiom; if (string.IsNullOrEmpty(platform)) { platform = UnknownPlatform; } return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin"); } ``` Предупреждение PVS\-Studio: [V3137](https://pvs-studio.ru/ru/docs/warnings/v3137/) The 'platform' variable is assigned but is not used by the end of the function\. InternalSDKUtils\.netstandard\.cs 70 Последняя строка метода выглядит очень странно\. С помощью _String\.Format_ в шаблон _"Xamarin\_\{0\}" _подставляют строковый литерал _"Xamarin"\._ При этом значение переменной _platform_, которое может хранить необходимую информацию, игнорируется\. Выглядит странно\. Скорее всего, выражение _return_ должно выглядеть так: ```cpp return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform); ``` Кстати, рядом есть похожий метод с получением информации о Unity\. Он написан по схожему шаблону, но возвращаемое значение уже формируется нормально: ```cpp private static string GetUnityInformation() { var unityApplication = Type.GetType("UnityEngine.Application, UnityEngine.CoreModule"); if (unityApplication == null) { return null; } var platform = unityApplication.GetProperty("platform") ?.GetValue(null) ?.ToString() ?? UnknownPlatform; return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform); } ``` ## 5 место\. Больше или меньше? Вторую половину топа открывает срабатывание из [статьи о проверке BTCPay Server](https://pvs-studio.ru/ru/blog/posts/csharp/1051/): ```cpp private IActionResult Validate(StoreBaseData request) { .... if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100) ModelState.AddModelError(nameof(request.PaymentTolerance), "PaymentTolerance can only be between 0 and 100 percent"); .... } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'request\.PaymentTolerance < 0 && request\.PaymentTolerance \> 100' is always false\. Probably the '\|\|' operator should be used here\. BTCPayServer\\Controllers\\GreenField\\GreenfieldStoresController\.cs 241 Каждый хоть раз сталкивался с отменой операции из\-за неверного ввода в финансовых приложениях\. Иногда они не очень удачно уведомляют пользователя о неверных данных, но иногда в коде просто есть глупая ошибка\. Так, в этом фрагменте кода перепутали операторы '&&' и '\|\|'\. Код предназначался для проверки значения на вхождение в диапазон от 0 до 100, но в итоге предупреждение пользователю не будет выдано\. Как результат — неверно введённое значение пройдёт дальше по коду и может привести к ошибке в логике приложения\. ## 4 место\. Неправильно расставленные приоритеты На 4\-ом месте расположилось срабатывание, которое практически ничем не уступает финалистам\. Его я взял из [статьи о проверке MudBlazor](https://pvs-studio.ru/ru/blog/posts/csharp/1032/)\. ```cpp internal async Task StartResizeColumn(....) { .... // In case resize mode is column, we have to find any column right // of the current one that can also be resized and is not hidden. var nextResizableColumn = _columns.Skip(_....) + 1) .FirstOrDefault(c => c.Resizable ?? true && !c.Hidden); // <= .... } ``` Предупреждение PVS\-Studio: [V3177](https://pvs-studio.ru/ru/docs/warnings/v3177/) The 'true' literal belongs to the '&&' operator with a higher priority\. It is possible the literal was intended to belong to '??' operator instead\. DataGridColumnResizeService\.cs 52 Рассмотрим выражение _c\.Resizable ?? true && \!c\.Hidden_\. Стоит сразу сказать, что приоритет у '??' ниже, чем у '&&'\. Исходя из этого, если _c\.Resizable_ — _null_, то оператор null\-coalescing вернёт результат операции _true && \!c\.Hidden_\. Операция "&&" с литералом _true_ бессмысленна\. Скорее всего, правильный вариант выглядит так: ```cpp (c.Resizable ?? true) && !c.Hidden ``` Разница этих записей проявит себя, если значения _c\.Resizable_ и _c\.Hidden_ будут равны _true_\. Есть ещё один момент, указывающий, что была допущена ошибка\. Строчкой выше написан комментарий, что столбец должен иметь возможность изменения размера \(информация об этом содержится в _c\.Resizable_\) и не быть скрытым \(это можно узнать из _c\.Hidden_\)\. Если _c\.Resizable_ — _true_, то логика работы будет нарушена, так как значение _c\.Hidden_ никак не повлияет на результат\. ## 3 место\. Порядок имеет значение И наконец мы добрались до тройки лидеров\. Её открывает срабатывание, которое я взял из [статьи про Обзор Top\-3 Open Source игр на C\#](https://pvs-studio.ru/ru/blog/posts/csharp/1056/)\. Проект, в котором была обнаружена ошибка — Barotrauma\. Описанное срабатывание хорошо демонстрирует мощь статического анализа, так как найти такую ошибку глазами совсем нелёгкая задача\. Рассмотрим фрагмент кода с ошибкой: ```cpp public void RecreateSprites() { .... for (int i = 0; i < DecorativeSprites.Count; i++) { var decorativeSprite = DecorativeSprites[i]; decorativeSprite.Remove(); var source = decorativeSprite.Sprite.SourceElement; DecorativeSprites[i] = new DecorativeSprite(source, ....); } } ``` Предупреждение PVS\-Studio: [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/)\. Possible null dereference\. Consider inspecting 'decorativeSprite\.Sprite'\. Limb\.cs 462\. В этом случае ошибка заключается в неправильном порядке выполнения операций, из\-за чего в приведённом коде неизбежно будет выброшено исключение типа _NullReferenceException_\. Чтобы это понять, достаточно посмотреть на реализацию метода _decorativeSprite\.Remove_: ```cpp partial class DecorativeSprite : ISerializableEntity { .... public Sprite Sprite { get; private set; } .... public void Remove() { Sprite?.Remove(); Sprite = null; .... } } ``` В указанном методе свойству _Sprite_ присваивается значение _null_\. Если ещё раз взглянуть на метод, в котором вызывается _Remove_, становится понятно, что при обращении к _decorativeSprite\.Sprite\.SourceElement_ будет выброшено исключение\. Такое поведение обусловлено вызовом _Remove_ перед обращением\. Вероятно, здесь нужно поменять местами вызов метода _Remove_ и инициализацию переменной _source_\. ## 2 место\. Сравнение с NAN С небольшим отставанием от победителя на втором месте расположилось срабатывание из статьи "[PVS\-Studio научился анализировать Blazor компоненты](https://pvs-studio.ru/ru/blog/posts/csharp/1023/)"\. Кстати, статья не зря имеет такое название :\)\. PVS\-Studio действительно научился анализировать Blazor компоненты\. Если у вас есть Blazor проект, то предлагаю [опробовать](https://pvs-studio.ru/ru/pvs-studio/try-free/) на нём анализатор\. Вернёмся к предупреждению\. Анализатор выдал его на код из проекта MudBlazor\. Рассмотрим подозрительный фрагмент кода: ```cpp @code { .... public void Evaluate() { .... var exp = new Expression(CalcExpression); var result = exp.Eval(); if (result == double.NaN) { Current = "ERROR"; return; } Current = Math.Round( result,8).ToString(CultureInfo.InvariantCulture); CalcExpression = Current; } .... } ``` Предупреждение PVS\-Studio: [V3076](https://pvs-studio.ru/ru/docs/warnings/v3076/) Comparison of 'result' with 'double\.NaN' is meaningless\. Use 'double\.IsNaN\(\)' method instead\. Анализатор сообщает, что сравнение с _double\.NaN_ бессмысленно\. Но почему? Согласно [MSDN](https://learn.microsoft.com/en-us/dotnet/api/system.double.op_equality?view=netframework-4.8) сравнение двух _NaN_ значений через оператор '\=\=' всегда возвращает _false_\. Для корректного сравнения необходимо использовать метод _double\.IsNaN_\. Вот такое простое срабатывание, но, думаю, оно достойно второй позиции в топе, так как для многих особенность сравнения с _NaN_ может быть не совсем очевидна\. ## 1 место\. А может ошибки нет? Вот он — венец сегодняшнего топа\. Почему именно это срабатывание на первом месте? Как мне кажется, проблема, которую оно показывает, является самой сложной для выявления относительно других представителей топа\. Это срабатывание было взято из [статьи о проверке проекта Microsoft PowerToys](https://pvs-studio.ru/ru/blog/posts/cpp/1078/)\. Предлагаю вам найти ошибку самостоятельно: ```cpp private static int CalculateClosestSpaceIndex(List spaceIndices, int firstMatchIndex) { if (spaceIndices.Count == 0) { return -1; } else { int? ind = spaceIndices.OrderBy(item => (firstMatchIndex - item)) .Where(item => firstMatchIndex > item) .FirstOrDefault(); int closestSpaceIndex = ind ?? -1; return closestSpaceIndex; } } ``` Удалось определить, в чём же здесь проблема? Буду признателен, если вы поделитесь своим вариантом в комментариях\. Только без подглядываний в описание ошибки :\)\.
Описание ошибки спрятано здесь\\\. Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'ind' is always not null\. The operator '??' is excessive\. StringMatcher\.cs 230 Анализатор посчитал, что оператор '??' не нужен, так как _ind_ всегда не равен _null_\. Так ли это на самом деле? _Ind_ имеет nullable тип, а значит в него можно записать _null_\. Значение, записанное в _Ind_, было получено с использованием метода _FirstOrDefault_, который может вернуть _null_\. Казалось бы, дело закрыто, анализатор ошибся\. Однако не всё так просто\. Копнём чуть глубже\. Если быть точнее, то _FirstOrDefault_ возвращает не _null_, а _default\(TSource\)_\. _default\(int?\)_ — это _null_\. Вот только _TSource_ в данном случае _int_, так как _TSource_ берётся по типу элементов перечисляемой последовательности\. В данном случае LINQ применяется к параметру _spaceIndices_ с типом _List_\. Получается, что в _ind_ будет записан _default\(int\)_, то есть 0\. А вот и ошибка в логике\. Метод поиска в случае ненахождения вернёт 0, а не \-1, как должен\.
## Заключение За 2023 мы проверили немало C\# проектов, но и нельзя сказать, что много\. Однако это не помешало составить этот топ\. Даже наоборот, есть ряд интересных срабатываний, которые не были добавлены сюда, так как не хотелось изменять традициям топа 10\-ти\. Предлагаю почитать несколько достойных статей, ошибки из которых по той или иной причине не попали в этот топ: * [Возвращаемся на Гроув\-Стрит\. Анализ движка Grand Theft Auto: San Andreas на Unity](https://pvs-studio.ru/ru/blog/posts/csharp/1083/); * [Обзор подозрительных мест в исходном коде MassTransit](https://pvs-studio.ru/ru/blog/posts/csharp/1061/); * [Пять забавных странностей в коде Entity Framework Core](https://pvs-studio.ru/ru/blog/posts/csharp/1070/)\. Вы также можете попробовать составить собственный топ срабатываний или же просто проверить интересующий проект :\)\. Для этого предлагаю вам [попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.