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

В 2021 году мы опубликовали несколько статей и показали вам ошибки, обнаруженные в проектах с открытым исходным кодом. Заканчивается 2021 год, а значит, пора представить вам традиционный топ-10 самых интересных багов. Наслаждаться!

Небольшое введение

Как и в Статье 2020, мы ранжировали предупреждения по следующим принципам:

велика вероятность того, что в коде присутствует ошибка;
эта ошибка должна быть интересной, редкой и необычной;
предупреждения в списке должны быть разнообразными — про одни и те же ошибки читать не хочется, правда?

Надо признать, статей о проверке C# проектов было немного. Предупреждения в этом списке часто относятся к одним и тем же проектам. Как-то так получилось, что большинство предупреждений были взяты из статей про DNN и PeachPie.

С другой стороны, ошибки, обнаруженные в этом году, не похожи друг на друга — все предупреждения были выданы разными диагностиками!

С тяжелым сердцем я вычеркивал предупреждения, которые были хорошими, но менее интересными, чем другие. Иногда приходилось вычеркивать предупреждения ради разнообразия. Итак, если вам нравятся обзоры предупреждений анализатора, можете посмотреть другие статьи. Кто знает, может вас впечатлит то, о чем я не писал. Пишите в комментариях свои топ-10 — с удовольствием прочитаю :).

10 место. Время не меняется

Начнем наш топ с предупреждения из статьи PeachPie:

using System_DateTime = System.DateTime;
internal static System_DateTime MakeDateTime(....) { .... }
public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);
  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}

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

V3010 Необходимо использовать возвращаемое значение функции AddHours. DateTimeFunctions.cs 1232
V3010 Необходимо использовать возвращаемое значение функции AddHours. DateTimeFunctions.cs 1239

Эти предупреждения говорят об одном и том же, поэтому я решил их объединить.

Анализатор говорит, что результаты звонков нужно куда-то записать. В противном случае они просто не имеют смысла. Такие методы, как AddHours, не изменяют исходный объект. Вместо этого они возвращают новый объект, отличающийся от исходного на количество часов, прописанное в вызове аргумента. Трудно сказать, насколько серьезна ошибка, но код работает некорректно.

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

9 место. Четвертый элемент присутствует, но лучше получить исключение

9 место за предупреждением из статьи Ryujinx:

public uint this[int index]
{
  get
  {
    if (index == 0)
    {
      return element0;
    }
    else if (index == 1)
    {
      return element1;
    }
    else if (index == 2)
    {
      return element2;
    }
    else if (index == 2)   // <=
    {
      return element3;
    }
    throw new IndexOutOfRangeException();
  }
}

Предупреждение PVS-Studio: V3003 Обнаружено использование паттерна if (A) {…} else if (A) {…}. Существует вероятность наличия логической ошибки. Проверить строки: 26, 30. ZbcSetTableArguments.cs 26

Очевидно, все будет хорошо, пока кто-то не захочет получить третий элемент. И если они это сделают, будет выброшено исключение. Это нормально, но почему с element3 никогда не выполняется блок?

Удивительно, но часто развиваются ситуации, вызванные опечатками с цифрами 0,1,2. Об этом есть целая статья — очень рекомендую прочитать. И мы идем дальше.

8 место. Полезный вызов Debug.WriteLine

Я взял это предупреждение из упомянутой выше статьи PeachPie. Удивительно, что код выглядит совершенно нормально и совсем не подозрительно:

public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";
  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);
  var config = ctx.Configuration.Core;
  
  ....
}

Что с этим не так? Все выглядит хорошо. Выполняются назначения, затем вызывается перегрузка Debug.WriteLine. В качестве первого аргумента эта перегрузка принимает… Подождите! Каков правильный порядок аргументов здесь?

Итак, давайте посмотрим на объявление Debug.WriteLine:

public static void WriteLine(string format, params object[] args);

Согласно сигнатуре, строка формата должна быть передана в качестве первого аргумента. Во фрагменте кода первым аргументом является «MAILER», а сам формат идет в массив args, который вообще ни на что не влияет.

