В течение 2024 года команда PVS-Studio активно делилась статьями о проверке открытых C# проектов. Мы решили продолжить традицию и отобрали для вас 10 самых интересных ошибок, обнаруженных за этот период. Приятного чтения!

Пара вводных слов

Уточню, по каким критериям составлялся топ:

• код из Open Source проекта;

• проблема в коде обнаружена с помощью PVS-Studio;

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

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

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

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

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

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

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

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

Уточню, что топ составлялся исходя из субъективного мнения автора. Если вам покажется, что та или иная ошибка находится не на своём месте, смело можете писать об этом в комментариях :)

Приступим же к самому интересному — просмотру ошибок!

10 место. Stack Overflow вместо сравнения

Открывает топ ошибка из статьи о Unity 6:

public readonly struct SearchField : IEquatable<SearchField>
{
  ....
  public override bool Equals(object other)
  {
    return other is SearchIndexEntry l && Equals(l);
  }

  public bool Equals(SearchField other)
  {
    return string.Equals(name, other.name, StringComparison.Ordinal);
  }
}

Предупреждение PVS-Studio:

V3197 The compared value inside the 'Object.Equals' override is converted to the 'SearchIndexEntry' type instead of 'SearchField' that contains the override. SearchItem.cs 634.

Как сказано в сообщении анализатора, в первом методе Equals параметр other ошибочно приводится к типу SearchIndexEntry вместо SearchField. Из-за этого при последующем вызове Equals(l) будет вызвана та же самая перегрузка метода. Если вдруг окажется, что other действительно имеет тип SearchIndexEntry, произойдёт зацикливание кода. Это приведёт к StackOverflowException.

Скорее всего, эта ошибка возникла из-за copy-paste.

9 место. Подозрительный foreach

Следующую позицию занимает ставшая уже классической ошибка из статьи про проверку Garnet:

public IEnumerable<long> GetPendingRequests()
{
  foreach (var kvp in ctx.prevCtx?.ioPendingRequests)
    yield return kvp.Value.serialNum;

  foreach (var kvp in ctx.ioPendingRequests)
    yield return kvp.Value.serialNum;
}

Предупреждение PVS-Studio:

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: ctx.prevCtx?.ioPendingRequests. ClientSession.cs 748

В этом фрагменте кода используется оператор ?.. Видимо, программист подразумевал, что prevCtx может иметь значение null. Вот только проблема в том, что foreach не работает с null. Получается, что исключение всё равно настигнет разработчика, но при вызове метода GetEnumerator.

Как мы уже неоднократно писали в других публикациях, для многих разработчиков это неочевидный момент. На эту тему у нас есть отдельная статья "Использование оператора ?. в foreach: защита от NullReferenceException, которая не работает".

С одной стороны, подобные ошибки в C# некритичны и быстро обнаруживаются (в отличие от C++), если ссылка станет нулевой. Возможно, в этом или другом коде нулевая ссылка — это редкая или вообще невозможная ситуация, поэтому в нём подобные ошибки присутствуют подолгу и в большом количестве. Но со временем это создаст проблемы. Падение программы из-за NullReferenceException может обернуться неприятным и долгим поиском проблемы. Особенно если ошибка появляется только у клиента, и воспроизвести её очень трудно или не получается вовсе.

8 место. Неиспользованное значение

На восьмом месте расположилась ошибка из статьи про проверку nopCommerce:

public virtual async Task<....> GetOrderAverageReportLineAsync(....)
{
  ....

  if (!string.IsNullOrEmpty(orderNotes))
  {
    query = from o in query
            join n in _orderNoteRepository.Table on o.Id equals n.OrderId
            where n.Note.Contains(orderNotes)
            select o;

    query.Distinct();                          // <=
  }

  ....
}

Предупреждение PVS-Studio:

V3010 The return value of function 'Distinct' is required to be utilized. OrderReportService.cs 342

