Поиск багов в Mono: их тысячи!

Очень интересно проверять большие проекты. Как правило, нам удается найти необычные и своеобразные ошибки и рассказать о них людям. Кроме того, это отличный способ протестировать наш анализатор и улучшить все его аспекты. Я давно ждал, чтобы проверить «Моно»; и, наконец, у меня появилась возможность. Надо сказать, что эта проверка действительно себя оправдала, так как я смог найти много интересного. В этой статье речь пойдет о найденных нами ошибках и некоторых нюансах, возникших при проверке.

О проекте

Mono — это проект по созданию полноценной реализации .NET Framework, который является бесплатным и с открытым исходным кодом. Основной разработчик Mono — корпорация Xamarin, ранее Novell.

Mono представляет собой набор инструментов, включающий компилятор C#, среду реализации .NET-mono (с поддержкой JIT) и mint (без поддержки JIT), отладчик, набор библиотек, включающий реализации WinForms, ADO.NET и ASP.NET, а также компиляторы smcs (для создания приложений для Moonlight) и vbc (для приложений, написанных на VB.NET).

В рамках этого проекта также предусмотрена привязка графической библиотеки GTK+ к платформе .NET.

Исходный код доступен в репозитории на GitHub. Количество строк кода для анализа из репозитория, загруженного с GitHub, составило около 3,6 млн (без учета пустых строк). Такая большая кодовая база выглядит очень привлекательно — где-то там обязательно должны прятаться ошибки. С другой стороны, анализ такого большого проекта будет полезен самому анализатору, так как послужит отличным стресс-тестом.

Инструмент анализа и особенности проверки

Инструмент анализа — статический анализатор кода PVS-Studio. На данный момент анализатор имеет более 100 диагностических правил, каждое из которых описано в документации, содержащей информацию об ошибке, возможных последствиях и способах ее исправления. С момента релиза мы успели проверить большое количество различных проектов, написанных на C#, таких как Roslyn, Xamarin.Forms, Space Engineers, CoreFX, Code Contracts и другие (можно посмотреть полный список по этой ссылке)

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

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

Как всегда, эта статья не содержит всех ошибок, так как это сделало бы ее слишком большой. Я постарался подобрать самые интересные фрагменты, но многие из них так и остались за рамками данной статьи. Не подумайте, что я хочу в чем-то обвинить авторов «Моно». Цифра такая большая из-за размера проекта, что логично. Тем не менее, было бы здорово исправить найденные, и избежать попадания новых ошибок в код. Внедрение статического анализа было бы большим подспорьем в этом. Подробнее можно узнать в соответствующем разделе.

Пару слов о том, почему анализ проекта — вещь нетривиальная

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

К сожалению, наш мир не идеален. Довольно часто проблемы возникают на различных этапах этого процесса. Если есть подробный мануал по сборке проекта, или можно сделать самому — отлично! Тогда можно смело приступать к проверке проекта и написанию статьи. В противном случае у нас будет сильная головная боль. Именно это и произошло с «Моно». Решение net_4_x.sln, объединяющее проекты C#, не компилируется «из коробки» (т.е. сразу после загрузки из репозитория). Один из скриптов сборки работал некорректно (был неправильный путь (возможно из-за того, что со временем менялась иерархия каталогов)), но и исправление пути не помогло.

Конечно, я не хотел сдаваться; поэтому я экспериментировал с компиляцией даже в свободное время. Но особого результата это не принесло. Наконец, потратив на это немало часов, мы решили написать статью «как есть».

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

Конечно, проверять нескомпилированный проект — плохая идея по нескольким причинам:

• Анализ не такой качественный, каким мог бы быть. Это факт. Насколько именно снижается качество, зависит от реализации диагностического правила. Вы можете получить ложное срабатывание или, наоборот, полезное предупреждение не будет выдано;
• следует быть предельно внимательным при просмотре лога, так как есть вероятность (хотя и небольшая) ложных срабатываний, которых можно было бы избежать, если проект был правильно скомпилирован;
• по мере исчезновения нескольких полезных предупреждений есть шанс пропустить некоторые интересные ошибки, которые могли бы попасть в статью и привлечь внимание разработчиков (впрочем, они могут узнать об этих ошибках сами, если проверят проект);
• Вот почему мы должны написать такие разделы, как: «Пару слов о том, почему проверка проекта…»

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