PVS-Studio предупреждает, что все аргументы форматирования игнорируются: V3025: Неверный формат. При вызове функции WriteLine ожидается другое количество элементов формата. Неиспользуемые аргументы: 1-й, 2-й, 3-й, 4-й, 5-й. Mail.cs 25

В результате на выходе будет просто «MAILER» без какой-либо другой полезной информации. Но мы бы хотели это увидеть :(.

7 место. Еще один вопрос

7 место снова за предупреждением от PeachPie.

Часто разработчики пропускают нулевые проверки. Особенно интересна ситуация, когда в одном месте переменная была проверена, а в другом нет (где она тоже может быть нулевой). Возможно, разработчики забыли это сделать или просто проигнорировали. Мы можем только догадываться, была ли проверка избыточной или нам нужно добавить еще одну проверку где-то в коде. Для проверки на null не всегда требуются операторы сравнения: например, в приведенном ниже фрагменте кода разработчик использовал null-условный оператор:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }
  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

Предупреждение V3105: переменная tinfo использовалась после того, как она была назначена через оператор с нулевым условием. NullReferenceException возможно. Объекты.cs 189

Разработчик считает, что вызов Type.GetTypeFromHandle(вызывающий) может возвращать значение null. Вот почему они использовали ? для вызова GetPhpTypeInfo. Согласно документации, это возможно.

Да, "?." спасает от одного исключения. Если вызов GetTypeFromHandle возвращает значение null, значение null также записывается в переменную tinfo. Однако, если мы попытаемся получить доступ к свойству BaseType, возникнет другое исключение. Когда я просмотрел код, я пришел к выводу, что еще один «?» отсутствует: return tinfo?.BaseType?.Name;

Однако решить эту проблему могут только разработчики. Это именно то, что они сделали после того, как мы отправили им отчет об ошибке. Вместо дополнительной проверки null они решили явно генерировать исключение, если GetTypeFromHandle возвращает null:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }
  
  // cannot be null; caller is either default or an invalid handle
  var t =    Type.GetTypeFromHandle(caller) 
          ?? throw new ArgumentException("", nameof(caller));
  var tinfo = t.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

Нам пришлось отформатировать код для этой статьи. Вы можете найти этот метод, перейдя по ссылке.

6 место. Неделя, которая длилась день

Иногда кажется, что время замедляется. Вам кажется, что прошла целая неделя, но это был только один день. Ну а на 6 месте у нас предупреждение из статьи DotNetNuke. Анализатор сработал по коду, в котором неделя состоит только из одного дня:

private static DateTime CalculateTime(int lapse, string measurement)
{
  var nextTime = new DateTime();
  switch (measurement)
  {
    case "s":
      nextTime = DateTime.Now.AddSeconds(lapse);
      break;
    case "m":
      nextTime = DateTime.Now.AddMinutes(lapse);
      break;
    case "h":
      nextTime = DateTime.Now.AddHours(lapse);
      break;
    case "d":
      nextTime = DateTime.Now.AddDays(lapse);   // <=
      break;
    case "w": 
      nextTime = DateTime.Now.AddDays(lapse);   // <=
      break;
    case "mo":
      nextTime = DateTime.Now.AddMonths(lapse);
      break;
    case "y":
      nextTime = DateTime.Now.AddYears(lapse);
      break;
  }
  return nextTime;
}

Предупреждение PVS-Studio: V3139 Две и более кейс-ветки выполняют одинаковые действия. DotNetNuke.Tests.Core PropertyAccessTests.cs 118

Очевидно, что функция должна возвращать DateTime, соответствующую некоторому моменту времени после текущего. Как-то так получилось, что буква «w» (подразумевающая «неделя») обрабатывается так же, как «d». Если мы попытаемся получить дату, через неделю от текущего момента, мы получим следующий день!

Обратите внимание, что при изменении неизменяемых объектов ошибки не возникает. Тем не менее, странно, что код для ветвей «d» и «w» одинаков. Стандартного метода AddWeeks в DateTime, конечно, нет, но можно добавить 7 дней :).

