# Files file manager: folder full of bugs Files is a third\-party file manager for Windows\. Its goal is to become the best open\-source OS file manager maintained by the community\. In this article, we break down some bugs in Files source code and contribute to the open\-source community\. ![1295_Files/image1.png](https://import.viva64.com/docx/blog/1295_Files/image1.png) Before we dive right into the project analysis, why don't we take a closer look at it? At the time of the check, Files has over 39,000 stars on GitHub—a significant achievement\. Files focuses on user convenience, file and organization management, and rich functionality with deep integrations\. Here's an example of the interface from the project [website](https://files.community/): ![1295_Files/image2.png](https://import.viva64.com/docx/blog/1295_Files/image2.png) As I think, it features a more modern design compared to the Windows 10 and 11 File Explorer\. Additionally, Files supports command palette, Git integration, convenient cloud storage integrations, and much more\. We can customize it to our liking by changing the design, reassigning keyboard shortcuts, and creating custom commands\. I hadn't used Files before, but after exploring it, I'll definitely give this project a chance, though it's hard to break the habit of automatically using the built\-in Windows File Explorer after so many years\. Now, let's move on to digging into the source code, which is available at the [link](https://github.com/files-community/Files)\. We analyzed the following [commit](https://github.com/files-community/Files/commit/a6b39a9fca99436b3e39d30d026fc6139459248f) using PVS\-Studio static analyzer\. This tool can analyze code in C, C\+\+, C\#, and Java\. Files happens to be written in C\#\. By the way, this project has already migrated from \.sln to the \.slnx solution format\. If you're curious about the advantages of the new format or haven't heard of \.slnx, I invite you to read this [article](https://pvs-studio.com/en/blog/posts/csharp/1265/)\. PVS\-Studio already supports the new solution format, so this doesn't pose any issues\. Naturally, we won't cover every issue the analyzer found\. Reviewing dozens of similar warnings in a single article isn't very engaging\. PVS\-Studio issued about 250 warnings in total\. As mentioned, we'll contribute to the open\-source community by reporting potential problems we found to the developers\. Let's get started\! ## Errors in code **Issue 1** ```cpp 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; .... } ``` The PVS\-Studio warning: [V3207](https://pvs-studio.com/en/docs/warnings/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 We'll start with an interesting yet simple mistake\. Let's look closer at the `itemCount is not 0 or 1` part\. Already guessed what's the issue? This pattern is redundant, its second part effects nothing\. When saying "x is not 0 or 1" people usually imply that x is neither 0 nor 1\. However, in C\#, operator precedence works differently—`x is not 0 or 1` actually means `x is (not 0) or 1`\. Such mistakes can lead not only to redundancy but also to errors like NullReferenceException: `list is not null or list.Count == 0`\. This issue was even discussed in a [meeting](https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-11-04.md#not-and-or-patterns) that explored potential solutions\. Based on that discussion, in the future such cases will likely be caught by the compiler or built\-in static analysis as a warning\. **Issue 2** ```cpp private void SetPathData(string pathData, FrameworkElement element) { .... if (GetTemplateChild(LayerPathPart) is Path path) { path.Data = geometry; path.Width = element.Width; path.Width = element.Height; } } ``` The PVS\-Studio warning: [V3127](https://pvs-studio.com/en/docs/warnings/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 A common copy\-paste error\. Instead of assigning to `path.Height`, the assignment to `path.Width` occurs twice\. **Issue 3** ```cpp 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; } ``` The PVS\-Studio warnings: [V3137](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3137/) \[CWE\-563\] The 'valueRingThickness' variable is assigned but is not used by the end of the function\. Files\.App\.Controls StorageRing\.cs 213 The analyzer detected two local variables unused after the assignment\. Values from the `_valueRingThickness` and `_trackRingThickness` fields are written to the `trackRingThickness` and `valueRingThickness` local variables\. These variables are then modified conditionally but never used later in the method\. Quite likely, the developers intended to use local variables in subsequent operations\. **Issue 4** ```cpp 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; } ``` The PVS\-Studio warning: [V3042](https://pvs-studio.com/en/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'context' object Files\.App OpenClassicPropertiesAction\.cs 40 A familiar error for C\# developers—NullReferenceException\. In the `if` condition, `context` is dereferenced without a null check, even though later both in this condition and in the `else if` statement there is a check that uses `?.`\. **Issue 5** ```cpp public async Task ExecuteAsync(object? parameter = null) { if (context.SelectedItems.Count > 0) { foreach (ListedItem listedItem in context.ShellPage?.SlimContentPage.SelectedItems) { .... } } } ``` The PVS\-Studio warning: [V3153](https://pvs-studio.com/en/docs/warnings/v3153/) \[CWE\-476\] Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Files\.App UnpinFromStartAction\.cs 32 Another potential NRE\. In the `foreach` loop, the enumerated collection is obtained using the `?.` operator\. Not all developers know that `foreach` doesn't handle `null`\. This type of error occurs frequently enough that we've written an article covering it: "[The ?\. operator in foreach will not protect from NullReferenceException](https://pvs-studio.com/en/blog/posts/csharp/0832/)"\. ![1295_Files/image3.png](https://import.viva64.com/docx/blog/1295_Files/image3.png) **Issue 6** ```cpp 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; } ``` The PVS\-Studio warning: [V3022](https://pvs-studio.com/en/docs/warnings/v3022/) \[CWE\-571\] Expression is always true\. Probably the '&&' operator should be used here\. Files\.App App\.xaml\.cs 181 The analyzer detected the `if` condition that is always `true`\. The `args.WindowActivationState` property is compared with enumeration elements\. The `||` operator is used instead of `&&`, so the condition checks whether the value from the `WindowActivationState` property is not equal either to `WindowActivationState.CodeActivated` or `WindowActivationState.PointerActivated`\. The value is always not equal to one of them\. This also results in unreachable code after `if`: [V3142](https://pvs-studio.com/en/docs/warnings/v3142/) \[CWE\-561\] Unreachable code detected\. It is possible that an error is present\. Files\.App App\.xaml\.cs 185 By the way, note the comment asking, "Is this code still needed?"\. It wasn't needed in the past and won't be in the future, as it contains a bug\. If the developers wanted to create unreachable code, they could simply use an unconditional return\. **Issue 7** ```cpp public AccessControlEntry(bool isFolder, string ownerSid, AccessControlEntryType type, AccessMaskFlags accessMaskFlags, bool isInherited, AccessControlEntryFlags inheritanceFlags) { AccessMaskItems = SecurityAdvancedAccessControlItemFactory .Initialize(this, AreAdvancedPermissionsShown, IsInherited, IsFolder); ChangeAccessControlTypeCommand = new RelayCommand(x => { //AccessControlType = Enum.Parse(x); }); ChangeInheritanceFlagsCommand = new RelayCommand(x => { //var parts = x.Split(','); //InheritanceFlags = Enum.Parse(parts[0]); }); IsFolder = isFolder; Principal = new(ownerSid); AccessControlType = type; AccessMaskFlags = accessMaskFlags; IsInherited = isInherited; AccessControlEntryFlags = inheritanceFlags; .... } ``` See the bug? Actually, we've got a couple of them here: * [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) \[CWE\-665\] The 'IsFolder' property is used before it is initialized in constructor\. Files\.App AccessControlEntry\.cs 337 * [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) \[CWE\-665\] The 'IsInherited' property is used before it is initialized in constructor\. Files\.App AccessControlEntry\.cs 337 The `IsFolder` and `IsInherited` properties are used here before they are initialized in the same constructor with their corresponding arguments\. So, `IsFolder` and `IsInherited` are always passed to the `SecurityAdvancedAccessControlItemFactory.Initialize` method with their default `false` value\. **Issue 8** ```cpp protected override async IAsyncEnumerable 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); } .... } ``` The PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/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 The analyzer knows which library methods can accept `null` arguments and which cannot\. We call this method annotation\. Out of the box, the analyzer contains annotations for system libraries, Unity, ASP\.NET Core, and more\. You can read more about static analysis technologies in this [article](https://pvs-studio.com/en/blog/posts/0908/)\. Thus, the `Add` method is an instance method for `Dictionary`\. Passing `null` as the first argument \(the dictionary key\) throws an `ArgumentNullException`\. The first argument here is an expression that uses `?.`\. Even though I can't assess the likelihood of this scenario, an error remains an error\. **Issue 9** ```cpp public void Log(....) { if (exception is null || exception.Data[Mechanism.HandledKey] is false || logLevel <= LogLevel.Information) return; var generalSettingsService = Ioc.Default.GetRequiredService(); 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 }; } ``` The PVS\-Studio warning: [V3202](https://pvs-studio.com/en/docs/warnings/v3202/) \[CWE\-561\] Unreachable code detected\. The 'case' value is out of range of the match expression\. Files\.App SentryLogger\.cs 35 More unreachable code\. The `switch` statement matches the `logLevel` value and `LogLevel` enumeration elements\. However, note the check before the `switch` statement: if `logLevel <= LogLevel.Information`, then `return`\. Let's look at the `LogLevel` enumeration definition: ```cpp public enum LogLevel { Trace = 0, Debug = 1, Information = 2, .... } ``` Thus, due to the condition before switch, `Trace`, `Debug`, and `Information` don't reach the `switch` statement\. Yet we see `Debug` and `Information` in the first two `switch` cases\. **Issue 10** ```cpp private async void Omnibar_QuerySubmitted(....) { .... if (action is not null) { var overload = action.GetOverloads().FirstOrDefault(); await overload?.InvokeAsync(actionInstance.Context); } .... } ``` The PVS\-Studio warning: [V3168](https://pvs-studio.com/en/docs/warnings/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 Another potential NRE, this time not from `foreach` or a method that doesn't handle `null`\. Here, the `await` operator is used with an expression that might be `null`\. The analyzer infers this possibility due to the `?.` operator\. **Issue 11** ```cpp 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: .... } .... } ``` The PVS\-Studio warning: [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. Files\.App StatusCenterItem\.cs 342 In the `switch` statement, the analyzer detected identical code snippets in the first two `case` expressions\. These `case` snippets are quite substantial, making it unlikely that they were intentionally duplicated rather than merged: ```cpp case (not 0, not 0): case (not 0, _): ``` They were probably meant to differ slightly\. But how? Let's read the comments for each `case`: ```cpp // 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: .... } ``` This is what we find out from the comments\. 1. The first `case` displays the item count and processed size in the progress\. 1. The second `case` displays the processed size in the progress\. 1. The third `case` displays the processed size in the progress\. It's challenging to spot errors in the project you didn't develop, but we can notice the following pattern\. 1. The first `case` uses only the `ProcessedSize` property\. 1. The second `case` uses only the `ProcessedSize` property\. 1. The third `case` uses only the `ProcessedItemsCount` property\. Thus, the first `case` lacks the necessary logic to display `ProcessedItemsCount`\. Congrats, we've got to the crux of the problem\! ![1295_Files/image4.png](https://import.viva64.com/docx/blog/1295_Files/image4.png) Picture 4\. Source: the TV series "Sherlock" **Issue 12** ```cpp public bool LoadTagsList => SelectedItem?.HasTags ?? false && PreviewPaneState is PreviewPaneStates.NoPreviewAvailable || PreviewPaneState is PreviewPaneStates.PreviewAndDetailsAvailable; ``` The PVS\-Studio warning: [V3177](https://pvs-studio.com/en/docs/warnings/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 Another non\-obvious mistake\. This one revolves around operator precedence\. The `??` operator has lower precedence than `&&`, therefore `false && PreviewPaneState is PreviewPaneStates.NoPreviewAvailable` is checked first, always returning `false`\. This doesn't seem like the developer's intention\. **Issue 13** ```cpp 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; .... } ``` The PVS\-Studio warning: [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) \[CWE\-476\] The 'input' object was used before it was verified against null\. Check lines: 375, 376\. Files\.App FileProperty\.cs 375 The analyzer flags that `input` is initially used without a `null` check, and then later with a check\. It's not the first time I've seen an object without a `null` check before calling `GetType()`\. Perhaps, some developers think it works as an extension method, but it doesn't\. This call results in a `NullReferenceException`\. **Issue 14** ```cpp 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; .... } ``` The PVS\-Studio warning: [V3105](https://pvs-studio.com/en/docs/warnings/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 A simple demonstration of interprocedural analysis—the analyzer detected that the `CloseUnnecessaryColumns` method receives the `shPage?.ColumnParams` value, which might be `null`\. Inside `CloseUnnecessaryColumns`, the parameter is immediately dereferenced, potentially causing an NRE\. And the `if` condition doesn't prevent this issue\. The condition is `true` when `shPage` is `null` anyway\. **Issue 15** ```cpp 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; .... } ``` The PVS\-Studio warning: [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) \[CWE\-476\] The 'FolderSettings' object was used after it was verified against null\. Check lines: 174, 148\. Files\.App DetailsLayoutPage\.xaml\.cs 174 This problem resembles others we've examined\. The `FolderSettings` property is checked for `null` in the `if` statement and is further used within the then\-branch\. However, after `if`, it's used without any check\. ## Conclusion We've reviewed some bugs found in Files\. Needless to say, the developers are creating a truly impressive product that inspires usage\. If you're curious about what else PVS\-Studio has discovered in this project, or want to analyze your own project, you're welcome to download and try the analyzer at this [link](https://pvs-studio.com/en/pvs-studio/try-free/)\. Happy coding\!