Вадим Кулешов

25 Мар 2022

Теги:

#CSharp

Ищем ошибки в C# коде GUI-фреймворка Eto.Forms

25 Мар 2022

Автор: Вадим Кулешов

Введение
Предупреждения анализатора
Заключение

Популярность GUI-фреймворков для .NET постоянно растёт – появляются новые, развиваются старые. Мы решили не обходить эту тему стороной и рассмотреть подозрительные места, найденные в C# коде одного из таких проектов – Eto.Forms.

Введение

Eto.Forms (или просто Eto) – это один из GUI-фреймворков, использующих C# и XAML для разработки. Сам он также написан на C#. Важной чертой Eto является кроссплатформенность: он позволяет создавать приложения с графическим интерфейсом для основных десктопных ОС: Windows, Linux и macOS. Поддержка мобильных платформ Android и iOS находится в разработке.

Кстати, PVS-Studio – тот самый анализатор, который позволил нам собрать ошибки для данного обзора, работает на всех этих ОС. Кроме, конечно, мобильных платформ :)

В этой статье был использован анализатор версии 7.17 и исходники Eto.Forms от 10.02.2022.

Ранее мы уже проверяли несколько фреймворков для разработки GUI-приложений, написанных на C#:

Предупреждения анализатора

Issue 1

Для лучшего понимания проблемы я решил привести код метода полностью:

/// <summary>
/// ....
/// </summary>
/// ....
/// <returns>True if successful, 
/// or false if the value could not be parsed
// </returns>
public static bool TryParse(string value, out DashStyle style)
{
  if (string.IsNullOrEmpty(value))
  {
    style = DashStyles.Solid;
    return true;
  }

  switch (value.ToUpperInvariant())
  {
    case "SOLID":
        style = DashStyles.Solid;
        return true;
      case "DASH":
        style = DashStyles.Dash;
        return true;
      case "DOT":
        style = DashStyles.Dot;
        return true;
      case "DASHDOT":
        style = DashStyles.DashDot;
        return true;
      case "DASHDOTDOT":
        style = DashStyles.DashDotDot;
        return true;
  }
  var values = value.Split(',');
  if (values.Length == 0)
  {
    style = DashStyles.Solid;
    return true;
  }
  float offset;
  if (!float.TryParse(values[0], out offset))
    throw new ArgumentOutOfRangeException("value", value);
  float[] dashes = null;
  if (values.Length > 1)
  {
    dashes = new float [values.Length - 1];
    for (int i = 0; i < dashes.Length; i++)
    {
      float dashValue;
      if (!float.TryParse(values[i + 1], out dashValue))
        throw new ArgumentOutOfRangeException("value", value);
      dashes[i] = dashValue;
    }
  }

  style = new DashStyle(offset, dashes);
  return true;
}

Предупреждение PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. Eto DashStyle.cs 56

Анализатор предупредил, что метод возвращает только true во всех своих многочисленных ветках с возвратами.

Давайте разберёмся, что не так с этим кодом. Начну с того, что обычно методы с префиксом "TryParse" в названии следуют одноимённому паттерну и имеют следующие особенности:

возврат bool;
наличие out-параметра;
отсутствие выброса исключений.

Предполагается, что:

когда операция успешна, возвращается true, а out-аргумент получает требуемое значение;
иначе возвращается false, а out-аргумент получает default-значение.

Затем программист должен проверить возвращённый bool и построить логику исходя из результатов проверки.

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

В этом методе возврат происходит только при изначально корректных входных данных – иначе выбрасывается исключение. Эта логика противоположна описанной в паттерне Try-Parse – метод не соответствует ему. Это делает префикс "TryParse" опасно запутывающим для программистов, знающих этот паттерн.

К слову, у метода есть XML-комментарий: <returns>True if successful, or false if the value could not be parsed</returns>. К сожалению, он несёт ложную информацию.

Issue 2

public static IEnumerable<IPropertyDescriptor> GetProperties(Type type)
{
  if (s_GetPropertiesMethod != null)
    ((ICollection)s_GetPropertiesMethod.Invoke(null, new object[] { type }))
                                       .OfType<object>()
                                       .Select(r => Get(r));  // <=
  return type.GetRuntimeProperties().Select(r => Get(r));
}

Предупреждение PVS-Studio: V3010 The return value of function 'Select' is required to be utilized. Eto PropertyDescriptorHelpers.cs 209

Анализатор обнаружил, что возвращаемое значение метода Select не используется.

Select – это один из LINQ-методов расширения типа IEnumerable<T>. Аргументом Select является проецирующая функция, а результатом – перечисление элементов, возвращённых этой функцией. Всегда есть вероятность того, что метод Get имеет побочные эффекты, но из-за ленивости LINQ ни для какого элемента коллекции Get не будет выполнен. Ошибка неиспользованного результата очевидна уже здесь.

