# Microsoft PowerToys: the GitHub king among C\# projects with C\+\+ errors

Microsoft PowerToys is a robust and useful tool\. It not only simplifies some Windows scenarios but also creates new ones\. It ranks as top\-rated among C\# projects on GitHub\. Let's see how well its developers write code and find out what C\+\+ is doing here\!

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

I'll make a brief reminder for those who are not familiar with this wonderful tool\. Microsoft PowerToys is a set of utilities to tune and streamline Windows experience for greater productivity\. I use this tool in my work too\. For me, the most useful is the 'What's using this file?' utility\. It's convenient to immediately terminate the process that interacts with a file when you need to delete or rename that file\. 

The project is open source and was created by a dev community together with Microsoft\. At the moment of writing this article, the project has 95,000 stars on GitHub, making it a rightful king among C\# projects\. What's this got to do with C\+\+? Currently, the project code base is divided as follows: the C\# part of code is 60%, the C\+\+ part of code is almost 40%\.

We expect the PowerToys developers to write high\-quality code\. Let's check if it's true\. Arm ourselves with the PVS\-Studio static analyzer and see what interesting things we can find\. So, we're doing a cross\-language code analysis here\. We don't write such articles very often\. I've used the PVS\-Studio plugin for Visual Studio 2022 to analyze the source code\. With a simple mouse click, we can analyze the project and review the report:

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

For your convenience, I've divided the errors into two blocks: one for C\# and one for C\+\+\. This way, all readers can easily find their programming language\. Feel free to read about another language as well — I've tried to describe the error in a way that everyone can understand\.

