Sergey Khrenov

Feb 27 2017

Tags:

#CSharp

Top 10 C# projects errors found in 2016

Feb 27 2017

Author: Sergey Khrenov

To measure the efficiency of our analyzer, and also to promote the methodology of static analysis, we regularly analyze open source projects for bugs and write articles about the results. 2016 was no exception. This year is especially important as it is the year of the "growth" of the C# analyzer. PVS-Studio has obtained a large number of new C# diagnostics, an improved virtual values mechanism (symbolic execution) and much more. Based on the results of our teamwork, I compiled a kind of chart of the most interesting bugs, found in various C# projects in 2016.

Tenth place: when a minute doesn't always have 60 seconds

I'll start the chart with a bug from the Orchard CMS project. The description of the error can be found in this article. In general, the full list of all the articles we checked is here.

V3118 Seconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182

void IBackgroundTask.Sweep()
{ 
  ....
  // Don't flood the database with progress updates; 
  // Limit it to every 5 seconds.
  if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5)
  {
     ....
  }
}

The developer mistakenly used Seconds instead of TotalSeconds in this case. Thus, we will get not the full number of seconds between the dates _clock.UtcNow and lastUpdateUtc, as the developer expected, but only the Seconds component of the interval. For example, for the time interval of 1 minute 4 seconds it will be not 64 seconds, but 4 seconds. Incredible, but even experienced developers make such mistakes.

Ninth place: the expression is always true

The following error is from the article about the analysis of GitExtensions.

V3022 Expression 'string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)' is always true. GitUI FormFormatPatch.cs 155

string rev1 = "";
string rev2 = "";

var revisions = RevisionGrid.GetSelectedRevisions();
if (revisions.Count > 0)
{
  rev1 = ....;
  rev2 = ....;
  ....
}
else

if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <=
{
    MessageBox.Show(....);
    return;
}

Note the else keyword. Most likely, this is not the right place for it. Inattention during refactoring, or just banal tiredness of a programmer, and we are getting a radical change in the logic of the program, which leads to unpredictable behavior. It's great that a static analyzer is never tired.

Eighth place: a possible typo

In the article about the analysis of FlashDevelop source code, we see a peculiar error, caused by a typo.

V3056 Consider reviewing the correctness of 'a1' item's usage. LzmaEncoder.cs 225

public void SetPrices(....)
{
    UInt32 a0 = _choice.GetPrice0();
    UInt32 a1 = _choice.GetPrice1();
    UInt32 b0 = a1 + _choice2.GetPrice0();  // <=
    UInt32 b1 = a1 + _choice2.GetPrice1();
    ....
}

I agree with the analyzer, as well as the author of the article. It feels like a0 should be used instead of the a1 variable in the marked line. In any case, it wouldn't hurt to give more meaningful names to variables.

Seventh place: logic error

This article was written based on the second check of the Umbraco project. An example of an interesting, in my opinion, error from it.

V3022 Expression 'name != "Min" || name != "Max"' is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415