Если посмотреть код внимательнее, то окажется, что метод Get, используемый в лямбде, возвращает IPropertyDescriptor:

public static IPropertyDescriptor Get(object obj)
{
  if (obj is PropertyInfo propertyInfo)
    return new PropertyInfoDescriptor(propertyInfo);
  else
    return PropertyDescriptorDescriptor.Get(obj);
}

Значит, типом возвращаемой методом Select коллекции будет IEnumerable<IPropertyDescriptor>. Это точно такой же тип, как и у возвращаемого значения метода GetProperties, для кода которого было сгенерировано предупреждение. Скорее всего, здесь был потерян return:

public static IEnumerable<IPropertyDescriptor> GetProperties(Type type)
{
  if (s_GetPropertiesMethod != null)
    return 
     ((ICollection)s_GetPropertiesMethod.Invoke(null, new object[] { type }))
                                        .OfType<object>()
                                        .Select(r => Get(r));
  return type.GetRuntimeProperties().Select(r => Get(r));
}

Issue 3

public override string Text
{
  get { return base.Text; }
  set
  {
    var oldText = Text;
    var newText = value ?? string.Empty;               // <=
    if (newText != oldText)
    {
      var args = new TextChangingEventArgs(oldText, newText, false);
      Callback.OnTextChanging(Widget, args);
      if (args.Cancel)
        return;
      base.Text = value;
      if (AutoSelectMode == AutoSelectMode.Never)
        Selection = new Range<int>(value.Length,       // <=
                                   value.Length - 1);  // <=
    }
  }

Предупреждение PVS-Studio: V3125 The 'value' object was used after it was verified against null. Check lines: 329, 320. Eto.WinForms(net462) TextBoxHandler.cs 329

Анализатор указывает, что ссылка была проверена на null, но в дальнейшем использована без проверки.

Что же произойдёт, если value будет равен null?

Значение value проверяется на null с помощью null-coalescing оператора. Строка newText получит значение string.Empty. Если oldText ранее не содержал пустую строку, то управление перейдёт в then-ветку. Внутри ветки производится присваивание null свойству:

base.Text = value;

Это уже выглядит странно. Ранее значение value было проверено на null, и была введена переменная newText, которая точно не является null. Возможно, здесь и далее предполагалось использование newText.

Но это не всё. Посмотрим код дальше. Несколькими строками ниже есть разыменование value:

Selection = new Range<int>(value.Length,  // <=
                           value.Length - 1);

И тут value всё ещё может быть null. Если управление дойдёт до этого кода и value будет null, произойдёт выброс исключения NullReferenceException.

Issue 4

protected virtual void OnChanging(BindingChangingEventArgs e)
{
  if (Changing != null)
    Changing(this, e);
}

Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'Changing', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. Eto Binding.cs 80

Анализатор выдал сообщение о том, что вызов события небезопасен, так как нет гарантии наличия подписчиков.

Несмотря на наличие проверки if (Changing != null), количество подписчиков может измениться между проверкой и вызовом. Ошибка проявится, если событие будет использовано в многопоточном коде. Само событие объявлено так:

public event EventHandler<BindingChangingEventArgs> Changing;

Класс, содержащий событие, также публичный:

public abstract partial class Binding

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

Следует использовать метод Invoke и Elvis operator для вызова события:

protected virtual void OnChanging(BindingChangingEventArgs e)
{
  Changing?.Invoke(this, e);
}

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

protected virtual void OnChanging(BindingChangingEventArgs e)
{
  EventHandler<BindingChangingEventArgs> safeChanging = Changing;
  if (safeChanging != null)
    safeChanging(this, e);
}

Issue 5

void UpdateColumnSizing(....)
{
  ....
  switch (FixedPanel)
  {
    case SplitterFixedPanel.Panel1:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));  // <=
      break;
    case SplitterFixedPanel.Panel2:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));  // <=
      break;
    case SplitterFixedPanel.None:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
  }
  ....
}

Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. Eto.Wpf(net462) SplitterHandler.cs 357

Анализатор обнаружил фрагмент конструкции switch, где разные case-ветви содержат одинаковый код.

switch покрывает 3 элемента перечисления SplitterFixedPanel, два из которых имеют название Panel1 и Panel2. В обеих ветках вызывается метод SetLength, который имеет такую сигнатуру:

void SetLength(int panel, sw.GridLength value)

Значение аргумента panel используется в качестве индекса внутри метода SetLength:

Control.ColumnDefinitions[panel] = ....