5 место. Логические операторы и null

На 5 месте одно из моих любимых предупреждений из статьи PeachPie. Предлагаю сначала внимательно посмотреть на этот фрагмент и найти здесь ошибку.

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         &&    langVersion.Major > 7 
            || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

В чем проблема?

Я думаю, вы легко нашли здесь ошибку. Действительно легко, если знать, где искать :). Должен признаться, что пытался вас запутать и немного изменил форматирование. На самом деле логическая конструкция была написана одной строкой.

Теперь давайте посмотрим на версию, отформатированную в соответствии с приоритетами операторов:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

Предупреждение PVS-Studio V3080: Возможно нулевое разыменование. Рассмотрите возможность проверки langVersion. AnalysisFacts.cs 20

Код проверяет, что переданный параметр langVersion не равен нулю. Разработчик предположил, что при вызове метода IsAutoloadDeprecated может быть передано значение null. Спасает ли нас чек?

Нет. Если переменная langVersion имеет значение null, значение первой части выражения равно false. Когда мы вычисляем вторую часть, возникает исключение.

Судя по комментарию, либо перепутали приоритеты операторов, либо разработчики просто неправильно поставили скобку. Кстати, эта и другие ошибки ушли (я так думаю) — мы отправили баг-репорт разработчикам, и они их быстро исправили. Вы можете увидеть новую версию функции IsAutoloadDeprecated здесь.

4 место. Обработка несуществующей страницы

Мы почти в шаге от финалистов. А до этого — 4 место. А вот и предупреждение из последней статьи об Умбрако. Что же мы имеем здесь?

public ActionResult<PagedResult<EntityBasic>> GetPagedChildren(....
                                                               int pageNumber,
                                                               ....)
{
  if (pageNumber <= 0)
  {
    return NotFound();
  }
  ....
  if (objectType.HasValue)
  {
    if (id == Constants.System.Root &&
        startNodes.Length > 0 &&
        startNodes.Contains(Constants.System.Root) == false &&
        !ignoreUserStartNodes)
    {
      if (pageNumber > 0)  // <=
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }
      IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, 
                                                  startNodes).ToArray();
      if (nodes.Length == 0)
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }
      if (pageSize < nodes.Length)
      {
        pageSize = nodes.Length; // bah
      }
      var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
      {
        Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
      };
      return pr;
    }
  }
}

Предупреждение PVS-Studio: V3022 Выражение ‘pageNumber › 0’ всегда истинно. EntityController.cs 625

Итак, pageNumber — это параметр, который не меняется внутри метода. Если его значение меньше или равно 0, выходим из функции. Далее код проверяет, больше ли pageNumber 0.

Здесь у нас возникает вопрос: какое значение мы должны передать в pageNumber, чтобы условия pageNumber ‹= 0 и pageNumber › 0 стали ложными?

Очевидно, что такой ценности нет. Если проверка pageNumber ‹= 0 имеет значение false, то pageNumber › 0 всегда истинно. Это страшно? Давайте посмотрим на код после проверки Always-True:

if (pageNumber > 0)
{
  return new PagedResult<EntityBasic>(0, 0, 0);
}
IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, 
                                            startNodes).ToArray();
if (nodes.Length == 0)
{
  return new PagedResult<EntityBasic>(0, 0, 0);
}
if (pageSize < nodes.Length)
{
  pageSize = nodes.Length; // bah
}
var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
{
  Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
};
return pr;

Поскольку проверка в начале этого фрагмента всегда true, метод всегда завершается. А как насчет кода ниже? Он содержит кучу значимых операций, но ни одна из них никогда не выполняется!

