DEV Community

Anastasiia Vorobeva
Anastasiia Vorobeva

Posted on

Top 10 errors found in C# projects in 2024

#csharp #programming #softwaredevelopment

Throughout 2024, the PVS-Studio team has been actively sharing articles about checking open-source C# projects. Continuing the tradition, we've compiled the Top 10 of the most intriguing bugs detected this year. Enjoy reading!

1203_CSharpTop_2024/image1.png

Foreword

There are several criteria the project code should meet to earn a place in our top list:

  • it comes from an open-source project;
  • the issues were identified by PVS-Studio;
  • the code most likely contains errors;
  • the code is interesting to check.

As we deliver such tops annually, we've gathered a wealth of curious bugs uncovered over the past years. You can explore the previous compilations of C# bugs here:

P.S. Don't take the rankings too seriously—they're based on the author's subjective opinion. If you think this or that bug deserves another place, feel free to leave a comments :)

Now, let's dive into the fascinating abyss of C# errors for 2024!

10th place. Stack Overflow instead of comparison

Our top opens with the analyzer warning highlighted in the article about Unity 6:

public readonly struct SearchField : IEquatable<SearchField>
{
  ....
  public override bool Equals(object other)
  {
    return other is SearchIndexEntry l && Equals(l);
  }

  public bool Equals(SearchField other)
  {
    return string.Equals(name, other.name, StringComparison.Ordinal);
  }
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3197 The compared value inside the 'Object.Equals' override is converted to the 'SearchIndexEntry' type instead of 'SearchField' that contains the override. SearchItem.cs 634.

As the analyzer message states, in the first Equals method, the other parameter is incorrectly casted to the SearchIndexEntry type instead of SearchField. This results in the same method overload being invoked when Equals(l) is called later. Yet, if other actually belongs to the SearchIndexEntry type, the code will loop, and the StackOverflowException will be thrown.

This issue may occur due to careless copy-paste.

9th place. Suspicious foreach

The next place is held by a classic error from the article about checking Garnet:

public IEnumerable<long> GetPendingRequests()
{
  foreach (var kvp in ctx.prevCtx?.ioPendingRequests)
    yield return kvp.Value.serialNum;

  foreach (var kvp in ctx.ioPendingRequests)
    yield return kvp.Value.serialNum;
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: ctx.prevCtx?.ioPendingRequests. ClientSession.cs 748

A developer used the ?. operator here, likely assuming that ctx.prevCtx could be null. However, foreach doesn't work with null, so the exception will still hit the developer—but when the GetEnumerator method is called.

As we've mentioned in other articles, many developers may not know this case. For those interested, we've dedicated an article to this topic: Using the ?. operator in foreach: protection against NullReferenceException that doesn't work.

1203_CSharpTop_2024/image2.png

On the one hand, unlike in C++, such errors in C# aren't critical and are quickly detected if the reference becomes null. In some cases, the null reference may be rare or even impossible, so these errors can persist for a long time and accumulate. Yet, it may pop up in the future. Crashing a program due to a NullReferenceException can lead to a frustrating and time-consuming search for the issue, especially if the error only occurs on the client side, making it difficult or even impossible to reproduce.

8th place. Unused value

The warning from the nopCommerce check takes the 8th place.

public virtual async Task<....> GetOrderAverageReportLineAsync(....)
{
  ....

  if (!string.IsNullOrEmpty(orderNotes))
  {
    query = from o in query
            join n in _orderNoteRepository.Table on o.Id equals n.OrderId
            where n.Note.Contains(orderNotes)
            select o;

    query.Distinct();                          // <=
  }

  ....
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3010 The return value of function 'Distinct' is required to be utilized. OrderReportService.cs 342

To delete duplicated items, we can use Distinct, the LINQ method. That's what developers wanted to do here, but something went wrong. The Distinct method doesn't modify the collection for which it's called. So, if we don't use the return value of a method, the call is meaningless. This is exactly what happens in this code snippet.

Most likely, the result of the Distinct execution should be assigned to the query variable.

7th place. Incorrect String.Format

We're moving on. This error lurks in the Unity 6.

void UpdateInfo()
{
  ....

  var infoLine3_format = "<color=\"white\">CurrentElement:" +
                         " Visible:{0}" +
                         " Enable:{1}" +
                         " EnableInHierarchy:{2}" +
                         " YogaNodeDirty:{3}";

  m_InfoLine3.text = string.Format(infoLine3_format,
                                   m_LastDrawElement.visible,
                                   m_LastDrawElement.enable,
                                   m_LastDrawElement.enabledInHierarchy,
                                   m_LastDrawElement.isDirty);

  var infoLine4_format = "<color=\"white\">" +
                         "Count of ZeroSize Element:{0} {1}%" +
                         " Count of Out of Root Element:{0} {1}%";

  m_InfoLine4.text = string.Format(infoLine4_format,
                                   countOfZeroSizeElement,
                                   100.0f * countOfZeroSizeElement / count,
                                   outOfRootVE,
                                   100.0f * outOfRootVE / count);
  ....
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: 3rd, 4th. UILayoutDebugger.cs 179.

Let's take a look at the second string.Format(....). Its format string (the first argument) has four placeholders, and four values are passed for insertion (from the second to the fifth argument). The issue is that the placeholders contain only 0 and 1. As a result, only the first and second values are inserted.

The fixed format string looks as follows:

var infoLine4_format = "<color=\"white\">" +
                       "Count of ZeroSize Element:{0} {1}%" +
                       " Count of Out of Root Element:{2} {3}%";
Enter fullscreen mode Exit fullscreen mode

6th place. How about a prefix?

Rounding out the first half of the top is a bug from the Diablo 3 server emulator:

public static void GrantCriteria(....)
{
  ....
  else
  {
    ....
    uint alcount = alreadycrits.CriteriaId32AndFlags8;

    var newrecord = D3.Achievements
                      ....
                      .SetQuantity32(alcount++) // <=
                      .Build();

    int critCount = UnserializeBytes(achievement.Criteria).Count;
    if (critCount >= 5)
    {
      GrantCriteria(client, 74987246353740);
    }
    client.Account.GameAccount.AchievementCriteria
                              .Remove(alreadycrits);

    client.Account.GameAccount.AchievementCriteria
                              .Add(newrecord);
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3159 Modified value of the 'alcount' operand is not used after the postfix increment operation. AchievementManager.cs 360

We expect that alcount should be incremented by 1, and then the resulting value should be passed to the SetQuantity32 method. In reality, the opposite happens: first, the alcount value is passed to the method, and only then the variable value is incremented by 1.

We can easily fix the error by replacing alcount++ with the alcount + 1 or ++alcount expression.

5th place. Impossible null!

An error detected in .NET 8 kicks off the top five! It's worth noting that we've analyzed this project at the very end of 2023, so its errors didn't make it into the previous top list. But we simply couldn't resist adding errors from such a renowned project to the compilation. Here's one of them:

Instruction[]? GetArgumentsOnStack (MethodDefinition method)
{
  int length = method.GetMetadataParametersCount ();
  Debug.Assert (length != 0);
  if (stack_instr?.Count < length)
    return null;

  var result = new Instruction[length];
  while (length != 0)
    result[--length] = stack_instr!.Pop ();    // <=

  return result;
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3125 The 'stack_instr!' object was used after it was verified against null. Check lines: 1918, 1913. UnreachableBlocksOptimizer.cs 1918

A developer used the ?. operator, suggesting that the stack_instr field could be null. Everything seemed fine, there was a check, but... no such luck. It was still possible to dereference a null reference in the specified line. Most likely, the developer thought that the stack_instr?.Count < length expression with stack_instr equal to null would return true, and the method would exit. But no—the result would be false.

Moreover, the developer suppressed the compiler message about the possible dereference of the null reference with !. They might have thought that the compiler static analysis just failed, and it didn't identify the check.

What do you think about a nullable context? If you're interested in our opinion or not yet familiar with it, I suggest you read our articles:

4th place. Always first

In the fourth place, there's also an error from .NET 8:

private static string GetTypeNameDebug(TypeDesc type)
{
  string result;
  TypeDesc typeDefinition = type.GetTypeDefinition();
  if (type != typeDefinition)
  {
    result = GetTypeNameDebug(typeDefinition) + "<";
    for (int i = 0; i < type.Instantiation.Length; i++)
      result += (i == 0 ? "" : ",") + GetTypeNameDebug(type.Instantiation[0]);
    return result + ">";
  }
  else
  {
    ....
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3102 Suspicious access to element of 'type.Instantiation' object by a constant index inside a loop. TypeLoaderEnvironment.GVMResolution.cs 32

Here, the following element may be created from information about a type: ConsoleApp1.Program.MyClass<string, int, double>. However, the type.Instantiation object is accessed in the loop by a constant index of 0. It's possible that it works as it should, but it looks odd because GetTypeNameDebug(type.Instantiation[i]) is expected.

3rd place. Garbage collector causes the NRE

The bronze place goes to the error from Unity 6. It ranks so high because of its non-obvious nature. You can see more details here:

[RequiredByNativeCode]
internal static void InvalidateAll()
{
  lock (s_Instances)
  {
    foreach (var kvp in s_Instances)
    {
      WeakReference wr = kvp.Value;
      if (wr.IsAlive)
        (wr.Target as TextGenerator).Invalidate();
     }
   }
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V3145 Unsafe dereference of a WeakReference target. The object could have been garbage collected between checking 'IsAlive' and accessing the 'Target' property. TextGenerator.cs 140.

Despite the small amount of code, this is probably the most complex error in the article. Here are the reasons why:

  1. Not everyone is familiar with the concept of WeakReference. The garbage collector can always remove the object referenced by a weak reference, despite the presence of that reference.
  2. The WeakReference.IsAlive property enables us to check whether the object referenced by the weak reference still exists. However, this code ignores the possibility that the object can be cleaned after passing the check but before the reference is dereferenced. As a result, the NullReferenceException may be thrown anyway.
  3. One would assume that the lock operator could protect the object from the garbage collector, but after reproducing the case, we discovered this is not true.

So, how do we protect ourselves in this case? To ensure that the NullReferenceException doesn't occur, we need to create a strong reference to the object. Then, the garbage collector will no longer delete it as long as the reference is still in use. In other words, create a simple local variable referring to the object and work with it. The safe method can look like this:

[RequiredByNativeCode]
internal static void InvalidateAll()
{
  lock (s_Instances)
  {
    foreach (var kvp in s_Instances)
    {
      WeakReference wr = kvp.Value;
      var target = wr.Target;

      If (target != null)
        (target as TextGenerator).Invalidate();     
     }
   }
}
Enter fullscreen mode Exit fullscreen mode

2nd place. ReDoS

The second place goes to an error from the article on analyzing ScreenToGif. It's notable for also being a vulnerability. What this issue is and what it can lead to, we'll explore next:

public static string ReplaceRegexInName(string name)
{
  const string dateTimeFileNameRegEx = @"[?]([ymdhsfzgkt]+[-_ ]*)+[?]";  // <=

  if (!Regex.IsMatch(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase))
    return name;

  var match = Regex.Match(name, dateTimeFileNameRegEx, RegexOptions.IgnoreCase);
  var date = DateTime.Now.ToString(Regex.Replace(match.Value, "[?]", ""));

  return name.Replace(match.ToString(), date);
}
Enter fullscreen mode Exit fullscreen mode

The PVS-Studio warning:

V5626 Possible ReDoS vulnerability. Potentially tainted data from the 'name' variable is processed by regular expression that contains unsafe pattern: '(...sfzgkt]+...)+'

The code contains a ReDoS vulnerability that can slow down or completely crash an application. The issue stems from using a vulnerable regular expression, in which an arbitrary string can be passed. If the input line is composed in a certain way, the application will hang.

The code contains the following vulnerable regular expression: "[?]([ymdhsfzgkt]+[-_ ]*)+[?]". Let's simplify it for clarity: "[?]([ymdhsfzgkt]+)+[?]". The problematic pattern is "(x+)+" here. It significantly slows down the regular expression when a certain string is passed. This vulnerability is relevant only for algorithms based on a nondeterministic finite automaton.

We've managed to reproduce it in the application without much difficulty:

1203_CSharpTop_2024/image3.png

As shown in the screenshot, if we enter a certain line in the output file name, the application will hang.

In our case, we used the following line: "?ymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgktymdhsfzgkt".

Indeed, it's unlikely that anyone would use a name like that, so the vulnerability is harmless here. However, if one lets this happen in the server-related code, it can slow down its work or even cause a crash.

You can read more about this vulnerability in the article: Catastrophic backtracking: how can a regular expression cause a ReDoS vulnerability?

PVS-Studio is a SAST solution that uses static analysis to detect various potential vulnerabilities. This helps improve the security of tested applications. The analyzer can find many security flaws, including SQL and LDAP injections, XSS, XXE, and others.

1st place. Format or interpolation?

Finally, we've reached the top spot! The error is straightforward, but that doesn't make it any less intriguing. It seems that any developer may easily make this mistake. By the way, this one is also from .NET 8.

You can try to find it yourself.

public void WriteTo(TextWriter writer, int methodRva, bool dumpRva)
{
  ....
  switch (Flags & CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_KIND_MASK)
  {
    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_NONE:
      writer.Write($" CATCH: {0}", ClassName ?? "null");
      break;

    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_FILTER:
      writer.Write($" FILTER (RVA {0:X4})",
                   ClassTokenOrFilterOffset + methodRva);
      break;
    ....
  }
  ....
}
Enter fullscreen mode Exit fullscreen mode

The answer is hidden here
The PVS-Studio warning:

V3025 Incorrect format. A different number of format items is expected while calling 'Write' function. Arguments not used: ClassName ?? "null". EHInfo.cs 135

Here's another format string error, but in the TextWriter class. A developer uses the string interpolation $ character. It simply substitutes 0 into the string, and the format string will be equal to " CATCH: 0". As a result, the text they want to substitute for the {0} placeholder isn't used. The same error occurs in the next case method.

Conclusion

While 2024 may not have been packed with articles about analyzing C# projects, that didn't stop us from creating this top list. From a relatively small number of articles, we've tried to select the most interesting and diverse errors. You can rate how well we did in the comments :)

You can also check your own project at any time—I've left here a link to the page where you can download PVS-Studio. Enjoy the holidays, and see you in the new year!

Top comments (0)

Read next

adzhydra profile image

# Azure Functions Memory Issues: How to Handle Large Files (Complete Guide 2024)

Adam -

keploy profile image

Introduction: What Are Test Strategies?

keploy -

mayankchawdhari profile image

CID-FUNNY-LOGIN

Mayank Chawdhari -

mayankchawdhari profile image

Funny-Captcha Web

Mayank Chawdhari -