In 2022, the PVS-Studio developers wrote lots of articles where they described bugs found in open-source projects. Now it's time to choose the most interesting ones.
How was this top created?
I looked through all C# articles published this year and inspected all the bugs described there. To make this top more diverse, I used the following criteria:
- one project — one error;
- errors should be different from each other.
Now let's see what we've got.
10th place. Attempt to unsubscribe in Stride
A classic analyzer warning takes the tenth place today. This error was found during the Stride Game Engine project check:
private void RemoveEditor(....)
{
....
if (....)
{
multiEditor.OpenedAssets.CollectionChanged -= (_, e) =>
MultiEditorOpenAssetsChanged(multiEditor, e);
....
}
....
}
V3084. Anonymous function is used to unsubscribe from 'CollectionChanged' event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. AssetEditorsManager.cs 444
Judging by the analyzer warning, the code doesn't unsubscribe from the event. Will it cause trouble? It can!
9th place. Ambiguous assignment in Bitwarden
The ninth place goes to an error from the Bitwarden project check. Let's look at it:
public class BillingInvoice
{
public BillingInvoice(Invoice inv)
{
Amount = inv.AmountDue / 100M; // <=
Date = inv.Created;
Url = inv.HostedInvoiceUrl;
PdfUrl = inv.InvoicePdf;
Number = inv.Number;
Paid = inv.Paid;
Amount = inv.Total / 100M; // <=
}
public decimal Amount { get; set; }
public DateTime? Date { get; set; }
public string Url { get; set; }
public string PdfUrl { get; set; }
public string Number { get; set; }
public bool Paid { get; set; }
}
V3008. The 'Amount' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 148, 142. BillingInfo.cs 148
At the time of writing the article, we could only guess what exactly the developers wanted. Either the first assignment is redundant, or vise versa — the second assignment is superfluous. However, after the article was published, we contacted the developers and reported the issues we found. The developers fixed them — in this case, they just removed the first assignment.
8th place. An element from the void in Barotrauma
The 8th place goes to the error we described in the article about the Barotrauma check:
public ParticlePrefab(XElement element, ContentFile file)
{
....
if (CollisionRadius <= 0.0f)
CollisionRadius = Sprites.Count > 0 ? 1 :
Sprites[0].SourceRect.Width / 2.0f;
}
V3106 Possibly index is out of bound. The '0' index is pointing beyond 'Sprites' bound. ParticlePrefab.cs 303
Accessing the first element of an empty collection is clearly something strange :). We can only hope that the exception thrown doesn't affect the gameplay.
7th place. Initializing fields in AvalonStudio
The seventh place goes to well-hidden error we found when checking AvalonStudio:
public class ColorScheme
{
private static List<ColorScheme> s_colorSchemes =
new List<ColorScheme>();
private static Dictionary<string, ColorScheme> s_colorSchemeIDs =
new Dictionary<string, ColorScheme>();
private static readonly ColorScheme DefaultColorScheme =
ColorScheme.SolarizedLight;
....
public static readonly ColorScheme SolarizedLight = new ColorScheme
{
....
};
}
Oh, seems like this time I won't show the analyzer warnings right away :) Just so you can feel what it's like to search for such issues without static analysis. This error is simple though, so I'm sure you'll handle it.
What's the matter here?
The analyzer issues the following warning:
V3070. Uninitialized variable 'SolarizedLight' is used when initializing the 'DefaultColorScheme' variable. ColorScheme.cs 32
Indeed, there's something wrong with the field initialization.
6th place. Confusion in .NET 7
Yes, we found this error in the famous .NET 7 — here's an article about other errors we found there. Nobody's perfect :).
First, let's look at the* XmlConfigurationElementTextContent* constructor:
public XmlConfigurationElementTextContent(string textContent,
int? linePosition,
int? lineNumber)
{ .... }
Now let's see where it's used:
public static IDictionary<string, string?> Read(....)
{
....
case XmlNodeType.EndElement:
....
var lineInfo = reader as IXmlLineInfo;
var lineNumber = lineInfo?.LineNumber;
var linePosition = lineInfo?.LinePosition;
parent.TextContent =
new XmlConfigurationElementTextContent(string.Empty,
lineNumber,
linePosition);
....
break;
....
case XmlNodeType.Text:
....
var lineInfo = reader as IXmlLineInfo;
var lineNumber = lineInfo?.LineNumber;
var linePosition = lineInfo?.LinePosition;
XmlConfigurationElement parent = currentPath.Peek();
parent.TextContent =
new XmlConfigurationElementTextContent(reader.Value,
lineNumber,
linePosition);
....
break;
....
}
Pay attention to the order of arguments and parameters:
- arguments: ..., lineNumber, linePosition;
- parameters: ..., linePosition, lineNumber.
That's exactly what the analyzer detected:
- V3066 Possible incorrect order of arguments passed to 'XmlConfigurationElementTextContent' constructor: 'lineNumber' and 'linePosition'. XmlStreamConfigurationProvider.cs 133
- V3066 Possible incorrect order of arguments passed to 'XmlConfigurationElementTextContent' constructor: 'lineNumber' and 'linePosition'. XmlStreamConfigurationProvider.cs 148
My teammate created an issue on GitHub: the developers fixed the order of arguments and added a test.
5th place. Orleans: don't rush with clearing
The error from the Orleans project check opens the second half of our top:
private class BatchOperation
{
private readonly List<TableTransactionAction> batchOperation;
....
public async Task Flush()
{
if (batchOperation.Count > 0)
{
try
{
....
batchOperation.Clear(); // <=
keyIndex = -1;
if (logger.IsEnabled(LogLevel.Trace))
{
for (int i = 0; i < batchOperation.Count; i++) // <=
{
logger.LogTrace(....)
}
}
}
catch (Exception ex)
{
....
}
}
}
}
V3116. Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. AzureTableTransactionalStateStorage.cs 345
Something tells me that the developers should have cleared batchOperation a bit later. Judging by the fix, the Orleans developers agree with me.
4th place. Shadow features of Eto.Forms
An error described in the article about Eto.Forms, the GUI framework almost made it to the top 3 errors:
public NSShadow TextHighlightShadow
{
get
{
if (textHighlightShadow == null)
{
textHighlightShadow = new NSShadow();
textHighlightShadow.ShadowColor = NSColor.FromDeviceWhite(0F, 0.5F);
textHighlightShadow.ShadowOffset = new CGSize(0F, -1.0F);
textHighlightShadow.ShadowBlurRadius = 2F;
}
return textHighlightShadow;
}
set { textShadow = value; } // <=
}
public NSShadow TextShadow
{
get
{
if (textShadow == null)
{
textShadow = new NSShadow();
textShadow.ShadowColor = NSColor.FromDeviceWhite(1F, 0.5F);
textShadow.ShadowOffset = new CGSize(0F, -1.0F);
textShadow.ShadowBlurRadius = 0F;
}
return textShadow;
}
set { textShadow = value; }
}
PVS-Studio warning: V3140 Property accessors use different backing fields. Eto.Mac64 MacImageAndTextCell.cs 162
Copy-paste says hello :).
Perhaps, assigning a value to the TextHighlightShadow property should somehow affect textShadow. But even so, it's not clear why it doesn't affect the textHighlightShadow field used in the get accessor.
3d place. Lost migration in Squidex
The bronze goes to an error we described in the article about checking the ASP.NET Core projects. This error was found in Squidex:
private IEnumerable<IMigration?> ResolveMigrators(int version)
{
yield return serviceProvider.GetRequiredService<StopEventConsumers>();
// Version 06: Convert Event store. Must always be executed first.
if (version < 6)
{
yield return serviceProvider.GetRequiredService<ConvertEventStore>();
}
// Version 22: Integrate Domain Id.
if (version < 22)
{
yield return serviceProvider
.GetRequiredService<AddAppIdToEventStream>();
}
// Version 07: Introduces AppId for backups.
else if (version < 7)
{
yield return serviceProvider
.GetRequiredService<ConvertEventStoreAppId>();
}
// Version 05: Fixes the broken command architecture and requires a
// rebuild of all snapshots.
if (version < 5)
{
yield return serviceProvider.GetRequiredService<RebuildSnapshots>();
}
else
{
// Version 09: Grain indexes.
if (version < 9)
{
yield return serviceProvider.GetService<ConvertOldSnapshotStores>();
}
....
}
// Version 13: Json refactoring
if (version < 13)
{
yield return serviceProvider
.GetRequiredService<ConvertRuleEventsJson>();
}
yield return serviceProvider.GetRequiredService<StartEventConsumers>();
}
Good luck finding the error in the code :).
Or you can look at the shortened fragment and try to find it again:
// Version 22: Integrate Domain Id.
if (version < 22)
{
yield return serviceProvider.GetRequiredService<AddAppIdToEventStream>();
}
// Version 07: Introduces AppId for backups.
else if (version < 7)
{
yield return serviceProvider
.GetRequiredService<ConvertEventStoreAppId>();
}
I'm sure it's way easier to search for it now. Oh, if only there were a button that would show only the code fragments related to errors... Wait, this button exists!
Clicking on this miracle button helped discover this issue:
V3022. Expression 'version < 7' is always false. MigrationPath.cs 55
Indeed, the version < 7 condition is checked only if version >= 22. Strange, isn't it?
My IDE doesn't have that button
If you want to get this button, download the extension here.
If your IDE is not supported, you can contact us and express your interest in the plugin with the button for it.
2nd place. Funny shifts in Discord.NET
This time the silver goes to an error we found when checking the Discord.NET project:
public enum GuildFeature : long
{
None = 0,
AnimatedBanner = 1 << 0,
AnimatedIcon = 1 << 1,
Banner = 1 << 2,
....
TextInVoiceEnabled = 1 << 32,
ThreadsEnabled = 1 << 33,
ThreadsEnabledTesting = 1 << 34,
....
VIPRegions = 1 << 40,
WelcomeScreenEnabled = 1 << 41,
}
Just like the previous case, I'll have a pleasure to let you find this error first. Or you can click on the miracle button:
V3134. Shift by 32 bits is greater than the size of 'Int32' type of expression '1'. GuildFeature.cs 147
The problem is, the type of shift operands here is int, so, the result will have the same type. In this case, a value of the 1 << 32 type is 1, the value of 1 << 33 is the same as 1 << 1, and so on.
So, it leads to a weird situation: the AnimatedBanner and TextInVoiceEnabled elements are the same — just like the AnimatedIcon-ThreadsEnabled pair, etc. Besides, the internal type of enumeration is long, and now it becomes clear that the developer did not expect this behavior at all.
1st place. Vulnerability from BlogEngine.NET
And we finally get to the most interesting bug described in 2022. Please welcome — the XXE vulnerability from BlogEngine.NET:
public XMLRPCRequest(HttpContext input)
{
var inputXml = ParseRequest(input);
// LogMetaWeblogCall(inputXml);
this.LoadXmlRequest(inputXml); // Loads Method Call
// and Associated Variables
}
private static string ParseRequest(HttpContext context)
{
var buffer = new byte[context.Request.InputStream.Length];
context.Request.InputStream.Position = 0;
context.Request.InputStream.Read(buffer, 0, buffer.Length);
return Encoding.UTF8.GetString(buffer);
}
private void LoadXmlRequest(string xml)
{
var request = new XmlDocument();
try
{
if (!(xml.StartsWith("<?xml") || xml.StartsWith("<method")))
{
xml = xml.Substring(xml.IndexOf("<?xml"));
}
request.LoadXml(xml); // <=
}
catch (Exception ex)
{
throw new MetaWeblogException("01",
$"Invalid XMLRPC Request. ({ex.Message})");
}
....
}
Wait. What? What vulnerability?
Even though I showed you only those methods that relate to the error, it's still hard to see a vulnerability to an XXE attack.
Read the article to find a detailed description of such vulnerabilities in general and this one in BlogEngine.NET in particular. Here, I'll briefly describe the most important parts.
First, look at the ParseRequest method:
private static string ParseRequest(HttpContext context)
{
var buffer = new byte[context.Request.InputStream.Length];
context.Request.InputStream.Position = 0;
context.Request.InputStream.Read(buffer, 0, buffer.Length);
return Encoding.UTF8.GetString(buffer);
}
context.Request.InputStream contains some data sent by a user to an apllication. If the data is not checked, then InputStream may contain anything. Such data is called potentially tainted (read more here).
The ParseRequest method reads the request data into a buffer, converts it into a string and then returns it "as is". Where does the data go?
The answer is here:
public XMLRPCRequest(HttpContext input)
{
var inputXml = ParseRequest(input);
// LogMetaWeblogCall(inputXml);
this.LoadXmlRequest(inputXml); // Loads Method Call
// and Associated Variables
}
We see that the result of the ParseRequest work is passed to the LoadXmlRequest method. Once more, the data may contain anything, since only the user controls its contents. Now let's look at how the data is processed in the LoadXmlRequest method:
private void LoadXmlRequest(string xml)
{
var request = new XmlDocument();
try
{
if (!(xml.StartsWith("<?xml") || xml.StartsWith("<method")))
{
xml = xml.Substring(xml.IndexOf("<?xml"));
}
request.LoadXml(xml); // <=
}
catch (Exception ex)
{
throw new MetaWeblogException("01",
$"Invalid XMLRPC Request. ({ex.Message})");
}
....
}
That's where the vulnerability shows itself! The potentially tainted data is first passed to the xml parameter and then — to the LoadXml method of the XmlDocument class.
You may think: "Well, and? We have try-catch here, and even a check". The problem is, they won't save you from the real threat.
If an attacker passes certain data to the request, then the contents of almost any file of a system, in which the request is processed, will be written to the request variable.
The article describes how the contents may return to the user. It shows not just a "theoretical danger", but a completely working scenario for exploiting the vulnerability.
How is it that processing custom xml leads to reading files from the system? A brief explanation by the analyzer:
V5614. Potential XXE vulnerability inside method. Insecure XML parser is used to process potentially tainted data from the first argument: 'inputXml'. BlogEngine.Core XMLRPCRequest.cs 41
Thus, PVS-Studio detects and points out 2 components of the XML External Entity vulnerability:
- unsafe (unsafely configured) XML parser;
- tainted data passed to this parser.
We have already mentioned the tainted data. However, it can be difficult to understand how the parser may be unsafe. In this example, the default configuration used is unsafe. However, in general, everything is more complicated. You can read more about this vulnerability and ways to protect from it in the article: "Vulnerabilities due to XML files processing: XXE in C# applications in theory and in practice".
The error in BlogEngine.NET is completely different from others discussed in this article — it is not just "strange code", but a real vulnerability. There is an entry for it in the CVE database.
Conclusion
In 2022 we published many articles, thanks to which I managed to collect a diverse top. It also allows you to evaluate the variety of real errors that PVS-Studio can find. Some of them are the result of writing code in a rush, some are the result of not fully understanding the language features. Some of the errors hide until the analyzer finds them.
As usual, I'll leave a link to the page where you can download PVS-Studio and evaluate it on your project. That's all folks!
Top comments (0)