This tough year, 2020, will soon be over at last, which means it's time to look back at our accomplishments! Over the year, the PVS-Studio team has written quite a number of articles covering a large variety of bugs found in open-source projects with the help of PVS-Studio. This 2020 Top-10 list of bugs in C# projects presents the most interesting specimens. Enjoy the reading!
How the list was formed
This list is composed of what I find the most interesting warnings collected across the articles my teammates and I have written over 2020. The main factor in deciding whether to include a warning or leave it out was the degree of certainty that the warning pointed at an actual issue. Of course, I also took the "appeal" of warnings into account when choosing and ranking them, but this quality is too subjective, so feel free to share your own opinion in the comments.
I've tried to make this list as varied as possible, in respect to both warnings and projects. The list spans eight projects, and almost every diagnostic rule is included only once – except V3022 and V3106, which are mentioned twice (no, these were not written by me, but they seem to be my favorites). I'm sure everyone will find something to their taste :).
Here we go! Top-10!
10 – Old new license
Our Top-10 list starts with a warning from an article by one very nice person, which deals with static analysis of C# projects on Linux and macOS. The RavenDB project is used as an example:
private static void UpdateEnvironmentVariableLicenseString(....)
{
....
if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
return;
....
}PVS-Studio's diagnostic message: V3066 Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'. LicenseHelper.cs(177) Raven.Server
Why, what's wrong here? The code compiles perfectly. Then why does the analyzer insist that we should first pass oldLicense and only then newLicense? You've guessed it already, haven't you? Let's take a look at the declaration of ValidateLicense:
private static bool ValidateLicense(License oldLicense,
RSAParameters rsaParameters,
License newLicense)Wow, indeed: the old license comes earlier than the new one in the parameter list. Now, can that dynamic analysis of yours catch things like that? :)
Anyway, this is an interesting case. Maybe the order doesn't actually matter here, but spots like that should be double checked, don't you think?
9 – 'FirstOrDefault' and unexpected 'null'
The 9th place goes to a warning from the article "Play "osu!", but Watch Out for Bugs" written in the beginning of the year:
public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
var ruleset = rulesets.GetRuleset(OnlineRulesetID);
var mods = Mods != null ? ruleset.CreateInstance()
.GetAllMods().Where(....)
.ToArray() : Array.Empty<Mod>();
....
}Do you see the bug? You don't? But it's there! Let's see what the analyzer says.
PVS-Studio's diagnostic message: V3146 [CWE-476] Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. APILegacyScoreInfo.cs 24
I didn't tell you everything at once. Actually, there's nothing suspicious about this code – but only because the FirstOrDefault method, which is mentioned in the warning, is located in the GetRuleset method's declaration:
public RulesetInfo GetRuleset(int id) =>
AvailableRulesets.FirstOrDefault(....);Oh my! The method returns RulesetInfo if a valid ruleset is found. But what if there's no such ruleset? No problem – here's your null. This null will crash elsewhere, when the program attempts to use the returned value. In this particular case, it's the call ruleset.CreateInstance().
You may wonder, what if that call simply cannot return null? What if the sought element is always present in the collection? Well, if the developer is so sure about this, why didn't they use First rather than FirstOrDefault?
8 – Python trail
The top warning of the lowest three comes from the project RunUO. The article was written in February.
The reported snippet is highly suspicious, though I can't tell for sure if it's a bug:
public override void OnCast()
{
if ( Core.AOS )
{
damage = m.Hits / 2;
if ( !m.Player )
damage = Math.Max( Math.Min( damage, 100 ), 15 );
damage += Utility.RandomMinMax( 0, 15 );
}
else { .... }
}PVS-Studio's diagnostic message: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Earthquake.cs 57
Yes – the indents! It looks as if the line damage += Utility.RandomMinMax( 0, 15 ) was meant to be executed only when m.Player is false. That's how this code would work if written in Python, where indents not only make the code look neater but also determine its logic. But the C# compiler has a different opinion! And I wonder what the developer has to say about this.
Actually, there are only two possible scenarios. Either the braces are indeed missing here and the code's logic has gone all awry, or this code is fine but you can be sure someone will eventually come and "fix" this spot, mistaking it for a bug.
I may be wrong, and maybe there are cases when patterns like that are legit. If you know anything about this, please let me know in the comments – I'm really eager to figure this out.
7 – Perfect, or Perfect, that is the question!
Ranking warnings is getting harder. Meanwhile, here's another warning from the article on osu!.
How long will it take you to spot the bug?
protected override void CheckForResult(....)
{
....
ApplyResult(r =>
{
if ( holdNote.hasBroken
&& (result == HitResult.Perfect || result == HitResult.Perfect))
result = HitResult.Good;
....
});
}PVS-Studio's diagnostic message: V3001 There are identical sub-expressions 'result == HitResult.Perfect' to the left and to the right of the '||' operator. DrawableHoldNote.cs 266
Not long, I suppose, because you just need to read the warning. That's what developers who are friends with static analysis usually do :). You could argue about the previous cases, but this one is definitely a bug. I'm not sure which of the elements of HitResult exactly should be used instead of the second Perfect (or the first, for that matter), but the current logic is obviously wrong. Well, that's not a problem: now that the bug is found, it can be fixed easily.
6 – null shall (not) pass!
The 6-th place is awarded to a very cool warning found in Open XML SDK. The check of this project is covered here.
The developer wanted to make sure that a property wouldn't be able to return null even if assigned it explicitly. This is a great feature indeed, which helps guarantee that you won't get null no matter what. The bad news is, it's broken here:
internal string RawOuterXml
{
get => _rawOuterXml;
set
{
if (string.IsNullOrEmpty(value))
{
_rawOuterXml = string.Empty;
}
_rawOuterXml = value;
}
}PVS-Studio's diagnostic message: V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164
As you can see, _rawOuterXml will be assigned value anyway, null or not. A brief glance at this snippet may mislead you into thinking that the property will never get null – the check won't let it! Well, if you do think so, you risk discovering a NullReferenceException instead of presents under the Christmas tree :(
5 – An ambush in an array with a nested array
The 5th specimen on this list comes from the project TensorFlow.NET, which I checked personally (and it's a very strange one, I should tell you).
By the way, you can follow me on Twitter if you like learning about interesting bugs in real C# projects. I'll be sharing examples of unusual warnings and code snippets, many of which, unfortunately, won't be included in the articles. See you on Twitter! :)
Okay, let's get back to the warning:
public TensorShape(int[][] dims)
{
if(dims.Length == 1)
{
switch (dims[0].Length)
{
case 0: shape = new Shape(new int[0]); break;
case 1: shape = Shape.Vector((int)dims[0][0]); break;
case 2: shape = Shape.Matrix(dims[0][0], dims[1][2]); break; // <=
default: shape = new Shape(dims[0]); break;
}
}
else
{
throw new NotImplementedException("TensorShape int[][] dims");
}
}PVS-Studio's diagnostic message: V3106 Possibly index is out of bound. The '1' index is pointing beyond 'dims' bound. TensorShape.cs 107
I actually found it hard to decide which place to rank this warning because it's nice but so are the rest. Anyway, let's try to figure out what's going on in this code.
If the number of arrays in dims is other than 1, a NotImplementedException is thrown. But what if that number is exactly 1? The program will proceed to check the number of elements in this "nested array". Note what happens when that number is 2. Unexpectedly, dims[1][2] is passed as an argument to the Shape.Matrix constructor. Now, how many elements were there in dims?
Right, exactly one – we've just checked this! An attempt to get a second element from an array that contains only one will result in throwing an IndexOutOfRangeException. This is obviously a bug. But what about the fix – is it as obvious?
The first solution that comes to mind is to change dims[1][2] to dims[0][2]. Will it help? Not a bit! You'll get the same exception, but this time the issue relates to the fact that in this branch the number of elements is 2. Did the developer make two mistakes at once indexing the array? Or maybe they meant to use some other variable? God knows… The analyzer's job is to find the bug; fixing it is the job of the programmer who let it through, or their teammates.
4 – A property of a non-existent object
Here's another warning from the article about OpenRA. Perhaps it deserves a higher place, but I ranked it 4th. That's a great result, too! Let's see what PVS-Studio says about this code:
public ConnectionSwitchModLogic(....)
{
....
var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
if (logo != null)
{
logo.GetSprite = () =>
{
....
};
}
if (logo != null && mod.Icon == null) // <=
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width; // <=
}
....
}PVS-Studio's diagnostic message: V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236
What should we look for in this code? Well, for one thing, note that logo may well be assigned null. This is hinted at by the numerous checks as well as the name of the GetOrNull method, whose return value is written to logo. If so, let's trace the sequence of events assuming that GetOrNull returns null. It starts off well, but then we hit the check logo != null && mod.Icon == null. Execution naturally goes down the else branch… where we attempt to access the Bounds property of the variable storing the null, and then – KNOCK-KNOCK! He knocks boldly at the door who brings NullReferenceException.
3 – Schrödinger's element
We have finally reached the three topmost winners. Ranked 3rd is a bug found in Nethermind – the check is covered in an intriguingly titled article "Single line code or check of Nethermind using PVS-Studio C# for Linux". This bug is incredibly simple yet invisible to the human eye, especially in a project that big. Do you think the rank is fair?
public ReceiptsMessage Deserialize(byte[] bytes)
{
if (bytes.Length == 0 && bytes[0] == Rlp.OfEmptySequence[0])
return new ReceiptsMessage(null);
....
}PVS-Studio's diagnostic message: V3106 Possibly index is out of bound. The '0' index is pointing beyond 'bytes' bound. Nethermind.Network ReceiptsMessageSerializer.cs 50
I guess it would be cool if you could pick up the first thing from an empty box, but in this case, you'll only get an IndexOutOfRangeException. One tiny mistake in the operator leads to incorrect behavior or even a crash.
Obviously, the '&&' operator must be replaced with '||' here. Logic errors like this are not uncommon, especially in complex constructs. That's why it's very handy to have an automatic checker to catch them.
2 – Less than 2 but larger than 3
Here's another warning from RavenDB. As a reminder, the results of checking this project (as well as other matters) are discussed in this article.
Meet the second-place winner on our 2020 Top-10 list of bugs:
private OrderByField ExtractOrderByFromMethod(....)
{
....
if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
throw new InvalidQueryException(....);
....
}PVS-Studio's diagnostic message: V3022 Expression 'me.Arguments.Count < 2 && me.Arguments.Count > 3' is always false. Probably the '||' operator should be used here. QueryMetadata.cs(861) Raven.Server
We have already looked at examples of unexpectedly thrown exceptions. Now, this case is just the opposite: an expected exception will never be thrown. Well, it still might but not until somebody invents a number less than 2 but larger than 3.
I won't be surprised if you disagree with my ranking, but I do like this warning more than all the previous ones. Yes, it's astonishingly simple and can be fixed by simply modifying the operator. That, by the way, is exactly what the message passed to the InvalidQueryException constructor hints at: "Invalid ORDER BY 'spatial.distance(from, to, roundFactor)' call, expected 2-3 arguments, got " + me.Arguments.Count.
Yes, it's just a blunder, but nobody had noticed and fixed it – at least not until we discovered it with PVS-Studio. This reminds me that programmers, no matter how skilled, are still only humans (unfortunately?). And for whatever reasons, humans, no matter their qualification, will once in a while overlook even silly mistakes like this. Sometimes a bug shows up right away; sometimes it takes a long-long time before the user gets a warning about an incorrect call of ORDER BY.
1 – Quotation marks: +100% to code security
Yippee! Meet the leader – the warning that I believe to be the most interesting, funny, cool, etc. It was found in the ONLYOFFICE project discussed in one of the most recent articles – "ONLYOFFICE Community Server: how bugs contribute to the emergence of security problems".
Now, I want you to read the saddest story ever about an ArgumentException never-to-be-thrown:
public void SetCredentials(string userName, string password, string domain)
{
if (string.IsNullOrEmpty(userName))
{
throw new ArgumentException("Empty user name.", "userName");
}
if (string.IsNullOrEmpty("password"))
{
throw new ArgumentException("Empty password.", "password");
}
CredentialsUserName = userName;
CredentialsUserPassword = password;
CredentialsDomain = domain;
}PVS-Studio's diagnostic message: V3022 Expression 'string.IsNullOrEmpty("password")' is always false. SmtpSettings.cs 104
Ranking the warnings wasn't easy, but I knew from the very beginning that this one was going to be the leader. A slightest tiny typo in a tiny, simple, and neat function has broken the code – and neither IDE highlighting, nor code review, nor the good old common sense helped catch it in good time. Yet PVS-Studio managed to figure out even this tricky bug, which experienced developers failed to notice.
The devil is in the details, as usual. Wouldn't it be nice to have all such details checked automatically? It sure would! Let developers do what analyzers cannot – create new cool and safe applications; enjoy creative freedom without bothering about an extra quotation mark in a variable check.
Conclusion
Picking ten most interesting bugs from this year's articles was easy. It was ranking them that proved to be the most difficult part. On the one hand, some of the warnings better showcase some of PVS-Studio's advanced techniques. On the other hand, some of the bugs are just fun to look at. Many of the warnings here could be swapped places – for example, 2 and 3.
Do you think this list should be entirely different? You can actually draw up your own: just follow this link to see the list of articles checked by our team and choose the tastiest warnings to your liking. Share your 2020 tops in the comments – I'd love to take a look at them. Think your list can beat mine?
Of course, whether one warning is more interesting than another is always a matter of taste. Personally, I believe the significance of a warning should be estimated based on whether it encourages the programmer to change anything in the problem code. It was this quality that I was keeping in mind when composing my list. I chose warnings that referred to those spots in code that I believe would look better if found and fixed through the use of static analysis. Besides, anyone can always try PVS-Studio on their own or someone else's projects. Just follow this link, download the version that suits you most, and fill out a small form to get a trial license.
That's all for today. Thank you for reading, and see you soon!