Это выглядит подозрительно. Если pageNumber меньше или равен 0, возвращается результат по умолчанию — NotFound(). Кажется логичным. Однако, если параметр больше 0, мы получим… что-то похожее на результат по умолчанию — new PagedResult‹EntityBasic›(0, 0, 0). И как добиться нормального результата? Не понятно :(.

3д место. Самая редкая ошибка

Итак, вот и финалисты. Третье место за диагностикой V3122, которая долгое время не выявляла ошибок в open-source проектах. Наконец, в 2021 году мы проверили DotNetNuke и нашли даже 2 предупреждения V3122!

Итак, представляю вам 3 место:

public static string LocalResourceDirectory
{
  get
  {
    return "App_LocalResources";
  }
}
private static bool HasLocalResources(string path)
{
  var folderInfo = new DirectoryInfo(path);
  if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
  {
    return true;
  }
  ....
}

Предупреждение PVS-Studio: V3122 Строка path.ToLowerInvariant() в нижнем регистре сравнивается со строкой в смешанном регистре Localization.LocalResourceDirectory. Dnn.PersonaBar.Extensions LanguagesController.cs 644

Разработчики преобразуют значение пути в нижний регистр. Затем они проверяют, заканчивается ли она строкой, содержащей символы верхнего регистра — «App_LocalResources» (литерал, возвращенный из свойства LocalResourceDirectory). Очевидно, эта проверка всегда возвращает false и все выглядит подозрительно.

Это предупреждение напоминает мне, что независимо от того, сколько ошибок мы видели, всегда есть что-то, что может нас удивить. Идем дальше :).

2-е место. Нет выхода

На втором месте отличное предупреждение из статьи ILSpy, написанной в начале 2021 года:

private static void WriteSimpleValue(ITextOutput output,
                                     object value, string typeName)
{
  switch (typeName)
  {
    case "string":
      output.Write(  "'"
                   + DisassemblerHelpers
                      .EscapeString(value.ToString())
                      .Replace("'", "\'")                   // <=
                   + "'");
      break;
    case "type":
    ....
  }
  ....
}

V3038 Аргумент «« несколько раз передавался в метод Заменить. Возможно, вместо этого следует передать другой аргумент. ICSharpCode.Decompiler ReflectionDisassembler.cs 772

Похоже, разработчик хотел заменить все вхождения одинарных кавычек строкой, состоящей из двух символов: обратной косой черты и символа одинарной кавычки. Однако из-за особенностей escape-последовательностей второй аргумент — это просто символ одинарной кавычки. Поэтому здесь без замены.

Я придумал две идеи:

разработчики забыли поставить символ @ перед второй строкой. Этот символ просто позволит сохранить «\» как отдельный символ;
Они должны были поставить дополнительный «\» перед первым во втором аргументе. Первый будет экранировать второй, что означает, что в последней строке будет только один «\».

1-е место. Призрачная угроза

Итак, мы наконец добрались до самой интересной и необычной ошибки 2021 года. Эта ошибка из упомянутой выше статьи DotNetNuke.

Что еще более интересно, ошибка примитивна, но человеческий глаз не замечает подобные ошибки без инструментов статического анализа. Смелое заявление? Ну тогда попробуйте найти здесь ошибку (если она есть, конечно):

private void ParseTemplateInternal(...., string templatePath, ....)
{
  ....
  string path = Path.Combine(templatePath, "admin.template");
  if (!File.Exists(path))
  {
    // if the template is a merged copy of a localized templte the
    // admin.template may be one director up
    path = Path.Combine(templatePath, "..\admin.template");
  }
  ....
}

Ну как дела? Не удивлюсь, если найдете ошибку. В конце концов, если вы знаете, что он существует, вы быстро его увидите. А если не нашли — тоже ничего удивительного. Не так просто увидеть опечатку в комментарии — «шаблон» вместо «шаблон» :).

Шутки. Конечно, есть реальная ошибка, которая нарушает работу программы. Давайте еще раз посмотрим на код:

private void ParseTemplateInternal(...., string templatePath, ....)
{
  ....
  string path = Path.Combine(templatePath, "admin.template");
  if (!File.Exists(path))
  {
    // if the template is a merged copy of a localized templte the
    // admin.template may be one director up
    path = Path.Combine(templatePath, "..\admin.template");
  }
  ....
}

