Files — это сторонний файловый менеджер для Windows. Он сделан для того, чтобы быть лучшим открытым файловым менеджером для ОС, который поддерживается сообществом. А в этой статье мы взглянем на ошибки в исходном коде Files и внесём свой вклад в open source сообщество.

Прежде чем мы перейдём непосредственно к анализу проекта, предлагаю поближе с ним познакомиться. На момент проверки Files имеет на GitHub чуть больше 39 тысяч звёзд. Это достаточно большой показатель. Files ориентирован на удобство пользователя, организацию файлов и папок и богатый функционал с глубокими интеграциями.

Вот пример интерфейса с сайта проекта:

Как мне кажется, он имеет более современный дизайн по сравнению с проводником в Windows 10 и 11. При этом в Files есть поддержка палитры команд, интеграция с Git, удобные интеграции с облачными хранилищами и многое другое. Вы можете гибко настроить его под себя, изменяя дизайн, переназначая сочетания клавиш и создавая свои команды.

Ранее я не использовал Files, но после знакомства с ним точно дам шанс этому проекту. Хотя тяжело перестать машинально использовать встроенный проводник Windows после стольких лет.

Перейдём к проверке исходного кода. Он доступен по ссылке. Проверялся следующий коммит. Для проверки использовался статический анализатор PVS-Studio. Он может анализировать код, написанный на C, C++, C# и Java. А Files как раз написан на C#.

Кстати, этот проект уже перешёл с формата решений .sln на .slnx. Если вам интересно, чем он лучше прошлого формата, или вы вообще не слышали про .slnx, то пригашаю почитать эту статью. PVS-Studio уже поддерживает новый формат решений, так что проблем это не добавляет.

Конечно, мы не будем рассматривать все ошибки, которые обнаружил анализатор. В рамках статьи смотреть десятки похожих предупреждений было бы неинтересно. Всего PVS-Studio выдал около 250 предупреждений. И, как я говорил, мы внесём свой вклад в open source сообщество, сообщив разработчикам о найденных потенциальных проблемах. А теперь поехали!

Ошибки в коде

Issue 1

protected void ChangeMode(OmnibarMode? oldMode, OmnibarMode newMode)
{
  ....
  var modeSeparatorWidth = 
    itemCount is not 0 or 1 
      ? _modesHostGrid.Children[1] is FrameworkElement frameworkElement
        ? frameworkElement.ActualWidth
        : 0
      : 0;
  ....
}

Предупреждение PVS-Studio: V3207 [CWE-670] The 'not 0 or 1' logical pattern may not work as expected. The 'not' pattern is matched only to the first expression from the 'or' pattern. Files.App.Controls Omnibar.cs 149

А начнём мы с одной интересной и в то же время лёгкой ошибки. В этом коде нас интересует часть itemCount is not 0 or 1. Уже догадались? Этот паттерн избыточен. Его вторая часть ни на что не влияет.

Люди привыкли, что когда мы в речи говорим "x это не 0 или 1", то имеем в виду, что x это не 0 и не 1. Но в C# приоритет операций другой. x is not 0 or 1 на самом деле x is (not 0) or 1. Подобные ошибки могут приводить не просто к избыточности, но и к ошибкам по типу NullReferenceException: list is not null or list.Count == 0. По этой проблеме даже была встреча, на которой обсуждались возможные пути решения. Исходя из обсуждения, вероятно, в будущем эта ситуация будет отлавливаться или компилятором, или встроенным статическим анализом как предупреждение.

Issue 2

private void SetPathData(string pathData, FrameworkElement element)
{
  ....
  if (GetTemplateChild(LayerPathPart) is Path path)
  {
    path.Data = geometry;
    path.Width = element.Width;
    path.Width = element.Height;
  }
}

Предупреждение PVS-Studio: V3127 [CWE-682] Two similar code fragments were found. Perhaps, this is a typo and 'Height' variable should be used instead of 'Width' Files.App.Controls ThemedIconLayer.cs 122

Часто встречаемая copy-paste ошибка. Вместо path.Height присваивание происходит дважды в path.Width.

