Our website uses cookies to enhance your browsing experience.
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.
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:
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. We analyzed the following commit 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. 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!
Issue 1
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 [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 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
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 [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
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 [CWE-563] The 'trackRingThickness' variable is assigned but is not used by the end of the function. Files.App.Controls StorageRing.cs 211
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
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 [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
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 [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".
Issue 6
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 [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 [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
public AccessControlEntry(bool isFolder,
string ownerSid,
AccessControlEntryType type,
AccessMaskFlags accessMaskFlags,
bool isInherited,
AccessControlEntryFlags inheritanceFlags)
{
AccessMaskItems = SecurityAdvancedAccessControlItemFactory
.Initialize(this,
AreAdvancedPermissionsShown,
IsInherited,
IsFolder);
ChangeAccessControlTypeCommand = new RelayCommand<string>(x =>
{
//AccessControlType = Enum.Parse<AccessControlType>(x);
});
ChangeInheritanceFlagsCommand = new RelayCommand<string>(x =>
{
//var parts = x.Split(',');
//InheritanceFlags = Enum.Parse<AccessControlEntryFlags>(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:
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
protected override async IAsyncEnumerable<ICloudProvider> 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 [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. Thus, the Add method is an instance method for Dictionary<TKey, TValue>. 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
public void Log<TState>(....)
{
if (exception is null ||
exception.Data[Mechanism.HandledKey] is false ||
logLevel <= LogLevel.Information)
return;
var generalSettingsService =
Ioc.Default.GetRequiredService<IGeneralSettingsService>();
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 [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:
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
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 [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
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 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:
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:
// 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.
case displays the item count and processed size in the progress.case displays the processed size in the progress.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.
case uses only the ProcessedSize property.case uses only the ProcessedSize property.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!
Picture 4. Source: the TV series "Sherlock"
Issue 12
public bool LoadTagsList
=> SelectedItem?.HasTags ?? false &&
PreviewPaneState is PreviewPaneStates.NoPreviewAvailable ||
PreviewPaneState is PreviewPaneStates.PreviewAndDetailsAvailable;The PVS-Studio warning: 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
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 [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
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 [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
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 [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.
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.
Happy coding!
0
0
0
36
0
0