Ещё одна ветвь покрывает элемент None. Предположу, он объединяет код для обеих панелей. Вероятно, использование магических чисел "0" и "2" вполне корректно, так как здесь производится работа со стандартным контролом "SplitContainer". Число "1" соответствует не упомянутому тут разделителю. Возможно, код должен иметь такой вид:

void UpdateColumnSizing(....)
{
  ....
  switch (FixedPanel)
  {
    case SplitterFixedPanel.Panel1:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
    case SplitterFixedPanel.Panel2:
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
    case SplitterFixedPanel.None:
      SetLength(0, new sw.GridLength(1, sw.GridUnitType.Star));
      SetLength(2, new sw.GridLength(1, sw.GridUnitType.Star));
      break;
  }
  ....
}

Issue 6

public Font SelectionFont
{
  get
  {
    ....
    Pango.FontDescription fontDesc = null;
    ....
    foreach (var face in family.Faces)
    {
      var faceDesc = face.Describe();
      if (   faceDesc.Weight == weight 
          && faceDesc.Style == style 
          && faceDesc.Stretch == stretch)
      {
        fontDesc = faceDesc;
        break;
      }
    }
    if (fontDesc == null)
      fontDesc = family.Faces[0]?.Describe();   // <=
    var fontSizeTag = GetTag(FontSizePrefix);
    fontDesc.Size =   fontSizeTag != null       // <=
                    ? fontSizeTag.Size
                    : (int)(Font.Size * Pango.Scale.PangoScale);
    ....
  }
}

Предупреждение PVS-Studio: V3105 The 'fontDesc' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Eto.Gtk3 RichTextAreaHandler.cs 328

Анализатор предупреждает об использовании без проверки переменной, которая может быть null, так как при присвоении ей значения применяется null-conditional оператор.

Переменной fontDesc при объявлении присваивается null. Если новое значение не было присвоено в цикле foreach, то существует ещё одна ветка, где производится присваивание значения для fontDesc. Но код присвоения использует null-conditional (Elvis) оператор:

fontDesc = family.Faces[0]?.Describe();

Это означает, что если в первом элементе массива будет null, то fontDesc будет присвоен null. А дальше производится разыменование:

fontDesc.Size = ....

Если fontDesc будет null, то попытка присвоить значение свойству Size приведёт к выбросу исключения NullReferenceException.

Впрочем, всё выглядит так, будто null-conditional оператор остался от рефакторинга или был добавлен случайно. Если в family.Faces[0] будет находиться null, то выброс NullReferenceException произойдёт ещё в цикле foreach. Там происходит разыменование:

foreach (var face in family.Faces)
{
  var faceDesc = face.Describe(); // <=
  if (   faceDesc.Weight == weight 
      && faceDesc.Style == style 
      && faceDesc.Stretch == stretch)
  {
    fontDesc = faceDesc;
    break;
  }
}

Issue 7

public override NSObject GetObjectValue(object dataItem)
{
  float? progress = Widget.Binding.GetValue(dataItem);  // <=
  if (Widget.Binding != null && progress.HasValue)      // <=
  {
    progress = progress < 0f ? 0f : progress > 1f ? 1f : progress;
    return new NSNumber((float)progress);
  }
  return new NSNumber(float.NaN);
}

Предупреждение PVS-Studio: V3095 The 'Widget.Binding' object was used before it was verified against null. Check lines: 42, 43. Eto.Mac64 ProgressCellHandler.cs 42

Анализатор указал, что разыменование ссылки производится раньше её проверки на null.

Если Widget.Binding будет null, то при вызове метода GetValue будет выброшено исключение NullReferenceException. Находящаяся ниже проверка Widget.Binding != null является бесполезной. Следует упростить код, используя уже упомянутый в этой статье Elvis оператор, и изменить условие. Более корректный код может быть таким:

public override NSObject GetObjectValue(object dataItem)
{
  float? progress = Widget.Binding?.GetValue(dataItem);
  if (progress.HasValue)
  {
    progress =   progress < 0f 
               ? 0f 
               : (progress > 1f 
                  ? 1f 
                  : progress);
    return new NSNumber((float)progress);
  }
  return new NSNumber(float.NaN);
}

Issue 8

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

public bool Enabled
{
  get { return Control != null ? enabled : Control.Sensitive; }
  set {
    if (Control != null)
      Control.Sensitive = value;
    else
      enabled = value;
  }
}

Где же она?

Ошибка находится здесь:

get { return Control != null ? enabled : Control.Sensitive; }

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'Control'. Eto.Gtk3 RadioMenuItemHandler.cs 143

Анализатор сообщает о возможном разыменовании нулевой ссылки.

