In 2023, PVS-Studio developers have written a lot of articles on checking open-source C# projects. As always, we're sharing the top 10 bugs we found this year. Have fun reading!
There are several criteria the code of a project needs to meet to get in the top:
As you can see, there aren't many conditions. However, selecting the best 10 warnings wasn't a trivial task. Yes, we'd like to look at more errors but still decided to follow the tradition and compile the top 10.
You can find the previous compilations of C# bugs here:
Well, let's take a look at the top warnings of 2023!
P.S. Don't take the rankings too seriously. The order is largely based on the author's subjective opinion.
Our top opens with the analyzer warning from the MudBlazor check:
public static bool operator ==(ResizeOptions l, ResizeOptions r)
=> l.Equals(r);
public static bool operator !=(ResizeOptions l, ResizeOptions r)
=> !l.Equals(r);The PVS-Studio warnings:
In the implementations of overloading '==' and '!=', the left operand isn't checked for null. If the value of the l parameter is null, the NullReferenceException exception is thrown when the Equals method is called. The parameter of the Equals method, by the way, is also dereferenced without checking:
public bool Equals(ResizeOptions other)
{
if (ReportRate != other.ReportRate || ....) // <=
{
return false;
}
....
}Such behavior can be a very unpleasant surprise to the user if the class is public. This is exactly what ResizeOptions is.
To make sure the issue exists, let's try to compare an object of the ResizeOptions type to null.
As expected, we get an exception of the NullReferenceException type. If the right operand is null, the behavior is the same.
By the way, the Microsoft documentation mentions that such cases have to be taken into account.
The warning from the Microsoft PowerToys check takes the 9th place.
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:
Please ignore the type of property and return value. I've shortened the code, so the snippet shown here 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.
Another warning from the aforementioned MudBlazor article comes in the 8th place:
internal void DateValueChanged(DateTime? value)
{
_valueDate = value;
if (value != null)
{
var date = value.Value.Date;
// get the time component and add it to the date.
if (_valueTime != null)
{
date.Add(_valueTime.Value); // <=
}
_filterDefinition.Value = date;
}
else
_filterDefinition.Value = value;
_dataGrid.GroupItems();
}The PVS-Studio warning: V3010 The return value of function 'Add' is required to be utilized. Filter.cs 140
The return value of the Add method called on a variable of the DateTime type was not used here. Most likely, the developer wanted to add _valueTime.Value to date. However, they didn't take into account that the Add method for the DateTime object returns the result of the addition and doesn't change the original object. The result is a useless call that was probably meant to have an impact.
We've taken the following warning from the the Ryujinx check:
private void YesButton_Clicked(object sender, EventArgs args)
{
....
Window.Functions = _mainWindow.Window.Functions =
WMFunction.All & WMFunction.Close;
....
}The PVS-Studio warning: V3182 The result of 'WMFunction.All & WMFunction.Close' expression is '0'. It is possible that the '|' operator should be used instead. UpdateDialog.cs 69
As we can see, the analyzer complains about the '&' operator. To understand why, let's take a look at the WMFunction enumeration:
[Flags]
public enum WMFunction
{
All = 0x1,
Resize = 0x2,
Move = 0x4,
Minimize = 0x8,
Maximize = 0x10,
Close = 0x20
}We are dealing with bit flags, but in this case, the implementation of combining them doesn't work: the result of the bitwise AND (&) operation for the WMFunction.All and WMFunction.Close values is zero.
Most likely, we need to replace '&' with '|' for correct method operation.
Moving on. Here is a warning from the check of the AWS SDK for .NET:
private static string GetXamarinInformation()
{
var xamarinDevice = Type.GetType("Xamarin.Forms.Device, Xamarin.Forms.Core");
if (xamarinDevice == null)
{
return null;
}
var runtime = xamarinDevice.GetProperty("RuntimePlatform")
?.GetValue(null)
?.ToString() ?? "";
var idiom = xamarinDevice.GetProperty("Idiom")
?.GetValue(null)
?.ToString() ?? "";
var platform = runtime + idiom;
if (string.IsNullOrEmpty(platform))
{
platform = UnknownPlatform;
}
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", "Xamarin");
}The PVS-Studio warning: V3137 The 'platform' variable is assigned but is not used by the end of the function. InternalSDKUtils.netstandard.cs 70
The last line of the method looks strange. The "Xamarin" string literal is substituted in the "Xamarin_{0}" template using String.Format. The value of the platform variable, which can store the necessary information, is ignored. That's strange.
We can assume that the return statement should look like this:
return string.Format(CultureInfo.InvariantCulture, "Xamarin_{0}", platform);By the way, there's a similar method for getting the Unity game engine information. It's written in a similar pattern, but the return value is produced correctly:
private static string GetUnityInformation()
{
var unityApplication
= Type.GetType("UnityEngine.Application, UnityEngine.CoreModule");
if (unityApplication == null)
{
return null;
}
var platform = unityApplication.GetProperty("platform")
?.GetValue(null)
?.ToString() ?? UnknownPlatform;
return string.Format(CultureInfo.InvariantCulture, "Unity_{0}", platform);
}A warning from the BTCPay Server check opens the second half of the top:
private IActionResult Validate(StoreBaseData request)
{
....
if (request.PaymentTolerance < 0 && request.PaymentTolerance > 100)
ModelState.AddModelError(nameof(request.PaymentTolerance),
"PaymentTolerance can only be between 0 and 100 percent");
....
}The PVS-Studio warning: V3022 Expression 'request.PaymentTolerance < 0 && request.PaymentTolerance > 100' is always false. Probably the '||' operator should be used here. BTCPayServer\Controllers\GreenField\GreenfieldStoresController.cs 241
Many people have faced transaction cancellations due to incorrect data input in financial apps. Sometimes apps just notify users of incorrect data, but sometimes it's all about a silly bug in the code.
So, in this code fragment, the '&&' and '||' operators are mixed up. The code was originally supposed to check for a value between 0 and 100, but the user wouldn't get a warning.
As a result, an incorrect value traverses through the code and can lead to an error in the application logic.
The warning, which is almost as good as the finalists, is in the 4th place. It is taken from the already known MudBlazor check.
internal async Task<bool> StartResizeColumn(....)
{
....
// In case resize mode is column, we have to find any column right
// of the current one that can also be resized and is not hidden.
var nextResizableColumn = _columns.Skip(_....) + 1)
.FirstOrDefault(c =>
c.Resizable ?? true && !c.Hidden); // <=
....
}The PVS-Studio warning: V3177 The 'true' literal belongs to the '&&' operator with a higher priority. It is possible the literal was intended to belong to '??' operator instead. DataGridColumnResizeService.cs 52
Let's look at the c.Resizable ?? true && !c.Hidden expression. It's worth saying that the priority of '??' is lower than that of '&&'. Based on this, if c.Resizable is null, then the null-coalescing operator returns the result of the true &&!c.Hidden operation. The "&&" operation with the true literal is pointless. Most likely, the correct code looks as follows:
(c.Resizable ?? true) && !c.HiddenThe difference between these variants can manifest itself if the values of c.Resizable and c.Hidden are true.
There's another thing indicating that an error was made. A line above has a comment saying that the column must be resizable (the information on this can be found in c.Resizable) and not hidden (you can learn about it in c.Hidden). If c.Resizable is true, then the logic of the operation is broken, since the value of c.Hidden doesn't affect the result in any way.
And finally, we've made it to the top three. It opens with a warning that is taken from the article on checking the top-3 open-source games in C#. The project where we found the error is Barotrauma. It's not easy to find a bug like this with your eyes, so the warning described here is a good example of how powerful static analysis is.
Let's look at the code fragment that contains the error:
public void RecreateSprites()
{
....
for (int i = 0; i < DecorativeSprites.Count; i++)
{
var decorativeSprite = DecorativeSprites[i];
decorativeSprite.Remove();
var source = decorativeSprite.Sprite.SourceElement;
DecorativeSprites[i] = new DecorativeSprite(source, ....);
}
}The PVS-Studio warnings: V3080. Possible null dereference. Consider inspecting 'decorativeSprite.Sprite'. Limb.cs 462.
In this case, the order of operations is wrong, which results in NullReferenceException. It's clear from the implementation of the decorativeSprite.Remove method:
partial class DecorativeSprite : ISerializableEntity
{
....
public Sprite Sprite { get; private set; }
....
public void Remove()
{
Sprite?.Remove();
Sprite = null;
....
}
}The method assigns the null value to the Sprite property.
Looking again at the method in which Remove is called, it's clear that the exception is thrown when accessing decorativeSprite.Sprite.SourceElement. Invoking Remove before the call causes this behavior.
To avoid the exception, we can swap the calls of the Remove method and initializations of the source variable.
The second place, a little behind the winner, goes to the warning from the "PVS-Studio now analyzes Blazor components" article. By the way, the article is titled that way for a reason :). Indeed, PVS-Studio can analyze Blazor components. If you have a Blazor project, we invite you to try the analyzer on it.
Let's get back to the warning. The analyzer issued it for the MudBlazor code.
Look at the suspicious fragment:
@code
{
....
public void Evaluate()
{
....
var exp = new Expression(CalcExpression);
var result = exp.Eval();
if (result == double.NaN)
{
Current = "ERROR";
return;
}
Current = Math.Round( result,8).ToString(CultureInfo.InvariantCulture);
CalcExpression = Current;
}
....
}The PVS-Studio studio: V3076 Comparison of 'result' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead.
According to the analyzer, the comparison with double.NaN is pointless. Why, though? According to MSDN, comparing two values of NaN using the '==' operator always returns false. To compare values correctly, we need to use the double.IsNaN method.
It's such a simple warning, but we think it deserves the second place in the top. The specifics of the NaN comparison may not be entirely clear.
Here is the crown jewel of today's top. Why exactly does this warning take the first place? In our opinion, the issue it shows is the most difficult to identify compared to the other top representatives. We've taken the following warning from the Microsoft PowerToys check.
Try to find the error yourself:
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;
}
}Did you succeed in detecting the issue here? We'd love to read your answers in the comments. Just don't peek at the error description :).
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. However, is this really so? 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. It would seem that the case is closed — the analyzer made a mistake. However, it's not that simple. Let's dig a little deeper.
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 in 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, is written into ind. Here comes the error in the program logic. If the search method doesn't detect the collection item, it returns 0 instead of the intended -1.
In 2023, we've checked quite a few C# projects, but we can't say it's a lot. However, that hasn't been an obstacle to the creation of this top. On the contrary, there are some interesting warnings that we didn't include here because we didn't want to break the Top 10 tradition.
If you'd like to read more about these not-included bugs, we have some worthwhile articles:
You can also compile your own top of warnings or just check a project :). To do this, we invite you to try PVS-Studio.