Артём Ровенский

02 Ноя 2023

Теги:

#Cpp #CSharp

Microsoft PowerToys: Король GitHub среди C# проектов с C++ ошибками

02 Ноя 2023

Автор: Артём Ровенский

C# часть
С++ часть
Заключение

Microsoft PowerToys – это мощный и полезный инструмент. Он не только упрощает некоторые сценарии работы с Windows, но и привносит новые. А еще он занимает первую строчку топа по звездам на GitHub среди C# проектов. Посмотрим, насколько хорошо пишут код его разработчики, и узнаем, при чём здесь C++!

Кратко напомню для тех, кто не знаком с этим замечательным инструментом. Microsoft PowerToys — это набор утилит для взаимодействия с Windows, направленный на повышение продуктивности. Я, кстати, также в своей работе использую данный инструмент. Для меня самой полезной стала утилита 'What's using this file?'. Очень удобно сразу убить процесс, который взаимодействует с файлом, когда нужно удалить или переименовать этот файл.

Проект создаётся Open Source сообществом совместно с Microsoft. На момент написания статьи проект имеет 95 тысяч звёзд на GitHub. Это однозначный король среди C# проектов. При чём же здесь С++? На данный момент кодовая база проекта поделена следующим образом: C# – 60%, С++ – почти 40%.

Ожидаемо, что разработчики PowerToys должны писать качественный код. Давайте проверим, так ли это. Вооружимся статическим анализатором PVS-Studio и посмотрим, что интересного мы сможем найти. Получается кросс-языковая проверка кода. Подобные статьи у нас выходят не часто. Для анализа исходного кода и просмотра отчёта я использовал плагин PVS-Studio для Visual Studio 2022. С его помощью можно проводить анализ в один клик и легко просматривать отчёт:

Для удобства разобранные ошибки будут поделены на два блока: C# и C++. Так каждый читатель сможет легко найти свой язык. Но не стесняйтесь читать и про другой язык — я постарался описать ошибку так, чтобы она была понятна каждому.

Исходный код PowerToys доступен по ссылке. Проверялся следующий коммит.

C# часть

Поздние проверки

public List<Result> Query(Query query)
{
  bool isGlobalQuery = string.IsNullOrEmpty(query.ActionKeyword);
  ....

  if (query == null)
  {
    throw new ArgumentNullException(paramName: nameof(query));
  }

  // Happens if the user has only typed the action key so far
  if (string.IsNullOrEmpty(query.Search))
  {
    return new List<Result>();
  }
}

Предупреждение PVS-Studio: V3095 The 'query' object was used before it was verified against null. Check lines: 56, 60. Main.cs 56

Как видно из кода, параметр query проверяется на равенство null. Если query равен null, то выбрасывается исключение типа ArgumentNullException. Вот только до этой проверки query используется при обращении к свойству ActionKeyword. А это значит будь query равен null, поток выполнения до проверки не дойдёт, т.к. будет выброшен NullReferenceException.

private void SuggestionsList_SelectionChanged(....)
{
  ListView listview = (ListView)sender;
  _viewModel.Results.SelectedItem = (ResultViewModel)listview.SelectedItem;
  if (e.AddedItems.Count > 0 && e.AddedItems[0] != null)
  {
    ....
  }

  if (_viewModel.Results != null &&
      !string.IsNullOrEmpty(SearchBox.QueryTextBox.Text))
  {
    ....
  }
}

Предупреждение PVS-Studio: V3095 The '_viewModel.Results' object was used before it was verified against null. Check lines: 542, 563. MainWindow.xaml.cs 542

И ещё одна запоздалая проверка. Свойство _viewModel.Results сначала используется, а уже после проверяется на null. Возможно, подобный код появился вследствие поспешного рефакторинга.

Настроек нет, держи NRE