Проверка бессмысленна и не защищает от NullReferenceException. В тернарном операторе если условие истинно, то вычисляется только первое выражение. Если условие ложно, то вычисляется второе выражение. Когда Control будет null, тогда условие будет ложным и произойдёт разыменование нулевой ссылки – очевидный NullReferenceException.

Issue 9

public NSShadow TextHighlightShadow
{
  get
  {
    if (textHighlightShadow == null)
    {
      textHighlightShadow = new NSShadow();
      textHighlightShadow.ShadowColor = NSColor.FromDeviceWhite(0F, 0.5F);
      textHighlightShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textHighlightShadow.ShadowBlurRadius = 2F;
    }
    return textHighlightShadow;
  }
  set { textShadow = value; }
}

Предупреждение PVS-Studio: V3140 Property accessors use different backing fields. Eto.Mac64 MacImageAndTextCell.cs 162

Анализатор обнаружил, что в сеттере и геттере свойства используются разные поля. В сеттере используется textShadow, а в геттере – textHighlightShadow. Взглянув на название свойства – TextHighlightShadow, можно понять, что правильным полем является textHighlightShadow. Вот его объявление:

public class MacImageListItemCell : EtoLabelFieldCell
{
  ....
  NSShadow textHighlightShadow;
}

Поле textHighlightShadow инициализируется только внутри свойства TextHighlightShadow. Таким образом, присваиваемое свойству значение не связано с возвращаемым значением, которое всегда будет одним и тем же объектом. При первом получении значения свойства, когда textHighlightShadow всегда является null, геттер создаёт этот объект и присваивает значение нескольким его свойствам, используя предопределённые значения. При этом существует свойство TextShadow, которое работает с полем textShadow:

public NSShadow TextShadow
{
  get
  {
    if (textShadow == null)
    {
      textShadow = new NSShadow();
      textShadow.ShadowColor = NSColor.FromDeviceWhite(1F, 0.5F);
      textShadow.ShadowOffset = new CGSize(0F, -1.0F);
      textShadow.ShadowBlurRadius = 0F;
    }
    return textShadow;
  }
  set { textShadow = value; }
}

Так как в сеттере TextHighlightShadow используется поле textShadow, то при каждом изменении TextHighlightShadow будет меняться и TextShadow. Сомнительно, что разработчик решил реализовать именно такое поведение.

Issue 10

public static NSImage ToNS(this Image image, int? size = null)
{
  ....
  if (size != null)
  {
    ....
    var sz = (float)Math.Ceiling(size.Value / mainScale);  // <=
    sz = size.Value;  // <=
  }
  ....
}

Предупреждение PVS-Studio: V3008 The 'sz' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 296, 295. Eto.Mac64 MacConversions.cs 296

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

На одной строке производятся объявление и инициализация переменной sz. И сразу на следующей строке значение sz перезаписывается, что делает вычисление инициализирующего значения бессмысленным.

Issue 11

public static IBinding BindingOfType(....)
{
  ....
  var ofTypeMethod = bindingType.GetRuntimeMethods()
                                .FirstOrDefault(....);
  return (IBinding)ofTypeMethod.MakeGenericMethod(toType)
                               .Invoke(...);
}

Предупреждение PVS-Studio: V3146 Possible null dereference of 'ofTypeMethod'. The 'FirstOrDefault' can return default null value. Eto BindingExtensionsNonGeneric.cs 21

Анализатор указывает, что метод FirstOrDefault, который используется для инициализации переменной ofTypeMethod, может вернуть null. Разыменование ofTypeMethod без проверки может привести к выбросу исключения NullReferenceException.

Если есть гарантия того, что элемент будет найден, следует использовать метод First:

var ofTypeMethod = bindingType.GetRuntimeMethods()
                               .First(r => 
                                         r.Name == "OfType"
                                      && r.GetParameters().Length == 2);

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

Заключение

Когда эталонная реализация .NET была крепко привязана к Windows, одним из достоинств той экосистемы была возможность быстро разрабатывать GUI-приложения. Со временем появились кроссплатформенные Mono, Xamarin и, в конце концов, .NET Core. Одним из первых желаний сообщества было портирование GUI-фреймворков с Windows на новые платформы. Появилось много хороших похожих фреймворков, использующих C# и XAML для разработки: Avalonia UI, Uno Platform и Eto.Forms. И, если вы знаете о похожем неупомянутом проекте, напишите, пожалуйста, о нём в комментариях. Даже как-то странно желать этим хороших проектам конкуренции, но конкуренция – двигатель прогресса.

PVS-Studio может помочь разработчикам этих проектов сделать код качественнее. Тем более что использовать анализатор в некоммерческих Open Source проектах можно бесплатно.

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

Большое спасибо за внимание, увидимся в следующих статьях!

#CSharp