Для удаления повторяющихся элементов коллекции можно использовать Distinct (метод LINQ). Так и хотели сделать разработчики в этом примере, но что-то пошло не так. Метод Distinct не модифицирует коллекцию, у которой вызывается. Следовательно, если не использовать возвращаемое значение метода, то вызов не имеет смысла. Именно такая ситуация возникла в рассматриваемом коде.

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

7 место. Ошибочный String.Format

Движемся дальше. Перед нами ещё одна ошибка, взятая из статьи о Unity 6:

void UpdateInfo()
{
  ....

  var infoLine3_format = "<color=\"white\">CurrentElement:" +
                         " Visible:{0}" +
                         " Enable:{1}" +
                         " EnableInHierarchy:{2}" +
                         " YogaNodeDirty:{3}";

  m_InfoLine3.text = string.Format(infoLine3_format,
                                   m_LastDrawElement.visible,
                                   m_LastDrawElement.enable,
                                   m_LastDrawElement.enabledInHierarchy,
                                   m_LastDrawElement.isDirty);

  var infoLine4_format = "<color=\"white\">" +
                         "Count of ZeroSize Element:{0} {1}%" +
                         " Count of Out of Root Element:{0} {1}%";

  m_InfoLine4.text = string.Format(infoLine4_format,
                                   countOfZeroSizeElement,
                                   100.0f * countOfZeroSizeElement / count,
                                   outOfRootVE,
                                   100.0f * outOfRootVE / count);
  ....
}

Предупреждение PVS-Studio:

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: 3rd, 4th. UILayoutDebugger.cs 179.

Обратите внимание на второй string.Format(....). В его строке формата (первый аргумент) имеется четыре placeholder'а. Значений для подстановки в строку формата также передаётся четыре. Проблема в том, что placeholder'ы содержат только 0 и 1. Следовательно, итоговая строка будет содержать значения только первых двух аргументов.

Исправленная строка формата выглядит следующим образом:

var infoLine4_format = "<color=\"white\">" +
                       "Count of ZeroSize Element:{0} {1}%" +
                       " Count of Out of Root Element:{2} {3}%";

6 место. А может, префикс?

Завершает первую половину топа ошибка из статьи про проверку эмулятора сервера Diablo 3:

public static void GrantCriteria(....)
{
  ....
  else
  {
    ....
    uint alcount = alreadycrits.CriteriaId32AndFlags8;

    var newrecord = D3.Achievements
                      ....
                      .SetQuantity32(alcount++) // <=
                      .Build();

    int critCount = UnserializeBytes(achievement.Criteria).Count;
    if (critCount >= 5)
    {
      GrantCriteria(client, 74987246353740);
    }
    client.Account.GameAccount.AchievementCriteria
                              .Remove(alreadycrits);

    client.Account.GameAccount.AchievementCriteria
                              .Add(newrecord);
  }
  ....
}

Предупреждение PVS-Studio:

V3159 Modified value of the 'alcount' operand is not used after the postfix increment operation. AchievementManager.cs 360

Ожидается, что alcount увеличивается на 1, после чего полученное значение передаётся в метод SetQuantity32. В действительности же всё происходит наоборот: сначала значение alcount передаётся в метод, и лишь после этого значение переменной увеличится на 1.

Исправить эту ошибку очень просто: нужно заменить выражение alcount++ на выражение alcount + 1 или ++alcount.

5 место. null невозможен!

Пятёрку лидеров открывает ошибка из статьи про проверку .NET 8. Стоит отметить, что этот проект был проверен в самом конце 2023 года, поэтому ошибки из него не вошли в предыдущий топ. Но я просто не мог не добавить в подборку ошибки из столь именитого проекта. Вот одна из них:

Instruction[]? GetArgumentsOnStack (MethodDefinition method)
{
  int length = method.GetMetadataParametersCount ();
  Debug.Assert (length != 0);
  if (stack_instr?.Count < length)
    return null;

  var result = new Instruction[length];
  while (length != 0)
    result[--length] = stack_instr!.Pop ();    // <=

  return result;
}