public LauncherViewModel(....)
{
  ....
  
  if (updatingSettingsConfig == null)
  {
    updatingSettingsConfig = new UpdatingSettings();
  }

  updatingSettingsConfig = UpdatingSettings.LoadSettings();

  if (updatingSettingsConfig.State == ....)
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'updatingSettingsConfig'. LauncherViewModel.cs 138

Анализатор рекомендует проверить updatingSettingsConfig. По коду видно, что updatingSettingsConfig проверяется на null, и если значение переменной равно null, то в неё будет записана ссылка на новый объект. После этого в updatingSettingsConfig безусловно записывается результат работы метода LoadSettings. А зачем тогда нужен был предыдущий код?

Давайте взглянем на код метода LoadSettings:

public static UpdatingSettings LoadSettings()
{
  FileSystem fileSystem = new FileSystem();
  var localAppDataDir = Environment.GetFolderPath(....);
  var file = localAppDataDir + SettingsFilePath + SettingsFile;

  if (fileSystem.File.Exists(file))
  {
    try
    {
      Stream inputStream = fileSystem.File.Open(file, FileMode.Open);
      StreamReader reader = new StreamReader(inputStream);
      string data = reader.ReadToEnd();
      ....
    }
    catch (Exception)
    {}
  }

  return null;
}

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

Неожиданная ошибка

private static int CalculateClosestSpaceIndex(List<int> spaceIndices,
                                              int firstMatchIndex)
{
  if (spaceIndices.Count == 0)
  {
    return -1;
  }
  else
  {
    int? ind = spaceIndices.OrderBy(item => (firstMatchIndex - item))
                           .Where(item => firstMatchIndex > item)
                           .FirstOrDefault();
    int closestSpaceIndex = ind ?? -1;
    return closestSpaceIndex;
  }
}

Предупреждение PVS-Studio: V3022 Expression 'ind' is always not null. The operator '??' is excessive. StringMatcher.cs 230

Анализатор посчитал, что оператор '??' не нужен, так как ind всегда не равен null. Давайте разбираться, так ли это. Ind имеет nullable тип, а значит может принимать null. Значение, записанное в Ind, было получено с использованием метода FirstOrDefault, который может вернуть null. Дело закрыто, анализатор ошибся? Не просто же так здесь этот код :).

Если быть точнее, то FirstOrDefault возвращает не null, а default(TSource). default(int?) – это null. Вот только TSource в данном случае int, так как TSource берётся по типу элементов перечисляемой последовательности. В данном случае LINQ применяется к параметру spaceIndices с типом List<int>. Получается, что в ind будет записан default(int), т.е. 0. А вот и ошибка в логике. Метод поиска в случае не нахождения вернёт 0, а не -1, как должен.

Странные сдвиги

public static List<PluginPair> AllPlugins
{
  get
  {
    ....
    try
    {
      // Return a comparable produce version.
      var fileVersion = FileVersionInfo.GetVersionInfo(x.ExecuteFilePath);
      return ((uint)fileVersion.ProductMajorPart << 48)
      | ((uint)fileVersion.ProductMinorPart << 32)
      | ((uint)fileVersion.ProductBuildPart << 16)
      | ((uint)fileVersion.ProductPrivatePart);
      }
    catch (System.IO.FileNotFoundException)
    {
      return 0U;
    }
    ....
  }
}

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

V3134 Shift by 48 bits is greater than the size of 'UInt32' type of expression '(uint)fileVersion.ProductMajorPart'. PluginManager.cs 62
V3134 Shift by 32 bits is greater than the size of 'UInt32' type of expression '(uint)fileVersion.ProductMinorPart'. PluginManager.cs 63

Не обращайте внимания на тип свойства и на тип фактически возвращаемого значения. Код пришлось сильно сократить, поэтому отрывок, который я демонстрирую, относится к лямбда выражению. Анализатор выдал на этот код два предупреждения. Тип uint имеет размер 32 бита. Получается, что первый сдвиг на 48 бит эквивалентен сдвигу на 16. Второй сдвиг на 32 бита эквивалентен сдвигу на 0 бит. Сложно сказать, что здесь хотели сделать, но, возможно, вместо uint стоит использовать ulong.

Как нельзя использовать оператор '?.'

public void Load(IMainView view)
{
  if (_batch.Files.Count == 0)
  {
    _batch.Files.AddRange(view?.OpenPictureFiles());
  }
  ....
}

Предупреждение PVS-Studio: V3105 The result of null-conditional operator is dereferenced inside the 'AddRange' method. NullReferenceException is possible. Inspect the first argument 'view?.OpenPictureFiles()'. MainViewModel.cs 47

В данном случае результат оператора '?.' передают в качестве аргумента метода AddRange. Это пользовательский метод расширения для ICollection<T>. Взглянем на код этого метода:

public static void AddRange<T>(this ICollection<T> collection,
                               IEnumerable<T> items)
{
  foreach (var item in items)
  {
    collection.Add(item);
  }
}

Интересующий нас параметр items, который может быть null, используется в foreach. Как мы помним, foreach никак не защищает нас от NullReferenceException. Помним же? Для многих программистов подобное поведение неочевидно. Мы даже писали статью на эту тему: "Использование оператора ?. в foreach: защита от NullReferenceException, которая не работает".

С++ часть

Утечка ресурса

void close_settings_window()
{
  if (g_settings_process_id != 0)
  {
    HANDLE proc = OpenProcess(PROCESS_TERMINATE, false, g_settings_process_id);
    if (proc != INVALID_HANDLE_VALUE)
    {
      TerminateProcess(proc, 0);
    }
  }
}

