Finally, such a difficult 2020 is coming to an end, which means it's time to take stock! During this year, the PVS-Studio team has written many articles that dealt with various errors found by the analyzer in open-source projects. You can see the most interesting of them right here, in the TOP errors found in C # projects in 2020. Happy viewing!
How the top was formed
This list contains the most interesting, in my opinion, triggers that my colleagues and I wrote about in articles for 2020. An important selection criterion was the degree of confidence that a mistake had indeed been made in the corresponding code fragment. And, of course, when selecting, as well as placing places, the 'interestingness' of the trigger was taken into account, but this is already my subjective opinion - you can always challenge it in the comments.
I tried to make the top as diverse as possible: both in terms of PVS-Studio messages, and in terms of projects for which code warnings were issued. The list includes detections on the sources of 8 verified projects. At the same time, the diagnostic rules are almost never repeated - you can meet twice here only V3022 and V3106 (and no, they weren't made by me, but apparently these are my favorites). Thus, variety is guaranteed here :).
Well, let's get started! Top 10!
10th place - New old license
Our top response opens from an article by one very good person about checking C # projects for Linux and macOS, where the RavenDB project was used as an example:
private static void UpdateEnvironmentVariableLicenseString(....)
{
....
if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
return;
....
}
Analyzer Warning : V3066 Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'. LicenseHelper.cs (177) Raven.Server
It would seem, what's wrong here? The code compiles quite well. Why did the analyzer decide that it is necessary to transfer the oldLicense first , and then the newLicense ? You guessed it, right? Let's take a look at the ValidateLicense declaration :
private static bool ValidateLicense(License oldLicense,
RSAParameters rsaParameters,
License newLicense)
Wow, it's true - first, the old one is in the parameters, and only then - the new license. Now tell me, can this dynamic analysis of yours catch this? :)
In any case, the triggering is interesting. Maybe the order is not really important there, but it's better to double-check such fragments, agree?
9th place - 'FirstOrDefault' and unexpected 'null'
In 9th place was the response from the article " Play in" osu! ", Don't forget about mistakes ", written at the beginning of the current year:
public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
var ruleset = rulesets.GetRuleset(OnlineRulesetID);
var mods = Mods != null ? ruleset.CreateInstance()
.GetAllMods().Where(....)
.ToArray() : Array.Empty<Mod>();
....
}
See the error? And she is! What does the analyzer say?
Analyzer warning : V3146 [CWE-476] Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. APILegacyScoreInfo.cs 24
Yes, yes, again I did not give all the necessary information at once. In fact, you really can't see anything suspicious in this code. After all, FirstOrDefault , which the analyzer tells us about, is in the definition of the GetRuleset method :
public RulesetInfo GetRuleset(int id) =>
AvailableRulesets.FirstOrDefault(....);
Terrible business! The method will return RulesetInfo if it finds a suitable one. And if not? Calmly return null . And it will shoot already in the place where the result of the call will be used. In this case, when calling ruleset.CreateInstance () .
The question may arise: what if there is never null ? What if the collection always contains the required element? Well, if the developer is sure about this, then why not use First instead of FirstOrDefault ?
8th place - Hello from Python
The last trigger from the first three was issued for the RunUO project code. An article on verifying it was written in February and is available here .
The fragment found is extremely suspicious, although it is difficult to say for sure if it is a mistake:
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 { .... }
}
Analyzer Warning : 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
That's right - it's about indentation! It seems that the line damage + = Utility.RandomMinMax (0, 15) should have been executed only if m.Player is false... Python code would work in a similar way, where indentation is written not only for beauty, but also to define the logic of the application. But the C # compiler has a different opinion on this matter! I wonder what opinion did the developer have?
In general, in this situation, there are 2 options. Either curly braces are really missing here, and everything works incorrectly, or everything works correctly, but over time, there will definitely be a person who will consider this a mistake and "fix" it.
Maybe I'm wrong, and there really are times when it's okay to write something like that. If you are aware of such cases, then please write a comment - I would be really interested to learn about such cases.
7th place - Perfect or Perfect - that is the question!
It is getting more and more difficult to distribute warnings in places, and we are moving on to the second alarm in this list from the article about osu! ...
How long will it take for you to see the error?
protected override void CheckForResult(....)
{
....
ApplyResult(r =>
{
if ( holdNote.hasBroken
&& (result == HitResult.Perfect || result == HitResult.Perfect))
result = HitResult.Good;
....
});
}
Analyzer warning : V3001 There are identical sub-expressions 'result == HitResult.Perfect' to the left and to the right of the '||' operator. DrawableHoldNote.cs 266 Not
much, I think, you just need to read the PVS-Studio warning. Developers using static analysis usually do this :). One could argue about the previous places in the top, but here the mistake is obvious. It is difficult to say which specific element of the HitResult enumeration should have been used here instead of the second Perfect (or instead of the first), but in the end something was clearly written wrong. Well, it's okay - the error was found, which means it can be easily fixed.
6th place - null (not) pass!
The 6th place was a very cool response to code from the Open XML SDK project. An article on checking it can be read here .
The developer wanted to implement a property that would not return null , even if assigned directly. And it's really great when you can be sure that null will not be written under any circumstances. It is a pity that this is not the situation at all:
internal string RawOuterXml
{
get => _rawOuterXml;
set
{
if (string.IsNullOrEmpty(value))
{
_rawOuterXml = string.Empty;
}
_rawOuterXml = value;
}
}
Parser Warning : V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164
It turns out that in _rawOuterXml will be recorded value regardless of whether null or not. Looking at this code inattentively, you might think that null will never be written to this property - after all, there is a check! Well, if so, then under the tree you can find not gifts, but an unexpected NullReferenceException :(
5th place - Shot from an array with an array inside
The top 5 triggering of 2020 was issued by the analyzer for the TensorFlow.NET project I personally tested. By clicking on the link , you can read the article on checking this project (oh, and I've seen everyone there a lot).
By the way, if you like to watch examples of interesting errors from real C # projects, then I suggest you subscribe to my twitter . There I plan to post curious triggers and simply interesting code fragments, many of which, alas, will not be included in articles. I will be glad to see you! :)
Well, now let's move on to triggering:
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");
}
}
Analyzer warning : V3106 Possibly index is out of bound. The '1' index is pointing beyond 'dims' bound. TensorShape.cs 107
In fact, it was very difficult to choose where to put this trigger, since it is really interesting, but others are not far behind in this regard. So, let's figure out what's going on here.
If the number of arrays in dims is not equal to 1, then an exception of the NotImplementedException type is thrown . And what will happen if in dimsexactly one array? The number of elements in this 'array within an array' is checked. Note what happens when it is 2. As one of the arguments to the Shape.Matrix constructor , unexpectedly, dims [1] [2] is passed . Now let's remember - how many elements were there in the dims array ?
That's right, exactly one - we just checked it! An attempt to get the second element from an array, which contains only one element, will throw an IndexOutOfRangeException exception . Obviously a mistake. But is there an obvious way to fix it?
The first thing that comes to mind is change dims [1] [2] to dims [0] [2] . Will the error go away? No matter how it is! There will be the same exception again. But this time it will be connected with the fact that in this case-branch the number of elements in dims [0] is equal to 2. Has the developer made two errors in the index in a row? Or should some other variable be used there? Who knows ... The analyzer's job is to find a mistake, and the person who made it or his colleagues will have to correct it.
4th place - Property of an object that does not exist
Another trigger got into this top from the article about the OpenRA check . Perhaps it deserves more, but by the will of fate, this trigger was in 4th place. Well, that's pretty good too! Let's take a look at what error PVS-Studio was able to detect this time:
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; // <=
}
....
}
Analyzer warning : V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236
What should you pay attention to here? Well, for starters, the logo can probably contain null . This is hinted at by the constant checks and the name of the GetOrNull method , which returns the value written to the logo . If so, let's think about what happens if GetOrNull really returns null . At first everything is in order, but then the condition is checked logo! = null && mod.Icon == null . As you can imagine, the result will be a transition to the else -branch and ... Attempt to access the Bounds property of the variable, which contains null , and then - bdysh! NullReferenceException is knocking on the door.
3rd place - Schrödinger's element
Finally, we come to the top three finalists. The top 3 bugs for 2020 were found in the Nethermind project, about the verification of which an article was written with the intriguing title " Code in one line or checking Nethermind using PVS-Studio C # for Linux " The error is incredibly simple, but invisible to the human eye, especially when you consider the size of the project. Do you think this trigger is worthy of its place?
public ReceiptsMessage Deserialize(byte[] bytes)
{
if (bytes.Length == 0 && bytes[0] == Rlp.OfEmptySequence[0])
return new ReceiptsMessage(null);
....
}
Analyzer warning : V3106 Possibly index is out of bound. The '0' index is pointing beyond 'bytes' bound. Nethermind.Network ReceiptsMessageSerializer.cs 50
Probably being able to take the first thing in an empty box would be cool, but here such a desire will only lead to an IndexOutOfRangeException being thrown . Just one trifle - a tiny mistake in the operator, and the application is already working incorrectly, or maybe even crashes.
Obviously, you should use '||' instead of '&&'. Such logical errors are not uncommon, especially in complex designs. Therefore, it is quite convenient to check such moments in automatic mode.
2nd place - Less than 2, but more than 3
In second place, I put one more trigger on the code from the RavenDB project. Let me remind you that you can read about the results of its verification (and not only) in the corresponding article .
Well, now meet - the top 2 mistake of 2020:
private OrderByField ExtractOrderByFromMethod(....)
{
....
if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
throw new InvalidQueryException(....);
....
}
Analyzer warning : 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
Earlier we looked at the moments in which an unexpected exception was thrown, but now, on the contrary, the expected exception will never be thrown. Well, or it will still be thrown out if someone comes up with a number that is less than 2, but more than 3.
I would not be surprised if you disagree, but I really like this trigger more than all the previous ones. Yes, the error is incredibly simple, and to fix it you just need to change the operator. By the way, this is also hinted at by the message passed to the constructor InvalidQueryException : "Invalid ORDER BY 'spatial.distance (from, to, roundFactor)' call, expected 2-3 arguments, got" + me.Arguments.Count .
I agree, this is an elementary oversight, but no one noticed or corrected it, at least until it was found using PVS-Studio. It reminds me that programmers, no matter how experienced they are, are still (unfortunately?) Human. And people, regardless of qualifications, can miss even such stupid mistakes for a variety of reasons. Sometimes the error fires right away, and sometimes you can wonder for a long time why the user does not see the message about the incorrect ORDER BY call.
1st place - Quotes for + 100% code safety
Hurray, hurray, hurray! We finally got to the trigger, which I found the most interesting, fun, cool, and so on. It was issued for code from the ONLYOFFICE project, the analysis of which is associated with one of the most recent articles of this year - " ONLYOFFICE Community Server: How Bugs Contribute to Security Problems ".
Well, I present to you the saddest story about ArgumentException , which will never 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;
}
Analyzer warning : V3022 Expression 'string.IsNullOrEmpty ("password")' is always false. SmtpSettings.cs 104
It was very difficult to choose which error to put in which place, but this trigger for me from the very beginning was the leader among all. The simplest minor typo - and the code already works incorrectly. Neither the highlighting in the IDE, nor the review, nor the good old common sense helped. It's a small, simple, beautifully written function. And even here PVS-Studio was able to find what was missed by the professionals.
As usual, the devil is in the details. Wouldn't it be great if all such details were searched automatically? Of course it would! And let the developer do what a static analyzer cannot do - he creates new beautiful and safe applications. He does it without thinking about whether he put extra quotes when checking a variable or not.
Conclusion
Finding 10 interesting mistakes in 2020 articles was easy. But to distribute them in places turned out to be quite a task. On the one hand, some triggers better reflect the work of advanced analyzer mechanisms. On the other hand, some of the mistakes just seem funny to some extent. Many of the positions presented could be swapped - for example, top 2 and top 3.
Or maybe you think there should be some other positives on this list? In fact, you can even create your own top by following the linkto the list of articles and finding the most delicious triggers in your opinion. In this case, be sure to throw off your 2020 tops in the comments, I will read it with great pleasure. Can you make a list more interesting than mine?
Of course, the question of the 'interestingness' of warnings is subjective anyway. In my opinion, the main criterion for evaluating the response is whether a programmer who sees a warning from PVS-Studio will change something in the corresponding code? This list was just assembled from triggers for fragments, which, in my opinion, would look better if the developers used static analysis. Moreover, there are no problems with trying PVS-Studio by checking your own or some other projects. You just need to follow the link, download the required version of the analyzer there and request a trial key by filling out a small form.
Well, that's all for me, thank you very much for your attention and see you soon!
If you want to share this article with an English-speaking audience, please use the translation link: Nikita Lipilin. Top-10 Bugs Found in C # Projects in 2020 .