Предупреждение PVS-Studio:

V3125 The 'stack_instr!' object was used after it was verified against null. Check lines: 1918, 1913. UnreachableBlocksOptimizer.cs 1918

Разработчик использовал оператор ?., подразумевая, что поле stack_instr может быть null. И вроде бы всё хорошо, есть проверка, но не тут-то было. В указанной строчке возможно разыменование нулевой ссылки. Скорее всего, разработчик подумал, что выражение stack_instr?.Count < length при stack_instr равным null вернёт true, и произойдёт выход из метода, но нет — результатом будет false.

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

А как вы относитесь к nullable-контексту? Если интересно наше мнение, или если вы ещё не знакомы с этим механизмом, то предлагаю почитать статьи:

• Помоги компилятору, и он поможет тебе. Тонкости работы с nullable reference типами в C#

• Nullable Reference не защищают, и вот доказательства

• Nullable Reference типы в C# 8.0 и статический анализ

4 место. Всегда первый

На четвёртом месте также находится ошибка из статьи про проверку .NET 8:

private static string GetTypeNameDebug(TypeDesc type)
{
  string result;
  TypeDesc typeDefinition = type.GetTypeDefinition();
  if (type != typeDefinition)
  {
    result = GetTypeNameDebug(typeDefinition) + "<";
    for (int i = 0; i < type.Instantiation.Length; i++)
      result += (i == 0 ? "" : ",") + GetTypeNameDebug(type.Instantiation[0]);
    return result + ">";
  }
  else
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio:

V3102 Suspicious access to element of 'type.Instantiation' object by a constant index inside a loop. TypeLoaderEnvironment.GVMResolution.cs 32

Предположу, что в этом фрагменте кода из информации о типе формируется запись следующего вида: ConsoleApp1.Program.MyClass<string, int, double>. Вот только в цикле обращаются к объекту type.Instantiation по константному индексу, равному 0. Чтобы код работал корректно, нужно заменить 0 на переменную-счётчик GetTypeNameDebug(type.Instantiation[i]).

3 место. NRE из-за сборщика мусора

Бронзу получает ошибка из статьи о Unity 6. Во многом так высоко она расположилась из-за своей неочевидности. Все подробности далее:

[RequiredByNativeCode]
internal static void InvalidateAll()
{
  lock (s_Instances)
  {
    foreach (var kvp in s_Instances)
    {
      WeakReference wr = kvp.Value;
      if (wr.IsAlive)
        (wr.Target as TextGenerator).Invalidate();
     }
   }
}

Предупреждение PVS-Studio:

V3145 Unsafe dereference of a WeakReference target. The object could have been garbage collected between checking 'IsAlive' and accessing the 'Target' property. TextGenerator.cs 140.

Несмотря на небольшое количество кода, пожалуй, это самый сложный кейс в статье. Причин несколько:

1. Не все знакомы с понятием слабой ссылки (WeakReference). Объект, на который ссылается такая ссылка, в любой момент может быть удален сборщиком мусора, несмотря на её наличие.

2. Свойство WeakReference.IsAlive позволяет проверить, существует ли ещё объект, на который ссылается слабая ссылка. Но в приведённом коде не учитывается, что этот самый объект может быть очищен уже после прохождения проверки, перед разыменованием. В результате всё равно может быть выброшен NullReferenceException.

3. Можно было бы предположить, что оператор lock может защитить объект от сборки мусора. Однако после воспроизведения такого кейса выяснилось, что это не так.

Так как же защититься в этом случае? Чтобы наверняка избежать исключения NullReferenceException, следует создать сильную ссылку на объект. В таком случае сборщик мусора уже не сможет его удалить, пока ссылка остается актуальной. Проще говоря, нужно создать простую локальную переменную, ссылающуюся на этот объект, и дальше работать уже с ней. Таким образом, безопасная версия метода может выглядеть так:

[RequiredByNativeCode]
internal static void InvalidateAll()
{
  lock (s_Instances)
  {
    foreach (var kvp in s_Instances)
    {
      WeakReference wr = kvp.Value;
      var target = wr.Target;

      If (target != null)
        (target as TextGenerator).Invalidate();     
     }
   }
}

2 место. ReDoS

Второе место забирает ошибка из статьи про проверку ScreenToGif. Она примечательна тем, что также является уязвимостью. О том, что это за проблема, и к чему она может привести, расскажем далее:

public static string ReplaceRegexInName(string name)
{
  const string dateTimeFileNameRegEx = @"[?]([ymdhsfzgkt]+[-_ ]*)+[?]";  // <=

  if (!Regex.IsMatch(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase))
    return name;

  var match = Regex.Match(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase);
  var date = DateTime.Now.ToString(Regex.Replace(match.Value, "[?]", ""));

  return name.Replace(match.ToString(), date);
}

Предупреждение PVS-Studio:

V5626 Possible ReDoS vulnerability. Potentially tainted data from the 'name' variable is processed by regular expression that contains unsafe pattern: '(...sfzgkt]+...)+'

В этом коде есть ReDoS-уязвимость, которая способна замедлить или вовсе остановить работу приложения. Проблема заключается в использовании уязвимого регулярного выражения, в которое можно передать произвольную строку. Если входная строка будет составлена определённым образом, то приложение зависнет.

В коде присутствует уязвимое регулярное выражение "[?]([ymdhsfzgkt]+[-_ ]*)+[?]". Упростим его для наглядности — "[?]([ymdhsfzgkt]+)+[?]". Опасным здесь является паттерн "(x+)+". Он позволяет сильно замедлить работу регулярного выражения при передаче определённой строки. Стоит отметить, что эта уязвимость актуальна только для алгоритмов на основе недетерминированного конечного автомата.

Без особого труда удалось воспроизвести уязвимость в самом приложении:

Как видно на скриншоте, если ввести в название выходного файла определённую строку, приложение зависнет.

В нашем случае использовалась строка "?ymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgkt".

Конечно, вряд ли кто-то станет делать подобное название, поэтому такая уязвимость тут безобидна. Но если допустить подобное в коде, связанном с сервером, то можно замедлить его работу или даже остановить.

Более подробно об этой уязвимости можно прочитать в статье "Что такое катастрофический возврат и ReDoS-уязвимости".

PVS-Studio является SAST-решением, при помощи статического анализа производится поиск различных потенциальных уязвимостей. Это позволяет улучшать безопасность тестируемых приложений. Анализатор может находить множество дефектов безопасности, среди которых: SQL и LDAP-инъекции, XSS, XXE и другие.

1 место. Формат или интерполяция?

И, наконец, мы добрались до первого места топа. Ошибка из этого примера проста, но менее занимательной от этого она не становится. Как мне кажется, её легко может допустить любой разработчик. Кстати, она тоже из статьи про проверку .NET 8.

Предлагаю вам самостоятельно отыскать её:

public void WriteTo(TextWriter writer, int methodRva, bool dumpRva)
{
  ....
  switch (Flags & CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_KIND_MASK)
  {
    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_NONE:
      writer.Write($" CATCH: {0}", ClassName ?? "null");
      break;

    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_FILTER:
      writer.Write($" FILTER (RVA {0:X4})",
                   ClassTokenOrFilterOffset + methodRva);
      break;
    ....
  }
  ....
}

Заключение

Нельзя сказать, что 2024 год был богат на статьи про проверку C# проектов, но это не стало поводом отказаться от составления топа. Даже из относительно небольшого количества статей я постарался выбрать наиболее интересные и разнообразные ошибки. О том, получилось ли это, можете написать в комментариях :)

Вы также можете в любой момент проверить собственный проект, для этого оставляю здесь ссылку на страницу, с которой можно скачать PVS-Studio. Хороших вам праздников, увидимся в новом году!

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