Предупреждение PVS-Studio: V773 Visibility scope of the 'proc' handle was exited without releasing the resource. A resource leak is possible. settings_window.cpp 635

Никто не любит, когда в приложении текут ресурсы. В данном случае с помощью функции OpenProcess получают дескриптор указанного процесса, после чего этот процесс убивают. Вот только после завершения работы с дескриптором нужно его закрыть с помощью функции CloseHandle (о чем Microsoft заботливо напоминает в ремарках к функции OpenProcess). Если этого не сделать, то получаем утечку ресурса.

Бесполезная проверка

void FancyZonesSettings::LoadSettings()
{
  ....
  if (auto val = values.get_int_value(....))
  {
    // Avoid undefined behavior
    if (   *val >= 0
        || *val < static_cast<int>(OverlappingZonesAlgorithm::EnumElements))
    {
      auto algorithm = (OverlappingZonesAlgorithm)*val;
      if (m_settings.overlappingZonesAlgorithm != algorithm)
      {
        m_settings.overlappingZonesAlgorithm = algorithm;
        NotifyObservers(SettingId::OverlappingZonesAlgorithm);
      }
    }
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression is always true. Settings.cpp 241

А вот здесь разработчики написали условие, результат которого всегда равен true. Значение переменной val больше или равно 0 или меньше константы EnumElements перечисления OverlappingZonesAlgorithm, которая имеет значение 4. Получается, что любое значение, которое может лежать в val, подходит под условие. Скорее всего, разработчики должны были использовать оператор '&&' вместо '||'.

Недостижимый код

void FancyZonesWindowUtils::SizeWindowToRect(HWND window, RECT rect) noexcept
{
  ....
  if ((placement.showCmd != SW_SHOWMINIMIZED) &&
      (placement.showCmd != SW_MINIMIZE))
  {
    placement.showCmd = SW_RESTORE;
  }
  // Remove maximized show command to make sure 
  // window is moved to the correct zone.
  if (placement.showCmd == SW_SHOWMAXIMIZED)
  {
    placement.showCmd = SW_RESTORE;
    placement.flags &= ~WPF_RESTORETOMAXIMIZED;
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression 'placement.showCmd == 3' is always false. WindowUtils.cpp 336

Ещё одна ошибка с булевыми выражениями, только в этот раз условие всегда false. Анализатор выдаёт своё предупреждение на второй if. В коде используют макросы. Взглянем на определение нужных нам макросов:

....
#define SW_SHOWMINIMIZED    2
#define SW_SHOWMAXIMIZED    3
....
#define SW_MINIMIZE         6
....
#define SW_RESTORE          9
....

Анализатор считает, что условие всегда false, тогда пойдём от обратного. Чтобы условие второго if было true, значение поля showCmd должно быть равно 3. Взглянем на первый if. Если значение поля showCmd не равно 2 и не равно 6, то присваиваем ему 9. Легко понять, что 3 не равно 2 и 6. Получается, что анализатор прав, и условие всегда false. Вдобавок к этому код внутри никогда не выполняется.

Самая опасная функция в мире C и C++

void SetNumLockToPreviousState(....)
{
    int key_count = 2;
    LPINPUT keyEventList = new INPUT[size_t(key_count)]();
    memset(keyEventList, 0, sizeof(keyEventList));
    ....
}

На этот код PVS-Studio выдаёт сразу три предупреждения:

V579 The memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. KeyboardEventHandlers.cpp 16
V568 It's odd that 'sizeof()' operator evaluates the size of a pointer to a class, but not the size of the 'keyEventList' class object. KeyboardEventHandlers.cpp 16
V1086 A call of the 'memset' function will lead to underflow of the buffer 'keyEventList'. KeyboardEventHandlers.cpp 16

На тему подобных ошибок у нас даже есть отдельная статья: "Самая опасная функция в мире С/С++".

В данном случае разработчики хотели занулить массив keyEventList. Обратим внимание на 3 параметр – количество байт, которые хотели заполнить нулями. Вот только sizeof(keyEventList) вычисляет не размер массива, а размер указателя. Он зависит от целевой платформы, но чаще всего это 4 или 8 байт. При этом размер структуры явно больше 4 или 8 байт:

typedef struct tagINPUT {
    DWORD   type;

    union
    {
        MOUSEINPUT      mi;
        KEYBDINPUT      ki;
        HARDWAREINPUT   hi;
    } DUMMYUNIONNAME;
} INPUT, *PINPUT, FAR* LPINPUT;

Идентичные выражения

int WINAPI wWinMain(....)
{
  // Start a thread to listen on the events.
  m_reparent_event_handle = CreateEventW(....);
  m_thumbnail_event_handle = CreateEventW(....);
  m_exit_event_handle = CreateEventW(....);
  if (   !m_reparent_event_handle 
      || !m_reparent_event_handle 
      || !m_exit_event_handle)
  {
    ....
  }
}

Предупреждение PVS-Studio: V501 There are identical sub-expressions '!m_reparent_event_handle' to the left and to the right of the '||' operator. main.cpp 191

По сообщению анализатора сразу всё понятно. Слева и справа от оператора '||' используется m_reparent_event_handle. А вот m_thumbnail_event_handle не используется. Простая, но такая обидная ошибка :).

Мусорное значение

LRESULT Toolbar::WindowProcessMessages(...., UINT msg, ....)
{
  switch (msg)
  {
    ....
    case WM_DPICHANGED:
    {
      UINT dpi = LOWORD(dpi);
      ....
    }
    ....
  }
}

Предупреждение PVS-Studio: V573 Uninitialized variable 'dpi' was used. The variable was used to initialize itself. Toolbar.cpp 112

В коде используют только что объявленную неинициализированную переменную для определения значения самой себя. В итоге переменная dpi будет инициализирована случайным значением.

Неправильное использование #pragma warning

....
#pragma warning(disable : 5205)
#include <winrt/base.h>
#pragma warning(default : 5205)
....

Предупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 6, 8. pch.h 8

Анализатор обнаружил неправильное использование директив #pragma warning. В данном случае программист захотел отключить предупреждение, а после включить обратно. Однако директива pragma warning(default : X) устанавливает предупреждение с номером X в состояние, которое действует по умолчанию. Это не то же самое, что включить отключенное предупреждение. Корректное использование будет следующим:

....
#pragma warning(push)
#pragma warning(disable : 5205)
#include <winrt/base.h>
#pragma warning(pop)
....

Подозрительная проверка

std::string toMediaTypeString(GUID subtype)
{
  if (subtype == MFVideoFormat_YUY2)      // <=
    return "MFVideoFormat_YUY2";
  else if (subtype == MFVideoFormat_RGB32)
    return "MFVideoFormat_RGB32";
  else ....
  else if (subtype == MFVideoFormat_AYUV)
    return "MFVideoFormat_AYUV";
  else if (subtype == MFVideoFormat_YUY2) // <=
    return "MFVideoFormat_YUY2";
  else ....
}

Предупреждение PVS-Studio: V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 80, 102. Logging.cpp 80

Я довольно часто встречаю подобные ошибки. В условии if использовали выражение subtype == MFVideoFormat_YUY2 два раза. Возможно, что это просто лишняя проверка. Но часто такой код пишут с помощью copy-paste и могли что-то забыть проверить. Тем более этот код содержит последовательность из 61 условия!

Бесхозное выражение

winrt::com_ptr<ID2D1Bitmap> ConvertID3D11Texture2DToD2D1Bitmap(....)
{
  capturedScreenTexture->view.pixels;
  ....
}

Предупреждение PVS-Studio: V607 Ownerless expression 'capturedScreenTexture->view.pixels'. MeasureToolOverlayUI.cpp 35

Лишнее выражение в коде. Сложно сказать, стоит ли это выражение просто убрать, или в логике программы есть ошибка. Часто такой код получается при рефакторинге, и, хотя код компилируется, смысла он не имеет.

Unsigned type value < 0

std::wstring NtdllExtensions::pid_to_user(DWORD pid)
{
  ....
  DWORD token_size = 0;
  GetTokenInformation(token, TokenUser, nullptr, 0, &token_size);

  if (token_size < 0)
  {
    return user;
  }
  ....
}

Предупреждение PVS-Studio: V547 Expression 'token_size < 0' is always false. Unsigned type value is never < 0. NtdllExtensions.cpp 314

Переменная token_size проверяется, что она меньше 0. Проблема в том, что это переменная типа DWORD, который является беззнаковым. Получается, что условие всегда false.

Заключение

Разработчики Microsoft PowerToys создают полезный инструмент, который многие (и я в их числе) активно используют. Было действительно интересно смотреть на то, что в коде нашёл PVS-Studio. Мне не очень хотелось в статье говорить про одни и же баги или писать про неинтересные ошибки. Вы всегда можете проверить сами любой проект с помощью PVS-Studio – скачать и попробовать можно по ссылке. Может, найдёте что-то интересное и сможете поделиться с нами? :)

Я же продолжу использовать PowerToys в своей работе и ждать обновлений с новыми утилитами.

Также я рекомендую посетить страницу проекта на GitHub, может, вы сможете внести что-то своё в развитие проекта :) Удачи!

#Cpp #CSharp