Топ-10 ошибок, найденных в C#-проектах за 2023 год

За 2023 год разработчиками PVS-Studio было написано немало статей о проверке Open Source C#-проектов. По традиции мы делимся с вами 10-ю самыми интересными ошибками, найденными за этот год. Приятного чтения!

Как попасть в топ?

Для попадания в топ нужно соответствовать нескольким критериям:

• быть кодом из Open Source проекта;

• быть проанализированным с помощью PVS-Studio;

• с большой вероятностью содержать в себе ошибку;

• быть интересным для рассмотрения.

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

Предыдущие подборки C# багов можно найти здесь:

• топ-10 ошибок за 2022 год;

• топ-10 ошибок за 2021 год;

• топ-10 ошибок за 2020 год;

• топ-10 ошибок за 2019 год.

Не буду томить, приступим к просмотру новых срабатываний!

P.S. Не стоит слишком серьёзно относиться к распределению позиций в топе. Порядок во многом формировался исходя из субъективного мнения автора.

10 место. Неожиданный NullReferenceException

Наш топ открывает срабатывание анализатора из статьи о проверке MudBlazor:

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 Passing 'null' to '==' operator should not result in 'NullReferenceException'. ResizeOptions.cs 34

• V3115 Passing 'null' to '!=' operator should not result in 'NullReferenceException'. ResizeOptions.cs 35

В реализациях перегрузки операторов '==' и '!=' левый операнд не проверяется на null. Если значение параметра l – null, то при вызове метода Equals будет выброшено исключение типа NullReferenceException. Параметр метода Equals, кстати, так же разыменовывается без проверки:

public bool Equals(ResizeOptions other)
{
  if (ReportRate != other.ReportRate || ....)     // <=
  {
    return false;
  }
  ....
}

Такое поведение может стать весьма неприятным сюрпризом для пользователя, если класс публичный. Именно таким и является ResizeOptions.

Чтобы убедиться в наличии проблемы, попробуем сравнить объект типа ResizeOptions с null.

Вполне ожидаемо получаем исключение типа NullReferenceException. Если правый операнд — null, поведение будет аналогичным.

Кстати о необходимости учитывать такие случаи говорится в документации Microsoft.

9 место. Передвинули

На 9-ом месте расположилось срабатывание из статьи о проверке проекта Microsoft PowerToys:

public static List<PluginPair> 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 Shift by 48 bits is greater than the size of 'UInt32' type of expression '(uint)fileVersion.ProductMajorPart'. PluginManager.cs 62

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

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 The return value of function 'Add' is required to be utilized. Filter.cs 140

При вызове метода Add у переменной типа DateTime не было использовано возвращаемое значение. Скорее всего, разработчик хотел добавить значение _valueTime.Value к date. Однако он не учёл, что метод Add для объекта типа DateTime возвращает результат добавления, а не изменяет исходный объект. В итоге имеем бесполезный вызов, который по задумке наверняка должен был на что-то влиять.

7 место. Побитовая путаница

Следующее срабатывание было взято из статьи о проверке Ryujinx:

private void YesButton_Clicked(object sender, EventArgs args)
{
  ....
  Window.Functions = _mainWindow.Window.Functions =
    WMFunction.All & WMFunction.Close;
  ....
}

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

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

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 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 должно выглядеть так:

return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);

Кстати, рядом есть похожий метод с получением информации о Unity. Он написан по схожему шаблону, но возвращаемое значение уже формируется нормально:

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:

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

internal async Task<bool> 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 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 бессмысленна. Скорее всего, правильный вариант выглядит так:

(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#. Проект, в котором была обнаружена ошибка — Barotrauma. Описанное срабатывание хорошо демонстрирует мощь статического анализа, так как найти такую ошибку глазами совсем нелёгкая задача.

Рассмотрим фрагмент кода с ошибкой:

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. Possible null dereference. Consider inspecting 'decorativeSprite.Sprite'. Limb.cs 462.

В этом случае ошибка заключается в неправильном порядке выполнения операций, из-за чего в приведённом коде неизбежно будет выброшено исключение типа NullReferenceException. Чтобы это понять, достаточно посмотреть на реализацию метода decorativeSprite.Remove:

partial class DeformableSprite
{
  ....
  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 компоненты". Кстати, статья не зря имеет такое название :). PVS-Studio действительно научился анализировать Blazor компоненты. Если у вас есть Blazor проект, то предлагаю опробовать на нём анализатор.

Вернёмся к предупреждению. Анализатор выдал его на код из проекта MudBlazor.

Рассмотрим подозрительный фрагмент кода:

@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 Comparison of 'result' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead.

Анализатор сообщает, что сравнение с double.NaN бессмысленно. Но почему? Согласно MSDN сравнение двух NaN значений через оператор '==' всегда возвращает false. Для корректного сравнения необходимо использовать метод double.IsNaN.

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

1 место. А может ошибки нет?

Вот он — венец сегодняшнего топа. Почему именно это срабатывание на первом месте? Как мне кажется, проблема, которую оно показывает, является самой сложной для выявления относительно других представителей топа. Это срабатывание было взято из статьи о проверке проекта Microsoft PowerToys.

Предлагаю вам найти ошибку самостоятельно:

private static int CalculateClosestSpaceIndex(List<int> 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;
  }
}

Удалось определить, в чём же здесь проблема? Буду признателен, если вы поделитесь своим вариантом в комментариях. Только без подглядываний в описание ошибки :).

Заключение

За 2023 мы проверили немало C# проектов, но и нельзя сказать, что много. Однако это не помешало составить этот топ. Даже наоборот, есть ряд интересных срабатываний, которые не были добавлены сюда, так как не хотелось изменять традициям топа 10-ти.

Предлагаю почитать несколько достойных статей, ошибки из которых по той или иной причине не попали в этот топ:

• Возвращаемся на Гроув-Стрит. Анализ движка Grand Theft Auto: San Andreas на Unity;

• Обзор подозрительных мест в исходном коде MassTransit;

• Пять забавных странностей в коде Entity Framework Core.

Вы также можете попробовать составить собственный топ срабатываний или же просто проверить интересующий проект :). Для этого предлагаю вам попробовать PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. Top 10 errors found in C# projects in 2023.