Результаты анализа

В последнее время мы стараемся предоставлять подробную статистику по проверенному проекту: общее количество предупреждений, количество ложных срабатываний и реальных ошибок.

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

Во-вторых, для полностью скомпилированного проекта эта статистика может варьироваться: число может как увеличиваться, так и уменьшаться. Анализатор может получить больше семантической информации в скомпилированном проекте, а значит, сможет провести более глубокий анализ (исчезнут ложные срабатывания, появятся новые предупреждения). Для тех, кому интересно, как семантическая информация влияет на анализ, и на какие принципы опирается его работа, предлагаю прочитать статью Введение в Roslyn. Использование инструментов статического анализа для разработки программ».

Но вам, наверное, не терпится увидеть, что интересного можно найти в кодовом проекте? Что ж, давайте посмотрим на некоторые фрагменты кода.

Одни и те же подвыражения в одном выражении

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

public int ExactInference (TypeSpec u, TypeSpec v)
{
  ....
  var ac_u = (ArrayContainer) u;
  var ac_v = (ArrayContainer) v;
  ....
  var ga_u = u.TypeArguments;
  var ga_v = v.TypeArguments;
  ....
  if (u.TypeArguments.Length != u.TypeArguments.Length) // <=
    return 0;
  ....
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора != одинаковые подвыражения u.TypeArguments.Length. универсальный.cs 3135

Теперь, когда код метода нельзя упростить, не составит труда заметить ошибку в операторе if — параметр v, а не u следует использовать как экземпляр типа TypeSpec. Возможно, ошибка была из-за того, что символы u и v выглядят довольно похоже, и их легко спутать, если человек не акцентирует внимание на этом выражении.

Остальная часть кода была дана, чтобы подчеркнуть, что эти параметры обычно используются вместе,

if (u.TypeArguments.Length != v.TypeArguments.Length) 
    return 0;

Случай, который также представляет интерес:

bool BetterFunction (....)
{
  ....
  int j = 0;
  ....
  if (!candidate_params && 
      !candidate_pd.FixedParameters [j - j].HasDefaultValue) { // <=
    return true;
  }
  ....
  if (!candidate_pd.FixedParameters [j - 1].HasDefaultValue &&   
       best_pd.FixedParameters [j - 1].HasDefaultValue)
    return true;
  if (candidate_pd.FixedParameters [j - 1].HasDefaultValue &&     
      best_pd.HasParams)
    return true;
  ....
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора - одинаковые подвыражения j. ecore.cs 4832

Программист ошибся, написав выражение j — j в одном из выражений для вычисления индекса. Таким образом будет доступ к первому элементу массива. Если это именно то, что здесь нужно, то логичнее было бы использовать целочисленный литерал, равный 0. Другие обращения по индексу к этому массиву: j — 1 доказывают, что это баг . Опять же могу предположить, что ошибка не была замечена из-за некоторого сходства символов j и 1, так что при быстром просмотре кода она может остаться незамеченной.

Примечание коллеги Андрея Карпова. Когда я читал черновик этой статьи, то хотел сделать пометку, что Сергей разместил неверный фрагмент кода. Смотрел код, ошибки не увидел. Только когда начал читать описание, до меня дошло. Подтверждаю, что эту опечатку очень сложно заметить. Наша PVS-Studio великолепна!

Продолжаем ломать голову:

internal void SetRequestLine (string req)
{
  ....
  if ((ic >= 'A' && ic <= 'Z') ||
      (ic > 32 && c < 127 && c != '(' && c != ')' && c != '<'    &&
       c != '<' && c != '>'  && c != '@' && c != ',' && c != ';' &&
       c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' &&
       c != ']' && c != '?'  && c != '=' && c != '{' && c != '}'))
    continue;
  ....
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора && одинаковые подвыражения ‘c != ‘‹’’. HttpListenerRequest.cs 99

Подвыражение c != ‘‹’ записывается в выражении дважды. Это, наверное, просто лишнее сравнение.

protected virtual bool ShouldSerializeLinkHoverColor ()
{
  return grid_style.LinkHoverColor != grid_style.LinkHoverColor;
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора != одинаковые подвыражения grid_style.LinkHoverColor. DataGrid.cs 2225

Мне не нужно было упрощать код, чтобы сделать ошибку более очевидной. В сравнении участвуют два похожих подвыражения — grid_style.LinkHoverColor.

То есть код, вероятно, должен был быть таким:

protected virtual bool ShouldSerializeLinkHoverColor ()
{
  return grid_style.LinkHoverColor != default_style.LinkHoverColor;
}

Почему так? В приведенном выше коде есть ряд методов, в которых различные свойства grid_style сравниваются со свойствами объекта default_style. Но в последнем случае программист ослабил бдительность и допустил ошибку. Хм… эффект последней строки?

Ну, эти ошибки просто классические:

static bool AreEqual (VisualStyleElement value1, 
                      VisualStyleElement value2)
{
  return
    value1.ClassName == value1.ClassName && // <=
    value1.Part == value2.Part &&
    value1.State == value2.State;
}

Предупреждение PVS-Studio:V3001 Слева и справа от оператора == одинаковые подвыражения value1.ClassName. ThemeVisualStyles.cs 2141

Подвыражение value1.ClassName было случайно сопоставлено само с собой. Конечно, во втором случае следует использовать объект value2.

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

static bool AreEqual (VisualStyleElement value1, 
                      VisualStyleElement value2)
{
  return
       value1.ClassName == value1.ClassName
    && value1.Part      == value2.Part
    && value1.State     == value2.State;
}

Код, отформатированный таким образом, намного легче читать и легче заметить, что с одним из столбцов что-то не так. Подробнее см. главу 13 из книги Главный вопрос программирования, рефакторинга и всего остального.

Остальные подозрительные фрагменты, обнаруженные диагностическим правилом V3001, отдаются в файл.

Аналогичные условия в конструкции else if

enum TitleStyle {
  None   = 0,
  Normal = 1,
  Tool   = 2
}
internal TitleStyle title_style;
public Point MenuOrigin {
  get {
    ....
    if (this.title_style == TitleStyle.Normal)  {        // <=
      pt.Y += caption_height;
    } else if (this.title_style == TitleStyle.Normal)  { // <=
      pt.Y += tool_caption_height;
    }
    ....
}

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

Одно и то же выражение this.title_style == TitleStyle.Normal проверяется дважды. Судя по всему, в этом коде есть ошибка. Несмотря на значение выражения, приведенного выше, выражение pt.Y += tool_caption_height никогда не будет выполнено. Могу предположить, что во втором случае программист хотел сравнить поле title_style с константой TitleStyle.Tool.

Те же тела "если-то" и "если-иначе"

public static void DrawText (....)
{
  ....
  if (showNonPrint)
    TextRenderer.DrawTextInternal (g, text, font, 
      new Rectangle (new Point ((int)x, (int)y), max_size), color,   
      TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
  else
    TextRenderer.DrawTextInternal (g, text, font, 
      new Rectangle (new Point ((int)x, (int)y), max_size), color, 
      TextFormatFlags.NoPadding | TextFormatFlags.NoPrefix, false);
  ....
}

Предупреждение PVS-Studio: V3004Утверждение тогда эквивалентно утверждению еще. System.Windows.Forms-net_4_x TextBoxTextRenderer.cs 79

Статический метод DrawTextInternal класса TextRenderer с теми же аргументами будет вызываться независимо от значения переменной showNonPrint. Не исключено, что ошибка была допущена из-за использования копипаста. Вызов метода был скопирован, но аргументы остались забытыми.

Возвращаемое значение метода не используется

public override object ConvertTo(.... object value, 
                                 Type destinationType) 
{
  ....
  if (destinationType == typeof(string)) {
    if (value == null) {
      return String.Empty;
    }
    else {
      value.ToString();
    }
  }
  ....
}

Предупреждение PVS-Studio: V3010Необходимо использовать возвращаемое значение функции ToString. ColumnTypeConverter.cs 91

Это довольно интересная ошибка с явно далеко идущими последствиями. Из кода видно, что есть проверка типа, и если тип string, то есть проверка на null. Затем начинается самая интересная часть; если ссылка value имеет значение null, то возвращается пустая строка, иначе… Скорее всего, ожидалось, что программа вернет строковое представление объекта, но есть нет оператора return. Поэтому возвращаемое значение метода ToString() никак не будет использоваться, а метод ConvertTo будет выполняться в дальнейшем. Таким образом, из-за забытого оператора return изменилась вся логика программы. Я предполагаю, что правильная версия кода должна выглядеть так:

if (value == null) {
  return String.Empty;
}
else {
  return value.ToString();
}

Ошибку, которую мы здесь имеем в виду, вы узнаете позже

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

Вы можете нажать на картинку, чтобы увеличить ее.

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

Предупреждение PVS-Studio:V3012 Оператор ‘?:’, независимо от его условного выражения, всегда возвращает одно и то же значение: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258

Вот он, злосчастный тернарный оператор:

button_pressed_highlight = use_system_colors ?
                             Color.FromArgb (150, 179, 225) : 
                             Color.FromArgb (150, 179, 225);

Независимо от значения переменной use_system_colors объекту button_pressed_highlight будет присвоено одно и то же значение. Если вы считаете, что такие ошибки иногда бывает сложно отследить, предлагаю посмотреть весь файл (ProfessionalColorTable.cs) и понять, что такие ошибки не просто трудно отследить самостоятельно — это просто невозможно.

Подобных фрагментов было довольно много (аж 32), что заставляет усомниться, что это реальный баг, а какое-то преднамеренное действие. Тем не менее код выглядит странно, поэтому я бы посоветовал его перепроверить. Даже если это не ошибка, а ожидаемая логика, было бы гораздо проще использовать простое присваивание, а не писать странные запутанные тернарные операторы. Остальные предупреждения V3012 приведены в файле.

Использование счетчика другого цикла

public override bool Equals (object obj)
{
  if (obj == null)
    return false;
  PermissionSet ps = (obj as PermissionSet);
  if (ps == null)
    return false;
  if (state != ps.state)
    return false;
  if (list.Count != ps.Count)
    return false;
  for (int i=0; i < list.Count; i++) {
    bool found = false;
    for (int j=0; i < ps.list.Count; j++) {
      if (list [i].Equals (ps.list [j])) {
        found = true;
        break;
      }
    }
    if (!found)
      return false;
  }
  return true; 
}

Предупреждение PVS-Studio:V3015 Вероятно, внутри оператора for сравнивается не та переменная. Попробуйте просмотреть i corlib-net_4_x PermissionSet.cs 607.

Условие выхода из вложенного цикла i ‹ ps.list.Count выглядит подозрительно. Переменная j работает здесь как счетчик цикла, но в условии выхода переменная i используется как счетчик внешнего цикла.

Замысел автора кода вполне понятен — проверить, что коллекции содержат одинаковые элементы. Но если элемента из коллекции list нет в ps.list, то выход из вложенного цикла с помощью оператор разрыва. При этом переменная i внутри этого цикла не меняется, т.е. выражение i ‹ ps.list.Count всегда будет иметь истинное значение. В результате цикл будет выполняться до тех пор, пока индекс коллекции не выйдет за границы (из-за постоянного приращения счетчика j).

Проверка на null неправильной ссылки после ее приведения с использованием оператора as

Оказалось, что это типичная ошибка для C#. Мы находим его практически в каждом проекте, о котором пишем статью. Как правило, V3019 выявляет случаи следующего рода:

var derived = base as Derived;
if (base == null) {
  // do something
}
// work with 'derived' object

Проверка base == null сохранится только в том случае, если base действительно имеет nullзначение, и тогда оно не Неважно, сможем ли мы провести кастинг или нет. Судя по всему, здесь имелась в виду проверка производной ссылки. Затем, если base != null, и программе не удалось выполнить приведение,нодалее происходит обработка членов производногообъекта , мы получим исключение типа NullReferenceException.

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

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

public override bool Equals (object o)
{
  UrlMembershipCondition umc = (o as UrlMembershipCondition);
  if (o == null)
    return false;
  ....
  return (String.Compare (u, 0, umc.Url, ....) == 0); // <=
}

Предупреждение PVS-Studio:V3019 Возможно некорректная переменная сравнивается с null после приведения типа с использованием ключевого слова as. Проверьте переменные o, umc. UrlMembershipCondition.cs 111

Эта схема точно такая же, как описанная выше. Если тип объекта o несовместим с типом UrlMembershipCondition, и в то же время объект o не null, то при попытке доступа к свойству umc.Url мы получим исключение NullReferenceException.

Таким образом, чтобы исправить ошибку, нам нужно исправить проверку:

if (umc == null)
  return false;

Взгляните на еще один баг:

static bool QSortArrange (.... ref object v0, int hi, 
                          ref object v1, ....)
{
  IComparable cmp;
  ....
  cmp = v1 as IComparable;
  if (v1 == null || cmp.CompareTo (v0) < 0) {
    ....
  }
  ....
}

Предупреждение PVS-Studio:V3019 Возможно некорректная переменная сравнивается с null после приведения типа с использованием ключевого слова as. Проверьте переменные v1, cmp. Массив.cs 1487

Эта ситуация аналогична описанной выше. Единственное отличие — в случае неудачного поведения исключение NullReferenceException будет сгенерировано сразу — прямо во время проверки выражения.

Примерно такая же ситуация и в нескольких других фрагментах, поэтому приведу еще 12 предупреждений в текстовом файле.

Безусловное исключение

public void ReadEmptyContent(XmlReader r, string name)
{
  ....
  for (r.MoveToContent(); 
         r.NodeType != XmlNodeType.EndElement; 
           r.MoveToContent())
  {
    if (r.NamespaceURI != DbmlNamespace)
      r.Skip();
    throw UnexpectedItemError(r); // <=
  }
  ....
}

Предупреждение PVS-Studio: V3020Безусловный бросок внутри цикла. System.Data.Linq-net_4_x XmlMappingSource.cs 180

Во время первой итерации мы получим исключение UnexpectedItemError. По крайней мере, это выглядит странно. Кстати, Visual Studio подсвечивает объект r в секции, где происходит изменение счетчика циклов, с подсказкой о недостижимом коде. Возможно, автор кода просто не использовал Visual Studio или не заметил предупреждений, поэтому ошибка осталась в коде.

Подозрительные операторы if

Довольно часто мы видим ошибки, когда в методе есть два одинаковых оператора if, а значение объектов, используемых в условных выражениях этих операторов, не изменяется. Если какое-либо из этих условных выражений истинно, то произойдет выход из тела тела метода. Таким образом, второе «если» никогда не будет выполнено. Давайте посмотрим на фрагмент кода, который содержит именно такую ​​ошибку:

public int LastIndexOfAny (char [] anyOf, int startIndex, int count)
{
  ....
  if (this.m_stringLength == 0)
    return -1;
  ....
  if (this.m_stringLength == 0)
    return -1;
  ....
}

Предупреждение PVS-Studio:V3021 есть два оператора if с одинаковыми условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второй оператор if бессмыслен. corlib-net_4_x String.cs 287

Выполнение метода никогда не дойдет до второго оператора if, указанного в этом фрагменте, потому что еслиthis.m_stringLength == 0, то выход будет выполнен при выполнении первого условное заявление. Мы могли бы оправдать код, если бы значение поля m_stringLength изменилось, но это не так.

Последствия бага зависят от причины, по которой он появился:

• Если оба условных выражения верны (с точки зрения логики), а второй код просто лишний — в этом нет ничего страшного, но удалить его стоит, чтобы не вводить в заблуждение других людей;
• Если имелась в виду проверка другого выражения в одном из операторов, или в случае, если имелись в виду другие действия — это более серьезная проблема, указывающая на ошибку в логике программы. Тогда к этому вопросу следует отнестись более серьезно.

Пример более серьезного случая можно увидеть в следующем фрагменте кода (нажмите на картинку для увеличения):

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

private PaperKind GetPaperKind (int width, int height)
{
  ....
  if (width == 1100 && height == 1700)
    return PaperKind.Standard11x17;
  ....
  if (width == 1100 && height == 1700)
    return PaperKind.Tabloid;
  ....
}

Предупреждение PVS-Studio:V3021 есть два оператора if с одинаковыми условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второй оператор if бессмыслен. System.Drawing-net_4_x PrintingServicesUnix.cs 744

Если выражение width == 1100 && height == 1700 истинно, будет выполнено только первое выражение if. Однако значения, возвращаемые этим выражением, если оно истинно, отличаются, поэтому мы не можем просто сказать, что второй оператор if является избыточным. Более того, возможно, в его состоянии должно быть и другое выражение. Очевидно, рабочий процесс программы поврежден.

Наконец, я хотел бы взглянуть на еще один фрагмент кода с этой ошибкой:

private void SerializeCore (SerializationStore store, 
                            object value, bool absolute)
{
  if (value == null)
    throw new ArgumentNullException ("value");
  if (store == null)
    throw new ArgumentNullException ("store");
  CodeDomSerializationStore codeDomStore = 
    store as CodeDomSerializationStore;
  if (store == null)
    throw new InvalidOperationException ("store type unsupported");
  codeDomStore.AddObject (value, absolute);
}

Предупреждение PVS-Studio:V3021 есть два оператора if с одинаковыми условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второй оператор if бессмыслен. System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562

Это предупреждение имеет много общего с предупреждением V3019, так как у нас есть паттерн проверки на null после приведения с использованием оператора as неправильной ссылки. Неважно, какое предупреждение выдается — ошибка очевидна.

Были и другие подобные предупреждения:

• V3021 Имеются два оператора if с идентичными условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второе утверждение «если» бессмысленно. Mono.Data.Sqlite-net_4_x SQLiteDataReader.cs 270
• V3021 Имеются два оператора if с идентичными условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второе утверждение «если» бессмысленно. System.Web-net_4_x HttpUtility.cs 220
• V3021 Имеются два оператора if с идентичными условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второй оператор if бессмысленен. System.Design-plaindesign-net_4_x CodeDomComponentSerializationService.cs 562
• V3021 Имеются два оператора if с идентичными условными выражениями. Первый оператор if содержит возврат метода. Это означает, что второе утверждение «если» бессмысленно. Mono.Security.Providers.DotNet-net_4_x DotNetTlsProvider.cs 77

Подозрительные строки формата

Правило диагностики V3025 обнаруживает строки неправильного формата. Это также тип ошибки, который мы находим во многих проектах, которые мы проверяем. Обычно бывают ситуации двух видов:

• строка формата предполагает большее количество параметров, чем задано;
• строка формата ожидает меньше параметров, чем задано.

В первом случае будет выброшено исключение типа FormatException, во втором случае неиспользуемые аргументы будут просто проигнорированы. В любом случае такие фрагменты стоит пересматривать и исправлять.

Конечно, я бы не стал говорить об этом диагностическом правиле, если бы не было найдено подобных ошибок.

static IMessageSink GetClientChannelSinkChain(string url, ....)
{
  ....
  if (url != null) 
  {
    string msg = String.Format (
      "Cannot create channel sink to connect to URL {0}. 
       An appropriate channel has probably not been registered.", 
      url); 
    throw new RemotingException (msg);
  }
  else 
  {
    string msg = String.Format (
      "Cannot create channel sink to connect to the remote object. 
       An appropriate channel has probably not been registered.", 
      url); 
    throw new RemotingException (msg);
  }
  ....
}

Предупреждение PVS-Studio:V3025 Неверный формат. При вызове функции Формат ожидается другое количество элементов формата. Аргументы не используются: url. corlib-net_4_x RemotingServices.cs 700

Хочу обратить ваше внимание на вторую строку формата. Это строковый литерал, который не обеспечивает замену аргументов (в отличие от строки формата выше). Однако метод Format принимает объект url в качестве второго аргумента. Из вышесказанного следует, что объект url при формировании новой строки будет просто проигнорирован, и информация о нем не попадет в текст исключения.

В C# 6.0 были добавлены интерполированные строки, что в ряде случаев поможет избежать проблем, связанных с использованием форматных строк, в том числе некорректного количества аргументов.

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

public override string ToString ()
{
  return string.Format ("ListViewSubItem {{0}}", text);
}

Предупреждение PVS-Studio:V3025 Неверный формат. При вызове функции Формат ожидается другое количество элементов формата. Неиспользуемые аргументы: текст. System.Windows.Forms-net_4_x ListViewItem.cs 1287

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

"ListViewSubItem {{0}}"
To fix this bug, we could use interpolated strings to rewrite the method:
public override string ToString ()
{
  return $"ListViewSubItem {{{text}}}",;
}

Или, оставаясь верным методу String.Format, мы должны добавить фигурные скобки с каждой стороны. Тогда строка формата будет выглядеть следующим образом:

"ListViewSubItem {{{0}}}"

Вот последний фрагмент со строкой формата. Как всегда самое интересное подается на десерт:

void ReadEntropy ()
{
  if (reader.IsEmptyElement)
    throw new XmlException (
      String.Format ("WS-Trust Entropy element is empty.{2}", 
                      LineInfo ()));
  ....
}

Предупреждение PVS-Studio:V3025 Неверный формат. При вызове функции Формат ожидается другое количество элементов формата. Элементы формата не используются: {2}. Аргументы не используются: 1-й. System.ServiceModel-net_4_x WSTrustMessageConverters.cs 147

Я понятия не имею, как элемент форматирования с индексом «2» попал в строку форматирования, но это приводит к довольно забавной ошибке. Это должно было вызвать исключение с некоторым текстом, созданным строкой формата. И будет выброшено исключение. Исключение типа FormatException, поскольку текущая строка формата требует 3 аргумента (поскольку нужен именно третий), а представлен только один.

Если программист перепутал только номер запрошенного аргумента (при рефакторинге, например), то этот баг будет легко исправить:

"WS-Trust Entropy element is empty.{0}"

В этом файле приведены другие подозрительные фрагменты, обнаруженные правилом V3025.

Доступ по нулевой ссылке

private bool IsContractMethod (string methodName, 
                               Method m, 
                               out TypeNode genericArgument)
{
  ....
  return m.Name != null && m.Name == methodName &&
    (m.DeclaringType.Equals (this.ContractClass)     // <=
     || (m.Parameters    != null && 
         m.Parameters.Count == 3 && 
         m.DeclaringType != null &&                  // <=
         m.DeclaringType.Name != ContractClassName));
}

Предупреждение PVS-Studio: V3027Переменная ‘m.DeclaringType’ использовалась в логическом выражении до того, как она была проверена на нуль в том же логическом выражении. Mono.CodeContracts-net_4_x ContractNodes.cs 211

Перед доступом к свойству Name свойства DeclaringType программист решил перестраховаться и проверить свойство DeclaringType на соответствие null, чтобы он случайно не обратился к нулевой ссылке. Желание сделать это понятно и вполне правомерно. Единственное, это не будет иметь никакого эффекта, потому что далее в коде мы видим, что метод экземпляра Equals для свойства DeclaringType, а это значит, что если DeclaringType == null, мы получим исключение типа NullReferenceException. Чтобы решить эту проблему, мы можем переместить проверку на значение NULL выше в коде или использовать оператор условия NULL ('?.'), который доступен в C# 6.0.

Node leftSentinel;
....
IEnumerator<T> GetInternalEnumerator ()
{
  Node curr = leftSentinel;
  while ((curr = curr.Nexts [0]) != rightSentinel && curr != null) {
    ....
  }
}

!aa || (aa && bb)

!aa || bb

!aa || (cc && bb)

public void Emit(OpCode opc)
{
  Debug.Assert(opc != OpCodes.Ret || (
               opc == OpCodes.Ret && stackHeight <= 1));
  ....
}

Debug.Assert(opc != OpCodes.Ret || stackHeight <= 1));

public bool Validate (bool checkAutoValidate)
{
  if ((checkAutoValidate && (AutoValidate != AutoValidate.Disable)) ||
      !checkAutoValidate)
    return Validate ();
  return true;
}

!checkAutoValidate || (AutoValidate != AutoValidate.Disable)

public bool this [ string header ] {
  set {
      ....
      if (value)
        if (!fields.Contains (header))
          fields.Add (header, true);
      else
        fields.Remove (header);
  }
}

if (value)
  if (!fields.Contains (header))
    fields.Add (header, true);
  else
    fields.Remove (header);

public void yyerror (string message, string[] expected) {
  ....
  for (int n = 0; n < expected.Length; ++ n)
    ErrorOutput.Write (" "+expected[n]);
    ErrorOutput.WriteLine ();
  ....
}

private string BuildParameters ()
{
  ....
  if (result.Length > 0)
    result.Append (", ");
    if (p.Direction == TdsParameterDirection.InputOutput) // <=
      result.Append (String.Format("{0}={0} output",     
                                   p.ParameterName));
    else
  result.Append (FormatParameter (p));
  ....
}

public void Restore ()
{
  while (saved_count < objects.Count)
    objects.Remove (objects.Last ().Key);
    referenced.Remove (objects.Last ().Key);
  saved_count = 0;
  referenced.RemoveRange (saved_referenced_count, 
                          referenced.Count - saved_referenced_count);
  saved_referenced_count = 0;
}

String str;
String str2 = str as String;

String str;
if (str is String)

public string GenerateHttpGetMessage (Port port, 
                                      OperationBinding obin, 
                                      Operation oper, 
                                      OperationMessage msg)
{
  ....
  MimeXmlBinding mxb = 
    (MimeXmlBinding) obin.Output
                         .Extensions
                         .Find (typeof(MimeXmlBinding)) 
      as MimeXmlBinding;
  if (mxb == null) return req;
  ....
}

internal static int SetErrorInfo (int dwReserved, 
                                  IErrorInfo errorInfo)
{
  int retVal = 0;
  errorInfo = null;
  ....
  retVal = _SetErrorInfo (dwReserved, errorInfo);
  ....
}

class ResXResourceWriter : IResourceWriter, IDisposable
{
  ....
  public static readonly string ResourceSchema = schema;
  ....
  static string schema = ....;
  ....
}

static class Profiler
{
  [ThreadStatic]
  private static Stopwatch timer = new Stopwatch();
  ....
}

public void EmitLong (long l)
{
  if (l >= int.MinValue && l <= int.MaxValue) {
    EmitIntConstant (unchecked ((int) l));
    ig.Emit (OpCodes.Conv_I8);
  } else if (l >= 0 && l <= uint.MaxValue) {
    EmitIntConstant (unchecked ((int) l));
    ig.Emit (OpCodes.Conv_U8);
  } else {
    ig.Emit (OpCodes.Ldc_I8, l);
  }
}

public void ConvertGlobalAttributes (
  TypeContainer member, 
  NamespaceContainer currentNamespace, 
  bool isGlobal)
{
  var member_explicit_targets = member.ValidAttributeTargets;
  for (int i = 0; i < Attrs.Count; ++i) {
    var attr = Attrs[0];
    if (attr.ExplicitTarget == null)
      continue;
    ....
  }
}

var attr = Attrs[i];

public RegistryKey Ensure (....)
{
  lock (typeof (KeyHandler)){
    ....
  }
}

void ConfigureHttpChannel (HttpContext context)
{
  lock (GetType())
  {
    ....
  }
}

const string Profiles_SettingsPropertyCollection = 
               "Profiles.SettingsPropertyCollection";
....
static void InitProperties ()
{
  ....
  lock (Profiles_SettingsPropertyCollection) {
  if (_properties == null)
    _properties = properties;
  }
}

public bool Equals (CounterSample other)
{
  return
    rawValue == other.rawValue &&
    baseValue == other.counterFrequency &&
    counterFrequency == other.counterFrequency &&
    systemFrequency == other.systemFrequency &&
    timeStamp == other.timeStamp &&
    timeStamp100nSec == other.timeStamp100nSec &&
    counterTimeStamp == other.counterTimeStamp &&
    counterType == other.counterType;
}

public bool Equals (CounterSample other)
{
  return
    rawValue         == other.rawValue         &&
    baseValue        == other.counterFrequency && // <=
    counterFrequency == other.counterFrequency && // <=
    systemFrequency  == other.systemFrequency  &&
    timeStamp        == other.timeStamp        &&
    timeStamp100nSec == other.timeStamp100nSec &&
    counterTimeStamp == other.counterTimeStamp &&
    counterType      == other.counterType;
}