Top 10 Bugs Found in C# Projects in 2020
A developer takes a look back at some of the most interesting bugs he found in C#-based projects and goes through why each of these bits of code is a bug.
Join the DZone community and get the full member experience.Join For Free
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! The 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. This RavenDB project is used as an example:
PVS-Studio's diagnostic message: V3066
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
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:
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
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:
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 will crash elsewhere, when the program attempts to use the returned value. In this particular case, it's the call
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
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:
PVS-Studio's diagnostic message: V3043
Yes – the indents! It looks as if the line
damage += Utility.RandomMinMax( 0, 15 )was meant to be executed only when
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?
PVS-Studio's diagnostic message: V3001
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 6th 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:
PVS-Studio's diagnostic message: V3008
As you can see,
_rawOuterXml will be assigned
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:
PVS-Studio's diagnostic message: V3106
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 something other than
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
dims is passed as an argument to the
Shape.Matrix constructor. Now, how many elements were there in
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. 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:
PVS-Studio's diagnostic message: V3125
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
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
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?
PVS-Studio's diagnostic message: V3106
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:
PVS-Studio's diagnostic message: V3022
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:
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. 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
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 my 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
PVS-Studio's diagnostic message: V3022
Ranking the warnings wasn't easy, but I knew from the very beginning that this one was going to be the leader. The slightest, tiny typo in a tiny, simple, and neat function has broken the code – and neither IDE highlighting, nor code review, nor 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.
Picking the 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, numbers 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!
Published at DZone with permission of Nick Lipilin. See the original article here.
Opinions expressed by DZone contributors are their own.