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

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

![1078_PowerToys_ru/image1.png](https://import.viva64.com/docx/blog/1078_PowerToys_ru/image1.png)

Кратко напомню для тех, кто не знаком с этим замечательным инструментом\. 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\. С его помощью можно проводить анализ в один клик и легко просматривать отчёт:

![1078_PowerToys_ru/image2.png](https://import.viva64.com/docx/blog/1078_PowerToys_ru/image2.png)

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

Исходный код PowerToys доступен по [ссылке](https://github.com/microsoft/PowerToys)\. Проверялся следующий [коммит](https://github.com/microsoft/PowerToys/commit/83f062542778a14d5dd260d0018d3db1f6c84af6)\.

## C\# часть

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_\.

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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

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

  updatingSettingsConfig = UpdatingSettings.LoadSettings();

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

Предупреждение PVS\-Studio: [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'updatingSettingsConfig'\. LauncherViewModel\.cs 138

Анализатор рекомендует проверить _updatingSettingsConfig\._ По коду видно, что _updatingSettingsConfig_ проверяется на _null_, и если значение переменной равно _null_, то в неё будет записана ссылка на новый объект\. После этого в _updatingSettingsConfig_ безусловно записывается результат работы метода _LoadSettings_\. А зачем тогда нужен был предыдущий код?

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

```cpp
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_, то произойдёт разыменование нулевой ссылки с выбрасыванием соответствующего исключения\.

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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, как должен\.

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3134/) Shift by 48 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMajorPart'\. PluginManager\.cs 62
* [V3134](https://pvs-studio.ru/ru/docs/warnings/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_\.

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

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

Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/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\>_\. Взглянем на код этого метода:

```cpp
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, которая не работает](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)"\.

![1078_PowerToys_ru/image3.png](https://import.viva64.com/docx/blog/1078_PowerToys_ru/image3.png)

## С\+\+ часть

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v773/) Visibility scope of the 'proc' handle was exited without releasing the resource\. A resource leak is possible\. settings\_window\.cpp 635

Никто не любит, когда в приложении текут ресурсы\. В данном случае с помощью функции [_OpenProcess_](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocess) получают дескриптор указанного процесса, после чего этот процесс убивают\. Вот только после завершения работы с дескриптором нужно его закрыть с помощью функции [_CloseHandle_](https://learn.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-closehandle) \(о чем Microsoft заботливо напоминает в [ремарках](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocess#remarks) к функции _OpenProcess_\)\. Если этого не сделать, то получаем утечку ресурса\.

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression is always true\. Settings\.cpp 241

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'placement\.showCmd \=\= 3' is always false\. WindowUtils\.cpp 336

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

```cpp
....
#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\+\+

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

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

* [V579](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/docs/warnings/v1086/) A call of the 'memset' function will lead to underflow of the buffer 'keyEventList'\. KeyboardEventHandlers\.cpp 16

На тему подобных ошибок у нас даже есть отдельная статья: "[Самая опасная функция в мире С/С\+\+](https://pvs-studio.ru/ru/blog/posts/cpp/0360/)"\.

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

```cpp
typedef struct tagINPUT {
    DWORD   type;

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_ не используется\. Простая, но такая обидная ошибка :\)\.

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

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

Предупреждение PVS\-Studio: [V573](https://pvs-studio.ru/ru/docs/warnings/v573/) Uninitialized variable 'dpi' was used\. The variable was used to initialize itself\. Toolbar\.cpp 112

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

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

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

Предупреждение PVS\-Studio: [V665](https://pvs-studio.ru/ru/docs/warnings/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_ в состояние, которое действует по умолчанию\. Это не то же самое, что включить отключенное предупреждение\. Корректное использование будет следующим:

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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 условия\!

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

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

Предупреждение PVS\-Studio: [V607](https://pvs-studio.ru/ru/docs/warnings/v607/) Ownerless expression 'capturedScreenTexture\-\>view\.pixels'\. MeasureToolOverlayUI\.cpp 35

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

### Unsigned type value < 0

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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 – скачать и попробовать можно по [ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Может, найдёте что\-то интересное и сможете поделиться с нами? :\)

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

Также я рекомендую посетить [страницу](https://github.com/microsoft/PowerToys) проекта на GitHub, может, вы сможете внести что\-то своё в развитие проекта :\) Удачи\!