Issue 3

private void UpdateRadii(double newRadius, bool isTrack)
{
  double valueRingThickness = _valueRingThickness;
  double trackRingThickness = _trackRingThickness;

  // Limit the Thickness values to no more than 1/5 of the container size
  if (isTrack)
    trackRingThickness =   newRadius > (AdjustedSize / 5) 
                         ? (AdjustedSize / 5) : newRadius;
  else
    valueRingThickness =   newRadius > (AdjustedSize / 5)
                         ? (AdjustedSize / 5) : newRadius;

  // If both Rings have Equal thickness, use 0;
  // otherwise, use the larger thickness to adjust the size
  double check = (AdjustedSize / 2) – 
    (_thicknessCheck is ThicknessCheck.Equal ? 0 : _largerThickness / 2);
  double minSize = 4;

  _sharedRadius = check <= minSize ? minSize : check;
}

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

V3137 [CWE-563] The 'trackRingThickness' variable is assigned but is not used by the end of the function. Files.App.Controls StorageRing.cs 211

V3137 [CWE-563] The 'valueRingThickness' variable is assigned but is not used by the end of the function. Files.App.Controls StorageRing.cs 213

Анализатор обнаружил, что две локальные переменные не используются после присваивания им значений. Значения из полей valueRingThickness и trackRingThickness записываются в локальные переменные trackRingThickness и valueRingThickness. Далее они, в зависимости от условия, изменяются, но позже в методе не используются. Вполне вероятно, что локальные переменные должны были использоваться в дальнейших операциях.

Issue 4

public Task ExecuteAsync(object? parameter = null)
{
  if (context.HasSelection && context?.SelectedItem?.ItemPath is not null)
    ExecuteShellCommand(context.SelectedItem.ItemPath); 
  else if (context?.Folder?.ItemPath is not null)
    ExecuteShellCommand(context.Folder.ItemPath);

  return Task.CompletedTask;
}

Предупреждение PVS-Studio: V3042 [CWE-476] Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'context' object Files.App OpenClassicPropertiesAction.cs 40

Привычная для C# разработчиков ошибка — NullReferenceException. В условии if разыменовывается context без проверки на null, хотя далее в этом уже условии и в else if есть проверка с помощью ?..

Issue 5

public async Task ExecuteAsync(object? parameter = null)
{
  if (context.SelectedItems.Count > 0)
  {
    foreach (ListedItem listedItem in 
             context.ShellPage?.SlimContentPage.SelectedItems)
    {
      ....
    }
  }
}

Предупреждение PVS-Studio: V3153 [CWE-476] Enumerating the result of null-conditional access operator can lead to NullReferenceException. Files.App UnpinFromStartAction.cs 32

Ещё один потенциальный NRE. В цикле foreach перечисляемая коллекция получается с использованием оператора ?.. Не все разработчики знают, что foreach не работает с null. Этот тип ошибки достаточно часто встречается. На эту тему есть даже отдельная статья: "Использование оператора ?. в foreach: защита от NullReferenceException, которая не работает".

Issue 6

private void Window_Activated(object sender, WindowActivatedEventArgs args)
{
  AppModel.IsMainWindowClosed = false;

  // TODO(s): Is this code still needed?
  if (args.WindowActivationState != WindowActivationState.CodeActivated ||
      args.WindowActivationState != WindowActivationState.PointerActivated)
    return;

  ApplicationData.Current.LocalSettings.Values["INSTANCE_ACTIVE"] = 
    -Environment.ProcessId;
}

Предупреждение PVS-Studio: V3022 [CWE-571] Expression is always true. Probably the '&&' operator should be used here. Files.App App.xaml.cs 181

Анализатор обнаружил условие оператора if, которое всегда true. args.WindowActivationState сравнивается с элементами перечисления. Из-за ошибочного использования оператора ||, вместо && в условии проверяется, что значение из свойства WindowActivationState не равно или WindowActivationState.CodeActivated, или WindowActivationState.PointerActivated. Оно всегда не равно чему-то одному.