private object Aggregate(....)
{
  ....
  if (name != "Min" || name != "Max")
  {
    throw new ArgumentException(
      "Can only use aggregate min or max methods on properties
       which are datetime");
  }
  ....
}

An exception of the ArgumentException type will be thrown for any value of name variable. It's because of the erroneous use of the || operator instead of && in the condition.

Sixth place: incorrect loop condition

The article about the check of Accord.Net has a description of several amusing bugs. I have chosen two, one of which is related to a typo again.

V3015 It is likely that the wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' Accord.Audio SampleConverter.cs 611

public static void Convert(float[][] from, short[][] to)
{
  for (int i = 0; i < from.Length; i++)
    for (int j = 0; i < from[0].Length; j++)
      to[i][j] = (short)(from[i][j] * (32767f));
}

There is an error in the condition of the second for loop, whose counter is the j variable. Using the variable names i and j for the counters is some sort of classic of programming. Unfortunately, these variables are very similar looking, so the developers often make mistakes in such code. I do not think that in this case it is worth any recommendation to use more meaningful names. Nobody will do that anyway. So, here is another recommendation: use static analyzers!

Fifth place: using a bitwise operator instead of a logical operator

One more interesting and quite a wide-spread error from the article about the analysis of Accord.Net.

V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461

public JaggedSingularValueDecompositionF(....)
{
  ....
  if ((k < nct) & (s[k] != 0.0))
  ....
}

It is obvious that even if the first condition is true, the erroneous use of & instead of && will lead to the check of the second condition, which in turn, will cause array index out of bounds.

Fourth place: quotation mark and... a quotation mark again

The fourth place is taken by the Xamarin.Forms project; the full article about its check is here.

V3038 The first argument of 'Replace' function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na)
{
  ....
  output.Write("string('{0}')",
    NRefactory.CSharp
.TextWriterTokenWriter
.ConvertString(
(string)na.Argument.Value).Replace("'", "\'"));
  ....
}

In this case, the quotation mark will be replaced with... a quotation mark. I do not think that this is what the developer wanted.

The analyzer did a great job!

Third place: ThreadStatic

In third place, is the Mono project that was also rich in amusing bugs. Here is the article about its check. One of these bugs is a real rarity.

V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16

static class Profiler
{
  [ThreadStatic]
  private static Stopwatch timer = new Stopwatch();
  ....
}

In a nutshell: the field marked by the ThreadStatic attribute is initialized incorrectly. In the documentation of the diagnostic you may see a detailed description of the situation, and some tips on how to avoid such errors. It's the perfect example of an error that is not that easy to find and fix using the usual methods.

Second place: Copy-Paste - classic!

One of the classical, to my mind, errors of the Copy-Paste type which was found in the already mentioned Mono project (see the full article).

V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258

Here is a short code fragment, where this bug was found:

button_pressed_highlight = use_system_colors ?
                           Color.FromArgb (150, 179, 225) : 
                           Color.FromArgb (150, 179, 225);

You may ask: "Is it really such a huge mistake?" "Does this obvious mistake really deserve second place in the chart?" The thing is that this code fragment was purposefully formatted to make it more visible. And now imagine that you have the task of finding an error like this without any tools. So, those with weak nerves, we would ask not to look at this; here is a screenshot of a full code fragment with an error (click on the image to enlarge):

Apparently, this mistake can be done by any programmer, regardless of the qualification. Successfully finding such errors demonstrates the full power of static analysis.

First place: PVS-Studio

No, it's not an illusion. "PVS-Studio" really is written here. It took first place in our chart. Not because it is a great static analyzer, but because our team has ordinary people, who make simple human errors when writing code. This was the main topic of an article we wrote previously. With the help of PVS-Studio, we detected two errors in the code of PVS-Studio itself.

V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559

V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561

public void ProcessFiles(....)
{
  ....
  int RowsCount = 
    DynamicErrorListControl.Instance.Plog.NumberOfRows;
  if (RowsCount > 20000)
    DatatableUpdateInterval = 30000; //30s
  else if (RowsCount > 100000)
    DatatableUpdateInterval = 60000; //1min
  else if (RowsCount > 200000)
    DatatableUpdateInterval = 120000; //2min
  ....
}

The result of this code fragment (on condition that RowsCount > 20000) will always be a value DatatableUpdateInterval equal to 30000.

Fortunately, we have already done some work in this sphere.

Thanks to widely used incremental analysis in our team, the articles "bugs in PVS-Studio found by PVS-Studio" are very unlikely to appear.

Conclusion

I should note that anyone can make a personal chart of bugs found using a free version of PVS-Studio static analyzer for checking individual projects.

Download and try PVS-Studio: http://www.viva64.com/en/pvs-studio/

To purchase a commercial license, please contact us via email. You can also write to us to get a temporary license key for a comprehensive investigation of PVS-Studio, if you want to avoid the limitations of the demo version.

#CSharp