Сергей Хренов

19 Дек 2019

Теги:

#CSharp

Топ 10 ошибок в проектах C# за 2019 год

19 Дек 2019

Автор: Сергей Хренов

Десятое место: "Запутать всех"
Девятое место: "Бесконечное рядом"
Восьмое место: "Попробуй, найди"
Седьмое место: "Просто недоглядели"
Шестое место: "Код документирован", – говорили они
Пятое место: "Индексируй меня, немедленно!"
Четвёртое место: "Всего-то доллара и не хватило"
Третье место: "Выхода нет"
Второе место: "По порядку становись!"
Первое место: "Доверяй, но проверяй"
Заключение

Приветствуем всех любителей багов. Уже скоро наступит Новый год, так что самое время подвести итоги года уходящего. По традиции - рейтинг самых интересных ошибок, которые были обнаружены командой PVS-Studio в открытых проектах C# за 2019 год. Готовы? Тогда приступим.

0698_Top_10_CSharp_Bugs_2019_ru/image1.png

Десятое место: "Запутать всех"

V3066 Possible incorrect order of arguments passed to 'AdjustCellBorderStyle' method: 'isFirstDisplayedRow' and 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

protected override void OnMouseMove(DataGridViewCellMouseEventArgs e)
{
  ....
  dgvabsEffective = AdjustCellBorderStyle(
    DataGridView.AdvancedCellBorderStyle,
    dgvabsPlaceholder,
    singleVerticalBorderAdded,
    singleHorizontalBorderAdded,
    isFirstDisplayedRow,      // <=
    isFirstDisplayedColumn);  // <=
  ....
}

Ошибка из статьи "WinForms: ошибки, Холмс". Анализатор указывает, что при передаче аргументов в метод два последних аргумента были перепутаны местами. Посмотрим на объявление AdjustCellBorderStyle:

public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle(
  DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput,
  DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder,
  bool singleVerticalBorderAdded,
  bool singleHorizontalBorderAdded,
  bool isFirstDisplayedColumn,
  bool isFirstDisplayedRow)
{
  ....
}

Похоже, что анализатор прав. Часто некоторые аргументы умышленно передают в обратном порядке, например, чтобы обменять местами какие-то переменные. Но не похоже, что это именно тот случай. Во-первых, перепутаны переменные типа bool. Во-вторых, названия методов также обычные: никаких "Swap" или "Reverse". К тому же, так ошибиться не так уж и сложно: люди часто по-разному воспринимают порядок следования пары "строка/столбец".

Девятое место: "Бесконечное рядом"

V3110 Possible infinite recursion inside 'TryValidateModel' method. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix)
{
  return TryValidateModel(model, Prefix(prefix));
}

Ошибка из статьи "Ищем и анализируем ошибки в коде Orchard CMS". Допущена ошибка, приводящая к возникновению бесконечной рекурсии. Для понимания того, как могла быть допущена эта ошибка, необходимо взглянуть на перегрузку метода TryValidateModel:

public bool TryValidateModel(object model)
{
  return _updateModel.TryValidateModel(model);
}

Вероятно, что и в первом случае должен использоваться вызов вида:

public bool TryValidateModel(object model, string prefix)
{
  return _updateModel.TryValidateModel(model, Prefix(prefix));
}

Код компилировался, так как _updateModel имеет тип IUpdateModel, а текущий класс также реализует интерфейс IUpdateModel.

Восьмое место: "Попробуй, найди"

V3091 Empirical analysis. It is possible that a typo is present inside the string literal: "Management Group Id". The 'Id' word is suspicious. Constants.cs 36

public class HelpMessages
{
  public const string SubscriptionId = "Subscription Id of the subscription
                                        associated with the management";
  public const string GroupId = "Management Group Id";       // <=
  public const string Recurse = "Recursively list the children of the
                                 management group";
  public const string ParentId = "Parent Id of the management group";
  public const string GroupName = "Management Group Id";     // <=
  public const string DisplayName = "Display Name of the management group";
  public const string Expand = "Expand the output to list the children of the
                                management group";
  public const string Force = "Force the action and skip confirmations";
  public const string InputObject = "Input Object from the Get call";
  public const string ParentObject = "Parent Object";
}

Ошибка из статьи "Azure PowerShell: "в основном безвреден". Анализатор заподозрил, что константу GroupName инициализировали ошибочной строкой. Вероятно, там должно быть что-то наподобие "Management Group Name". Трудно судить о критичности этой ошибки, но она однозначно редкая и трудно обнаруживаемая.

Седьмое место: "Просто недоглядели"

V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. GridModel.Selection.cs 107

internal partial class GridModel
{
  private void BuildCellSelectionRegions(....)
  {
    ....
    this.MergeCellSelectionRegions(selectedItemsInView
        .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
        .OrderBy(c => c.RowItemInfo.LayoutInfo.Line));
    }
}

Ошибка из статьи "Проверка Telerik UI for UWP для знакомства с PVS-Studio". Вследствие повторного использования OrderBy для уже отсортированной коллекции, результат предыдущей сортировки будет потерян. Необходимо использовать ThenBy:

this.MergeCellSelectionRegions(selectedItemsInView
    .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
    .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));

Такие ошибки допускают по невнимательности или незнанию. Я думаю, что виноват copy-paste.

Шестое место: "Код документирован", – говорили они

V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529