Выливается это ещё и в недостижимость кода, который идёт после if: V3142 [CWE-561] Unreachable code detected. It is possible that an error is present. Files.App App.xaml.cs 185

Кстати, можно заметить комментарий "Этот код всё ещё нужен?". Но он и в прошлом не был нужен, и в будущем не нужен, т.к. содержит ошибку. Если разработчики хотели сделать недостижимый код, то можно просто поставить безусловный return.

Issue 7

public AccessControlEntry(bool isFolder,
                          string ownerSid,
                          AccessControlEntryType type,
                          AccessMaskFlags accessMaskFlags,
                          bool isInherited,
                          AccessControlEntryFlags inheritanceFlags)
{
  AccessMaskItems = SecurityAdvancedAccessControlItemFactory
                      .Initialize(this,
                                  AreAdvancedPermissionsShown,
                                  IsInherited,
                                  IsFolder);

  ChangeAccessControlTypeCommand = new RelayCommand<string>(x =>
  {
    //AccessControlType = Enum.Parse<AccessControlType>(x);
  });

  ChangeInheritanceFlagsCommand = new RelayCommand<string>(x =>
  {
    //var parts = x.Split(',');
    //InheritanceFlags = Enum.Parse<AccessControlEntryFlags>(parts[0]);
  });

  IsFolder = isFolder;
  Principal = new(ownerSid);
  AccessControlType = type;
  AccessMaskFlags = accessMaskFlags;
  IsInherited = isInherited;
  AccessControlEntryFlags = inheritanceFlags;
  ....
}

Видите ошибку? А их тут целых две:

• V3128 [CWE-665] The 'IsFolder' property is used before it is initialized in constructor. Files.App AccessControlEntry.cs 337

• V3128 [CWE-665] The 'IsInherited' property is used before it is initialized in constructor. Files.App AccessControlEntry.cs 337

Здесь свойства IsFolder и IsInherited используются до того, как произойдёт их инициализация в этом же конструкторе с помощью соответствующих им аргументов. Выходит, что IsFolder и IsInherited всегда передаются в метод SecurityAdvancedAccessControlItemFactory.Initialize с дефолтным значением false.

Issue 8

protected override async IAsyncEnumerable<ICloudProvider> GetProviders()
{
  ....
  var reader = cmdConnection.ExecuteReader();
  while (reader.Read())
  {
    var connection =
    (
      ConnectionType: reader["conn_type"]?.ToString(),
  HostName: reader["host_name"]?.ToString()
    );

    connections.Add(reader["id"]?.ToString(), connection);
  }
  ....
}

Предупреждение PVS-Studio: V3105 [CWE-690] The result of null-conditional operator is passed as the first argument to the 'Add' method and is not expected to be null. Files.App SynologyDriveCloudDetector.cs 52

У анализатора есть информация о том, в какой библиотечный метод можно передавать null, а в какой нельзя. Мы называем эту технологию аннотированием методов. Из коробки в анализаторе есть информацию, например, о системных библиотеках, Unity, ASP.NET Core и т.д. Подробнее про технологии статического анализа вы можете прочитать в этой статье. Так, метод Add — это экземплярный метод для Dictionary. При передаче null первым аргументом, т.е. в качестве ключа словаря, будет выброшено исключение ArgumentNullException. В качестве первого аргумента идёт выражение с использованием ?.. Не могу сказать, насколько это возможная ситуация, но ошибка есть ошибка.

Issue 9

public void Log<TState>(....)
{
  if (exception is null ||
      exception.Data[Mechanism.HandledKey] is false ||
      logLevel <= LogLevel.Information)
    return;

  var generalSettingsService = 
    Ioc.Default.GetRequiredService<IGeneralSettingsService>();

  var level = logLevel switch
  {
    LogLevel.Debug => SentryLevel.Debug,
    LogLevel.Information => SentryLevel.Info,
    LogLevel.Warning => SentryLevel.Warning,
    LogLevel.Error => SentryLevel.Error,
    LogLevel.Critical => SentryLevel.Fatal,
    _ => SentryLevel.Debug
  };
}