The PowerToys source code is available at the [link](https://github.com/microsoft/PowerToys)\. Here you can find the [commit](https://github.com/microsoft/PowerToys/commit/83f062542778a14d5dd260d0018d3db1f6c84af6) which I've checked in this article\.

## C\# part

### Late checks

```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>();
  }
}
```

The PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'query' object was used before it was verified against null\. Check lines: 56, 60\. Main\.cs 56

As we can see from the code snippet, the _query _parameter is checked against _null_\. If _query _is _null_, _ArgumentNullException _is thrown\. However, before this check, the developers used _query _when accessing the _ActionKeyword _property\. This means that if _query _is _null_, the execution flow won't reach the check because _NullReferenceException_ will be thrown\.

```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))
  {
    ....
  }
}
```

The PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The '\_viewModel\.Results' object was used before it was verified against null\. Check lines: 542, 563\. MainWindow\.xaml\.cs 542

Here is another late check\. The _\_viewModel\.Results_ property is used first, and checked for _null_ afterward\. This code may be the result of rushed refactoring\.

### No settings — take NRE

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

  updatingSettingsConfig = UpdatingSettings.LoadSettings();

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

The PVS\-Studio warning: [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'updatingSettingsConfig'\. LauncherViewModel\.cs 138

The analyzer recommends to check _updatingSettingsConfig\._ The code clearly shows that _updateSettingsConfig_ is checked for _null_\. If the variable value is _null_, a reference to the new object is written into it\. After that, the result of the _LoadSettings _method is written to _updatingSettingsConfig_ without condition\. Then, the question arises, "Why was the previous code needed?"\.

Let's take a look at the code of the _LoadSettings_ method:

```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;
}
```

The method can return _null _if the settings file doesn't exist, or if an exception is encountered while working with the file\. If the method returns _null_, _NullReferenceException_ will be thrown\.

### Unexpected mistake

```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;
  }
}
```

The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'ind' is always not null\. The operator '??' is excessive\. StringMatcher\.cs 230

The analyzer thinks that the operator '??' is needless because _ind _is always _non\-null_\. Let's see if that's the case\. _Ind _is nullable, which means it can be _null_\. The developers have used the _FirstOrDefault _method, which may return _null_, to write the value into _Ind_\. Is that all? Is the case closed? Has the analyzer been wrong? There's a reason this code is here :\)\.

In fact_, FirstOrDefault _returns _default\(TSource \) _instead of _null_, and _default\(int?\)_ is _null_\. However, _TSource _turns into _int _here because TSource is the type of elements of the enumerated sequence\. In this case, the developers apply LINQ to the _spaceIndices _parameter with the _List<int\>_ type\. This means that _default\(int\)_, i\.e\. 0, will be written into _ind_\. Here comes the error in the program logic\. If the search method doesn't detect the element of collection, it'll return 0 instead of the intended \-1\.

### Weird shifts

```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;
    }
    ....
  }
}
```

The PVS\-Studio warnings:

* [V3134](https://pvs-studio.com/en/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.com/en/docs/warnings/v3134/) Shift by 32 bits is greater than the size of 'UInt32' type of expression '\(uint\)fileVersion\.ProductMinorPart'\. PluginManager\.cs 63

Don't pay attention to the type of property and the type of return value\. The code had to be shortened a lot, so I'll show you the snippet that refers to the lambda expression\. The analyzer issues two warnings for this code\. The _uint _type has a size of 32 bits\. It turns out that the first 48\-bit shift is equivalent to the 16\-bit shift\. The second 32\-bit shift is equivalent to the 0\-bit shift\. It's hard to tell what the developers wanted to do here, but they might have used _ulong _instead of _uint_\.

### How not to use the '?\.' operator

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

The PVS\-Studio warning: [V3105](https://pvs-studio.com/en/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

In this case, the result of the null\-conditional operator '?\.' is passed to the _AddRange _method\. This is the custom extension method for _ICollection<T\>_\. Let's take a look at the code of this method:

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

The _items _parameter, which can be _null_, is used in _foreach_\. As we remember, _foreach _doesn't protect us from _NullReferenceException_\. We remember, don't we? For many programmers, this behavior isn't obvious\. Our team even wrote an article about it: "[The ?\. operator in foreach will not protect from NullReferenceException](https://pvs-studio.com/en/blog/posts/csharp/0832/)"\.

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

## C\+\+ part

### Resource leak

```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);
    }
  }
}
```

The PVS\-Studio warning: [V773](https://pvs-studio.com/en/docs/warnings/v773/) Visibility scope of the 'proc' handle was exited without releasing the resource\. A resource leak is possible\. settings\_window\.cpp 635

Nobody likes to see resources leaking out in an application\. In this case, the developers use the [_OpenProcess_](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocess) function to get the descriptor of the specified process, after which the process is terminated\. When we finish working with the descriptor, we need to close it with the [_CloseHandle_](https://learn.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-closehandle) function \(Microsoft carefully reminds us about closing it in the [remarks](https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-openprocess#remarks) to the _OpenProcess_ function\)\. If we don't do it, we get the resource leak\.

### Useless check

```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);
      }
    }
  }
  ....
}
```

The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression is always true\. Settings\.cpp 241

In this code snippet, the developers have written a condition that is always _true_\. The value of the _val_ variable is greater than or equal to 0 or less than the _EnumElements_ constant of the _OverlappingZonesAlgorithm_ enumeration, which has a value of 4\. This means that any value that can be in _val_ fits the condition\. Most likely, the developers should have used the '&&' operator instead of '\|\|'\.

### Unreachable code 

```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;
  }
  ....
}
```

The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'placement\.showCmd \=\= 3' is always false\. WindowUtils\.cpp 336

Another error is with boolean expressions, but in this case the condition is always _false_\. The analyzer issues the warning for the second _if_\. The developers use macros in the code\. Let's take a look at the definition of the macros that we need:

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

The analyzer suggests that the condition is always _false_, so let's go the other way around\. To make the second _if _statement _true_, the value of the _showCmd _field should be 3\. Let's take a look at the first _if_\. If the value of the _showCmd _field is neither 2 nor 6, we assign it to 9\. It's clear that 3 is unequal to 2 and 6\. It turns out that the analyzer is triggered correctly, and the condition is always _false_\. Moreover, the code inside will never be executed\.

### Most dangerous function in the C and C\+\+ worlds

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

PVS\-Studio issues three warnings for this code at once:

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

We even have a separate article about such mistakes: "[The most dangerous function in the C/C\+\+ world](https://pvs-studio.com/en/blog/posts/cpp/0360/)"\.

In this case, the developers wanted to fill the _keyEventList_ array with zeros\. Pay attention to the third parameter — the number of bytes the developers wanted to fill with zeros\. In this case, _sizeof\(keyEventList\)_ evaluates the pointer size instead of the array size\. It depends on the target platform, but most often it's 4 or 8 bytes\. However, the size of the structure is clearly larger than 4 or 8 bytes:

```cpp
typedef struct tagINPUT {
    DWORD   type;

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

### Identical expressions

```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)
  {
    ....
  }
}
```

The PVS\-Studio warning: [V501](https://pvs-studio.com/en/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

We immediately understand everything when we see the analyzer message\. The _m\_reparent\_event\_handle_ sub\-expression is used to the left and right of the '\|\|' operator\. But _m\_thumbnail\_event\_handle_ isn't used\. It's a simple but such a frustrating mistake :\)\.

### Garbage value

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

The PVS\-Studio warning: [V573](https://pvs-studio.com/en/docs/warnings/v573/) Uninitialized variable 'dpi' was used\. The variable was used to initialize itself\. Toolbar\.cpp 112

The code uses a newly declared uninitialized variable to determine its value\. As a result, the _dpi_ variable will be initialized with a random value\.

### Incorrect use of \#pragma warning

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

The PVS\-Studio warning: [V665](https://pvs-studio.com/en/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

The analyzer detected incorrect use of the _\#pragma warning _directives\. In this case, the developers wanted to disable the warning and then enable it\. However, the _pragma warning\(default : X\) _directive sets the warning with the _X_ number to the default state\. This is not the same as enabling the disabled warning\. The correct code is as follows:

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

### Suspicious check

```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 ....
}
```

The PVS\-Studio warning: [V517](https://pvs-studio.com/en/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

I quite often encounter this kind of error\. The developers used the _subtype \=\= MFVideoFormat\_YUY2 expression_ twice in the _if_ statement\. This may be just an unnecessary check\. However, sometimes developers just copy\-paste such code, and may forget to check something\. Especially, when the code has a sequence of 61 conditions\!

### Ownerless expression

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

The PVS\-Studio warning: [V607](https://pvs-studio.com/en/docs/warnings/v607/) Ownerless expression 'capturedScreenTexture\-\>view\.pixels'\. MeasureToolOverlayUI\.cpp 35

Here is a redundant expression in the code\. It's hard to say whether the developers should just remove this expression, or there is an error in the program execution\. Such code is often the result of refactoring\. The code is compiled, but it doesn't make sense\.

### 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;
  }
  ....
}
```

The PVS\-Studio warning: [V547](https://pvs-studio.com/en/docs/warnings/v547/) Expression 'token\_size < 0' is always false\. Unsigned type value is never < 0\. NtdllExtensions\.cpp 314

The _token\_size_ variable is checked for being less than 0\. But there's a small problem here\. The variable takes the _DWORD_ type, and this type is unsigned\. It turns out that the condition is always _false_\.

## Conclusion

The developers of Microsoft PowerToys created a tool that many \(including me\) actively use\. It was curious experience to watch what PVS\-Studio found in the code\. I'd prefer not to describe the same errors several times or write about dull bugs\. You can always check any project yourself with PVS\-Studio — download and try it [here](https://pvs-studio.com/en/pvs-studio/try-free/)\. After you find something interesting, maybe you can share it with us, can't you? :\)

I'm going to continue to use PowerToys in my work and look forward to updates and new utilities\.

I'd also recommend visiting the [project page](https://github.com/microsoft/PowerToys) on GitHub\. You may also contribute to the project development:\) Good luck\!