Предупреждение PVS-Studio: V3057 Ожидается, что функция Объединить получит допустимую строку пути. Проверьте второй аргумент. DotNetNuke.Library PortalController.cs 3538

Здесь у нас есть две операции для построения пути (вызов Path.Combine). Первая хороша, а вторая нет. Понятно, что во втором случае разработчики хотели взять файл admin.template не из каталога templatePath, а из родительского. Увы! После того, как они добавили ..\, путь стал недействительным, так как была сформирована escape-последовательность: ..\admin.template.

Похоже на предыдущее предупреждение, верно? Не совсем. Тем не менее, решение то же самое: добавить «@» перед строкой или дополнительный «\».

0 место. «смеется» против Visual Studio

Ну а так как первый элемент коллекции имеет индекс 0, то и наша коллекция должна иметь 0 место!

Конечно, погрешность здесь особенная, выходящая за пределы обычного верха. И все же стоит упомянуть, так как ошибка была обнаружена в всеми любимой Visual Studio 2022. И при чем тут статический анализ? Что ж, давайте найдем на него ответ.

Мой товарищ по команде Сергей Васильев нашел эту проблему и описал ее в статье Как Visual Studio 2022 съела 100 ГБ памяти и при чем тут XML-бомбы». Вот вкратце опишу ситуацию.

В Visual Studio 2022 Preview 3.1 добавление в проект определенного XML-файла приводит к отставанию IDE. А значит вместе с этой IDE пострадает все. Вот пример такого файла:

<?xml version="1.0"?>
<!DOCTYPE lolz [
 <!ENTITY lol "lol">
 <!ELEMENT lolz (#PCDATA)>
 <!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
 <!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
 <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
 <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
 <!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
 <!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
 <!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
 <!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
 <!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
 <!ENTITY lol10 "&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;">
 <!ENTITY lol11 
   "&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;">
 <!ENTITY lol12 
   "&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;">
 <!ENTITY lol13 
   "&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;">
 <!ENTITY lol14 
   "&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;">
 <!ENTITY lol15 
   "&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;">
]>
<lolz>&lol15;</lolz>

Как оказалось, Visual Studio была уязвима для атаки XEE. Пытаясь расширить все эти lol-объекты, IDE зависла и сожрала огромное количество оперативной памяти. В итоге он просто съел всю возможную память :(.

Эта проблема была вызвана использованием небезопасно настроенного синтаксического анализатора XML. Этот синтаксический анализатор позволяет обрабатывать DTD и не устанавливает ограничений для сущностей. Мой совет: не читайте внешние файлы из неизвестного источника с помощью парсера XML. Это приведет к DoS атаке.

Статический анализ помогает найти такие проблемы. Кстати, недавно PVS-Studio представила новую диагностику для выявления потенциальных XEE — V5615.

Мы отправили Visual Studio отчет об ошибке об этом, и они исправили это в новой версии. Хорошая работа, Майкрософт! :)

Заключение

К сожалению, в 2021 году мы написали не так много статей о реальных проверках проектов. С другой стороны, мы написали ряд других статей, связанных с C#. Ссылки вы найдете в конце этой статьи.

Выбрать интересные предупреждения для этого топа было несложно. Но выбрать 10 лучших было непросто, так как их было гораздо больше.

Оценивать их тоже было адской задачей — топ субъективен, так что не принимайте места слишком близко к сердцу :). Так или иначе, все эти предупреждения серьезны и еще раз напоминают нам, что мы поступаем правильно.

Вы уверены, что в вашем коде нет таких проблем? Вы уверены, что ошибки не прячутся между строк? Возможно, вы никогда не можете быть уверены в этом с большим проектом. Однако эта статья показывает, что небольшие (и не очень) ошибки можно найти с помощью статического анализатора. Поэтому я приглашаю вас попробовать PVS-Studio на своих проектах.

Ну вот и все. С Новым годом и до скорой встречи!

Интересные статьи в 2021 году

Я собрал несколько статей, за которыми можно наверстать упущенное долгими зимними вечерами :).