Предупреждение PVS-Studio: V3202 [CWE-561] Unreachable code detected. The 'case' value is out of range of the match expression. Files.App SentryLogger.cs 35

Ещё немного недостижимого кода. В switch соотносится значение logLevel и элементов перечисления LogLevel. Но обратите внимание, что перед switch есть проверка: если logLevel <= LogLevel.Information, то return. ��зглянем на определение перечисления LogLevel:

public enum LogLevel
{
  Trace = 0,
  Debug = 1,
  Information = 2,
  ....
}

Т.е., исходя из условия, Trace, Debug и Information не доходят до switch. Но как раз-таки Debug и Information мы видим в первые двух выражениях в switch.

Issue 10

private async void Omnibar_QuerySubmitted(....)
{
  ....
  if (action is not null)
  {
    var overload = action.GetOverloads().FirstOrDefault();
    await overload?.InvokeAsync(actionInstance.Context);
  }
  ....
}

Предупреждение PVS-Studio: V3168 [CWE-476] Awaiting on the 'overload?.InvokeAsync(actionInstance.Context)' expression with potential null value can lead to NullReferenceException. Files.App NavigationToolbar.xaml.cs 214

Ещё один потенциальный NRE, но теперь не из-за foreach или метода, который не работает с null. Здесь используется оператор await с выражением, значением которого может быть null. Анализатор предполагает это из-за оператора ?..

Issue 11

private void ReportProgress(StatusCenterItemProgressModel value)
{
  switch (value.TotalSize, value.ItemsCount)
  {
    // In progress, displaying items count & processed size
    case (not 0, not 0):
      ProgressPercentage = 
        Math.Clamp((int)(value.ProcessedSize * 100.0 / value.TotalSize),
                   0, 100);
      SpeedText = $"{value.ProcessingSizeSpeed.ToSizeString()}/s";
      point = 
        new((float)(value.ProcessedSize * 100.0 / value.TotalSize),
            (float)value.ProcessingSizeSpeed);

  break;
    // In progress, displaying processed size
    case (not 0, _):
      ProgressPercentage =
        Math.Clamp((int)(value.ProcessedSize * 100.0 / value.TotalSize),
                   0, 100);
      SpeedText = $"{value.ProcessingSizeSpeed.ToSizeString()}/s";
      point =
        new((float)(value.ProcessedSize * 100.0 / value.TotalSize),
            (float)value.ProcessingSizeSpeed);

  break;
    // In progress, displaying items count
    case (_, not 0):
      ProgressPercentage = 
        Math.Clamp((int)(value.ProcessedItemsCount * 100.0 / value.ItemsCount),
                   0, 100);
      SpeedText = $"{value.ProcessingItemsCountSpeed:0} items/s";
      point = 
        new((float)(value.ProcessedItemsCount * 100.0 / value.ItemsCount),
            (float)value.ProcessingItemsCountSpeed);

  break;
    default:
      ....
  }
  ....
}

Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. Files.App StatusCenterItem.cs 342

Анализатор обнаружил ситуацию, когда в switch первые два case содержат одинаковые фрагменты кода. Здесь эти фрагменты достаточно большие, так что вряд ли их написали просто так вместо того, чтобы объединить оба case:

case (not 0, not 0):
case (not 0, _):

Вероятно, они должны немного отличаться. Но чем? Давайте посмотрим на комментарии к case:

// Set speed text and percentage
switch (value.TotalSize, value.ItemsCount)
{
  // In progress, displaying items count & processed size
  case (not 0, not 0):
    ....
  // In progress, displaying processed size
  case (not 0, _):
    ....
  // In progress, displaying items count
  case (_, not 0):
    ....
  default:
    ....
}

Читаем и видим:

• в первом случае в прогресс отображается количество элементов и обработанный размер чего-то;

• во втором случае в прогресс отображается обработанный размер чего-то;

• в третьем случае в прогресс отображается количество элементов.

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

• в первом case используется только свойство ProcessedSize;

• во втором case используется только свойство ProcessedSize;

• в третьем case используется только свойство ProcessedItemsCount.

Выходит, что в первом case не хватает обработки для отображения ProcessedItemsCount. Поздравляю, мы добрались до проблемы.

