# Top 10 bugs found in C\# projects in 2020

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\!

![0787_Top_10_CSharp_Bugs_2020/image1.png](https://import.viva64.com/docx/blog/0787_Top_10_CSharp_Bugs_2020/image1.png)

## 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](https://pvs-studio.com/en/docs/warnings/v3022/) and [V3106](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0740/) 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:

```cpp
private static void UpdateEnvironmentVariableLicenseString(....)
{
  ....
  if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
    return;
  ....
}
```

**PVS\-Studio's diagnostic message**: [V3066](https://pvs-studio.com/en/docs/warnings/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_:

```cpp
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](https://pvs-studio.com/en/blog/posts/csharp/0704/)" written in the beginning of the year:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

```cpp
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](https://pvs-studio.com/en/blog/posts/csharp/0711/) was written in February\.

The reported snippet is highly suspicious, though I can't tell for sure if it's a bug:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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\!](https://pvs-studio.com/en/blog/posts/csharp/0704/)\.

How long will it take you to spot the bug?

```cpp
protected override void CheckForResult(....)
{
  ....
  ApplyResult(r =>
  {
    if (   holdNote.hasBroken
        && (result == HitResult.Perfect || result == HitResult.Perfect))
      result = HitResult.Good;
    ....
  });
}
```

**PVS\-Studio's diagnostic message**: [V3001](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0777/)\.

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:

```cpp
internal string RawOuterXml
{
  get => _rawOuterXml;

  set
  {
    if (string.IsNullOrEmpty(value))
    {
      _rawOuterXml = string.Empty;
    }

    _rawOuterXml = value;
  }
}
```

**PVS\-Studio's diagnostic message**: [V3008](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0725/) \(and it's a very strange one, I should tell you\)\. 

By the way, you can follow [me on Twitter](https://twitter.com/Nikita30005701) 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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0754/)\. 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:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0737/)"\. This bug is incredibly simple yet invisible to the human eye, especially in a project that big\. Do you think the rank is fair?

```cpp
public ReceiptsMessage Deserialize(byte[] bytes)
{
  if (bytes.Length == 0 && bytes[0] == Rlp.OfEmptySequence[0])
    return new ReceiptsMessage(null);
    ....
}
```

**PVS\-Studio's diagnostic message**: [V3106](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0740/)\. 

Meet the second\-place winner on our 2020 Top\-10 list of bugs:

```cpp
private OrderByField ExtractOrderByFromMethod(....)
{
  ....
  if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
    throw new InvalidQueryException(....);
  ....
}
```

**PVS\-Studio's diagnostic message**: [V3022](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/csharp/0783/)"\.

Now, I want you to read the saddest story ever about an _ArgumentException_ never\-to\-be\-thrown:

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/?tag=CSharp) 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](https://pvs-studio.com/en/pvs-studio/download/), 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\!