public bool Remove(out int testPosition,
  out MaskedTextResultHint resultHint)
{
  ....
  if (lastAssignedPos == INVALID_INDEX)
  {
    ....
    return true; // nothing to remove.
  }
  ....
  return true;
}

Ошибка из статьи "Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio". Метод всегда вернёт true. Да, это ошибка, но любопытно другое. К методу прилагается развернутый комментарий:

Removes the last character from the formatted string. (Remove last character in virtual string). On exit the out param contains the position where the operation was actually performed. This position is relative to the test string. The MaskedTextResultHint out param gives more information about the operation result. Returns true on success, false otherwise.

Фокус на последнее предложение. Но кто вообще читает эти комментарии? А если серьёзно, то такая ошибка легко вкрадывается, например, при рефакторинге или отладке. Захотели проверить вариант, когда результат метода будет всегда true, ну и позабыли вернуть всё как было обратно.

Пятое место: "Индексируй меня, немедленно!"

V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738

public bool MatchesXmlType(IList<XPathItem> seq, ....)
{
  ....
  for (int i = 0; i < seq.Count; i++)
  {
    if (!CreateXmlType(seq[0]).IsSubtypeOf(....))
      return false;
  }

  return true;
}

Ошибка из статьи "Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio". При обходе коллекции seq в цикле for ошибочно используют доступ только к её первому элементу на всех итерациях (индекс 0 вместо i).

Четвёртое место: "Всего-то доллара и не хватило"

V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42

internal static void CacheCredential(SafeFreeCredentials newHandle)
{
  try
  {
    ....
  }
  catch (Exception e)
  {
    if (!ExceptionCheck.IsFatal(e))
    {
      NetEventSource.Fail(null, "Attempted to throw: {e}");
    }
  }
}

Ошибка из статьи "Проверка исходного кода библиотек .NET Core статическим анализатором PVS-Studio". Строка "Attempted to throw: {e}", по всей видимости, должна быть интерполированной. Из-за пропущенного символа $ строковое представление исключения e не будет подставлено в строку. В результате строка будет использована "как есть".

Третье место: "Выхода нет"

V3008 [CWE-563] The 'this.linker.s3.region' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. AWSSDK.DynamoDBv2.Net45 S3Link.cs 116

public string Region 
{ 
  get 
  {
    ....
  } 
  set 
  {
    if (String.IsNullOrEmpty(value))
    {
      this.linker.s3.region = "us-east-1";
    }
    this.linker.s3.region = value; 
  } 
}

Ошибка из статьи "Ищем ошибки в исходном коде Amazon Web Services SDK для .NET". В теле блока if был пропущен return. В результате переменная this.linker.s3.region всегда получит значение value, включая пустую строку и null.

Второе место: "По порядку становись!"

V3070 Uninitialized variable 'LANG_USER_DEFAULT' is used when initializing the 'LOCALE_USER_DEFAULT' variable. NativeMethods.cs 890

internal static class NativeMethods
{
  ....
  public static readonly int LOCALE_USER_DEFAULT =
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT);
  ....
}

Ошибка из статьи "WinForms: ошибки, Холмс". Перепутан порядок инициализации полей класса. Для вычисления значения поля LOCALE_USER_DEFAULT используют поле LANG_USER_DEFAULT, которое в данный момент ещё не инициализировано и имеет значение 0. Переменная LANG_USER_DEFAULT далее в коде нигде не используется. Для понимания, к чему приводит такая ошибка, была написана тестовая программа, содержащая методы из кода WinForms. Для простоты, вместо некоторых используемых констант были подставлены их фактические значения:

internal static class NativeMethods
{
  public static readonly int LOCALE_USER_DEFAULT = 
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(0x00, 0x01);
  
  public static int MAKELANGID(int primary, int sub)
  {
    return ((((ushort)(sub)) << 10) | (ushort)(primary));
  }
  public static int MAKELCID(int lgid)
  {
    return MAKELCID(lgid, 0x0);
  }
  public static int MAKELCID(int lgid, int sort)
  {
    return ((0xFFFF & lgid) | (((0x000f) & sort) << 16));
  }
}
class Program
{
  static void Main()
  {
    System.Console.WriteLine(NativeMethods.LOCALE_USER_DEFAULT);
  }
}

В результате запуска на консоль будет выведено: 0. Теперь исправим ошибку, поменяв местами объявление полей LOCALE_USER_DEFAULT и LANG_USER_DEFAULT. Результат выполнения программы в таком виде: 1024.

Первое место: "Доверяй, но проверяй"

С первым местом всегда непросто. Здесь должно быть что-то экстраординарное и очень интересное. Изначально для этой статьи я отобрал более двадцати интересных ошибок, но ничего достойного первого места среди них не было. И тут я вспомнил о статье моего коллеги Сергея Васильева, посвященной одной единственной ошибке. Прелесть этой ошибки в том, что она напрямую влияла на работу нашего анализатора. Как? Это понятно уже из названия статьи "История о том, как PVS-Studio нашёл ошибку в библиотеке, используемой в... PVS-Studio". Опять же, лень никто не отменял. :) А я здесь совершенно с чистой совестью заленюсь и не стану приводить описание ошибки, а предложу проследовать по ссылке на статью и узнать подробности. Гарантирую – это того стоит. Да и статья короткая.

Заключение

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

Дочитали? Поздравляю с переходом на следующий уровень! В следующих статьях нашего блога - лучшие баги 2019 года в проектах на Java и C++.