Issue 12

public bool LoadTagsList
  => SelectedItem?.HasTags ?? false &&
  PreviewPaneState is PreviewPaneStates.NoPreviewAvailable ||
  PreviewPaneState is PreviewPaneStates.PreviewAndDetailsAvailable;

Предупреждение PVS-Studio: V3177 [CWE-783] The 'false' literal belongs to the '||' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. Files.App InfoPaneViewModel.cs 122

Ещё одна не самая очевидная ошибка. Здесь всё дело в приоритете операций. Т.к. приоритет оператора ?? ниже, чем у &&, сначала будет выполнена проверка выражения false && PreviewPaneState is PreviewPaneStates.NoPreviewAvailable, которая всегда возвращает false. Сомнительно, что именно этого хотел добиться разработчик.

Issue 13

private static string FormatEncodingBitrate(object input)
{
  // For cases when multiple files are selected and it has a string value
  if (input.GetType() != typeof(uint))
    return input?.ToString() ?? string.Empty;

  ....
}

Предупреждение PVS-Studio: V3095 [CWE-476] The 'input' object was used before it was verified against null. Check lines: 375, 376. Files.App FileProperty.cs 375

Анализатор сигнализирует о том, что input сначала используется без проверки на null, а после с проверкой. Не первый раз встречаю паттерн, когда объект не проверяют на равенство null при вызове метода GetType(). Может, кто-то думает, что он работает как метод расширения, но нет. При вызове получим NullReferenceException.

Issue 14

private void ColumnViewBase_KeyUp(object sender, ....)
{
   var shPage = ActiveColumnShellPage as ColumnShellPage;
   if (shPage?.SlimContentPage?.SelectedItem?.PrimaryItemAttribute
       is not StorageItemTypes.Folder)
    CloseUnnecessaryColumns(shPage?.ColumnParams);         // <=
}

private void CloseUnnecessaryColumns(ColumnParam column)
{
  if (string.IsNullOrEmpty(column.NavPathParam))
    return;
  ....
}

Предупреждение PVS-Studio: V3105 [CWE-690] The result of null-conditional operator is dereferenced inside the 'CloseUnnecessaryColumns' method. NullReferenceException is possible. Inspect the first argument 'shPage?.ColumnParams'. Files.App ColumnsLayoutPage.xaml.cs 287

Просто пример работы межпроцедурного анализа. Анализатор увидел, что в метод CloseUnnecessaryColumns передаётся значение выражения shPage?.ColumnParams, которое может быть null. В методе CloseUnnecessaryColumns параметр сразу разыменовывается, что может привести к NRE.

При этом условие в if не спасает от проблемы. Условие в if будет равно true, если знаечние переменной shPage будет равно null.

Issue 15

protected override void OnNavigatedTo(NavigationEventArgs eventArgs)
{
  ....
  if (FolderSettings?.ColumnsViewModel is not null)   // <=
  {
    ColumnsViewModel.DateCreatedColumn.Update(
      FolderSettings.ColumnsViewModel.DateCreatedColumn);
    ColumnsViewModel.DateDeletedColumn.Update(
      FolderSettings.ColumnsViewModel.DateDeletedColumn);
    ColumnsViewModel.DateModifiedColumn.Update(
      FolderSettings.ColumnsViewModel.DateModifiedColumn);
    ....
  }
  ParentShellPageInstance.ShellViewModel.EnabledGitProperties =
    GetEnabledGitProperties(ColumnsViewModel);

  FolderSettings.LayoutModeChangeRequested +=         // <=
    FolderSettings_LayoutModeChangeRequested;
  ....
}

Предупреждение PVS-Studio: V3125 [CWE-476] The 'FolderSettings' object was used after it was verified against null. Check lines: 174, 148. Files.App DetailsLayoutPage.xaml.cs 174

Проблема похожа на те, что мы рассматривали ранее. Свойство FolderSettings проверяется на равенство null в условии if и далее используется в then-ветви. Но после if используется уже без проверки.

Заключение

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

Удачи!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. Files file manager: folder full of bugs.