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!

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:

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. Here you can find the commit which I've checked in this article.

C# part

Late checks

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

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 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

public LauncherViewModel(....)
{
  ....

  if (updatingSettingsConfig == null)
  {
    updatingSettingsConfig = new UpdatingSettings();
  }

  updatingSettingsConfig = UpdatingSettings.LoadSettings();

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

The PVS-Studio warning: 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:

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

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 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 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

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 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

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

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

The PVS-Studio warning: 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. Let's take a look at the code of this method:

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".

C++ part

Resource leak

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 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 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 function (Microsoft carefully reminds us about closing it in the remarks to the OpenProcess function). If we don't do it, we get the resource leak.

Useless check

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 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

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 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:

....
#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

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 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

We even have a separate article about such mistakes: "The most dangerous function in the C/C++ world".

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:

typedef struct tagINPUT {
    DWORD   type;

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

Identical expressions

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 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

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

The PVS-Studio warning: 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

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

The PVS-Studio warning: 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:

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

Suspicious check

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 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

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

The PVS-Studio warning: 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

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 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. 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 on GitHub. You may also contribute to the project development:) Good luck!