Hi to all fans of bugs! The New Year is coming soon, so it is time to take stock of the the outgoing year. By tradition, we're glad to present the top list of errors found by the PVS-Studio team in open C# projects in 2019. Ready? Then let's get going.

Tenth place «Fool everyone»

V3066 Possible incorrect order of arguments passed to 'AdjustCellBorderStyle' method: 'isFirstDisplayedRow' and 'isFirstDisplayedColumn'. DataGridViewComboBoxCell.cs 1934

protected override void OnMouseMove(DataGridViewCellMouseEventArgs e)
{
  ....
  dgvabsEffective = AdjustCellBorderStyle(
    DataGridView.AdvancedCellBorderStyle,
    dgvabsPlaceholder,
    singleVerticalBorderAdded,
    singleHorizontalBorderAdded,
    isFirstDisplayedRow,      // <=
    isFirstDisplayedColumn);  // <=
  ....
}

The error from the article "WinForms: Errors, Holmes". The analyzer points out that when two last arguments of the method are mixed up. Let's look at the AdjustCellBorderStyle declaration:

public virtual DataGridViewAdvancedBorderStyle AdjustCellBorderStyle(
  DataGridViewAdvancedBorderStyledataGridViewAdvancedBorderStyleInput,
  DataGridViewAdvancedBorderStyle dataGridViewAdvancedBorderStylePlaceholder,
  bool singleVerticalBorderAdded,
  bool singleHorizontalBorderAdded,
  bool isFirstDisplayedColumn,
  bool isFirstDisplayedRow)
{
  ....
}

Looks like the analyzer is right. Often developers intentionally pass some arguments in reverse order, for example, to swap around variables. But it doesn't look like that's the case. First, the bool type variables are mixed up. Secondly, there are no unusual methods' names: no «Swap» or «Reverse». Furthermore, it's not that difficult to make an error like this: people perceive differently the «line/column» collation order.

Ninth place «So close to eternity»

V3110 Possible infinite recursion inside 'TryValidateModel' method. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix)
{
  return TryValidateModel(model, Prefix(prefix));
}

The error from the article "Scanning the code of Orchard CMS for Bugs". There was a mistake that led to infinite recursion. To grasp the way the error was made, one has to consider the TryValidateModel method's overloading:

public bool TryValidateModel(object model)
{
  return _updateModel.TryValidateModel(model);
}

It is likely that the first case should also use such a call:

public bool TryValidateModel(object model, string prefix)
{
  return _updateModel.TryValidateModel(model, Prefix(prefix));
}

The code compiled successfully, because _updateModel is of the IUpdateModel type and the current class also implements the IUpdateModel interface.

Eighth place «Find me if you can»

V3091 Empirical analysis. It is possible that a typo is present inside the string literal: «Management Group Id». The 'Id' word is suspicious. Constants.cs 36

public class HelpMessages
{
  public const string SubscriptionId = "Subscription Id of the subscription
                                        associated with the management";
  public const string GroupId = "Management Group Id";       // <=
  public const string Recurse = "Recursively list the children of the
                                 management group";
  public const string ParentId = "Parent Id of the management group";
  public const string GroupName = "Management Group Id";     // <=
  public const string DisplayName = "Display Name of the management group";
  public const string Expand = "Expand the output to list the children of the
                                management group";
  public const string Force = "Force the action and skip confirmations";
  public const string InputObject = "Input Object from the Get call";
  public const string ParentObject = "Parent Object";
}

The error from the article "Azure PowerShell: Mostly Harmless". The analyzer suspected the GroupName constant to be initialized by an incorrect string. There should be probably something like «Management Group Name». Criticality of this error is still questionable, but the error is definitely rare and is difficult to detect.

Seventh place «Just underlooked»

V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. GridModel.Selection.cs 107

internal partial class GridModel
{
  private void BuildCellSelectionRegions(....)
  {
    ....
    this.MergeCellSelectionRegions(selectedItemsInView
        .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
        .OrderBy(c => c.RowItemInfo.LayoutInfo.Line));
    }
}

The error from the article "Checking Telerik UI for UWP as a Way to Get Started with PVS-Studio". The result of the previous sorting will be lost due to a repeated call to OrderBy on the already sorted collection. One has to use ThenBy in this case:

this.MergeCellSelectionRegions(selectedItemsInView
    .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line)
    .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));

Such mistakes are made by inattention or ignorance. I think copy-paste is to blame here.

Sixth place «The code is documented», they said

V3009 It's odd that this method always returns one and the same value of 'true'. MaskedTextProvider.cs 1529

public bool Remove(out int testPosition,
  out MaskedTextResultHint resultHint)
{
  ....
  if (lastAssignedPos == INVALID_INDEX)
  {
    ....
    return true; // nothing to remove.
  }
  ....
  return true;
}

The error from the article "Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer". The method will always return true. Yes, it's an error, but there is another thing that's really curious. The method is followed by the detailed comment:

Removes the last character from the formatted string. (Remove last character in virtual string). On exit the out param contains the position where the operation was actually performed. This position is relative to the test string. The MaskedTextResultHint out param gives more information about the operation result. Returns true on success, false otherwise.

Pay attention to the last sentence. Who even reads these comments? Nevertheless, if we take it seriously, such an error is easily insinuated, for example, during refactoring or debugging. Well, the authors wanted to check the variant when the method's result is always true, but forgot to return everything back as it was.

Fifth place «Index me, now!»

V3102 Suspicious access to element of 'seq' object by a constant index inside a loop. XmlQueryRuntime.cs 738

public bool MatchesXmlType(IList<XPathItem> seq, ....)
{
  ....
  for (int i = 0; i < seq.Count; i++)
  {
    if (!CreateXmlType(seq[0]).IsSubtypeOf(....))
      return false;
  }

  return true;
}

The error from the article "Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer". When traversing the seq collection in the for loop, the developer mistakenly uses the access only to its first element on all iterations (index 0 instead of i).

Fourth place «Just a dollar short»

V3138 String literal contains potential interpolated expression. Consider inspecting: e. SSPIHandleCache.cs 42

internal static void CacheCredential(SafeFreeCredentials newHandle)
{
  try
  {
    ....
  }
  catch (Exception e)
  {
    if (!ExceptionCheck.IsFatal(e))
    {
      NetEventSource.Fail(null, "Attempted to throw: {e}");
    }
  }
}

The error from the article "Checking the .NET Core Libraries Source Code by the PVS-Studio Static Analyzer". Apparently, the string «Attempted to throw: {e}» should be interpolated. Due to missed $ character, line representation of the e exception won't be put in the string. As a result, the line will be used «as it is.»

Third place «There's no way out»

V3008 [CWE-563] The 'this.linker.s3.region' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 116, 114. AWSSDK.DynamoDBv2.Net45 S3Link.cs 116

public string Region 
{ 
  get 
  {
    ....
  } 
  set 
  {
    if (String.IsNullOrEmpty(value))
    {
      this.linker.s3.region = "us-east-1";
    }
    this.linker.s3.region = value; 
  } 
}

The error from the article "Searching for errors in the Amazon Web Services SDK source code for .NET". Return was missed in the body of the if block. As a result, the this.linker.s3.region variable will always get value, including an empty line and null.

Second place «Right dress!»

V3070 Uninitialized variable 'LANG_USER_DEFAULT' is used when initializing the 'LOCALE_USER_DEFAULT' variable. NativeMethods.cs 890

internal static class NativeMethods
{
  ....
  public static readonly int LOCALE_USER_DEFAULT =
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT);
  ....
}

The error from the article "WinForms: Errors, Holmes". The initialization order of class fields is confused. To calculate the value of the LOCALE_USER_DEFAULT field, the LANG_USER_DEFAULT field is used, which isn't initialized yet at the moment and is 0. The variable isn't used anywhere further in the code. To get what this error leads to, an entire test program containing methods from WinForms code was written. Instead of some constants used, their actual values were substituted for simplicity:

internal static class NativeMethods
{
  public static readonly int LOCALE_USER_DEFAULT = 
    MAKELCID(LANG_USER_DEFAULT);
  public static readonly int LANG_USER_DEFAULT = 
    MAKELANGID(0x00, 0x01);
  
  public static int MAKELANGID(int primary, int sub)
  {
    return ((((ushort)(sub)) << 10) | (ushort)(primary));
  }
  public static int MAKELCID(int lgid)
  {
    return MAKELCID(lgid, 0x0);
  }
  public static int MAKELCID(int lgid, int sort)
  {
    return ((0xFFFF & lgid) | (((0x000f) & sort) << 16));
  }
}
class Program
{
  static void Main()
  {
    System.Console.WriteLine(NativeMethods.LOCALE_USER_DEFAULT);
  }
}

As a result of execution we will have the following: 0. Now let's fix the error by swapping declaration of fields LOCALE_USER_DEFAULT and LANG_USER_DEFAULT. Result of the program execution: 1024.

First place «First try, then trust»

It's never easy with the first place. There must be something extraordinary and captivating here. Initially, for this article I selected more than twenty interesting errors, but there was nothing worthy of the first place among them. That's when I recalled the article of my colleague Sergey Vasiliev. The article elaborated only on a single error. The beauty of this error is that it directly influenced the work of our analyzer. How? You can already get it from the article's title: "The story of how PVS-Studio found an error in the library used in… PVS-Studio". Here's where I felt too lazy to give the description of the error and thus would suggest that you follow the link and find out the details. :) I guarantee it's worth it. Moreover, the article is short.

Conclusion

I hope the errors were outstanding for you and the article was not tiring. Just for the record, you can always download the PVS-Studio analyzer to find bugs in your and third-party projects to please yourself, colleagues, and any Tom, Dick, or Harry. Let the errors be less, and time for self-improvement — more! :)

Did you read up to the end? My congrats on reaching the new level! Don't miss our upcomimg articles in our blog — best bugs in Java and C++ projects found in 2019.