# Rechecking SharpDevelop: Any New Bugs?

PVS\-Studio analyzer is continuously improving, and the C\#\-code analysis module is developing most actively: ninety new diagnostic rules were added in 2016\. However, the best way to estimate the analyzer's efficiency is to look at the bugs it can catch\. It's always interesting, as well as useful, to do recurring checks of large open\-source projects at certain intervals and compare their results\. Today I will talk about the results of the second analysis of SharpDevelop project\.

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

## Introduction

The previous [article](https://pvs-studio.com/en/blog/posts/csharp/0359/) about the analysis results for [SharpDevelop](https://github.com/icsharpcode) was written by Andrey Karpov in November 2015\. We were only going through the testing stage of our new C\# analyzer then and were preparing for its first release\. However, with just the beta version on hand, Andrey successfully checked SharpDeveloper and found a few interesting bugs there\. After that, SharpDevelop was "laid on the shelf" to be used with a number of other projects solely within our team for testing new diagnostics\. Now the time has come to check SharpDevelop once again but with the more "brawny" version, [PVS\-Studio 6\.12](https://pvs-studio.com/en/pvs-studio/download/)\.

I downloaded the latest version of SharpDevelop's source code from [GitHub](https://github.com/icsharpcode/SharpDevelop)\. The project contains about one million lines of code in C\#\. At the end of the analysis, PVS\-Studio output 809 warnings: 74 first\-level, 508 second\-level, and 227 third\-level messages:

![0469_SharpDevelop/image2.png](https://import.viva64.com/docx/blog/0469_SharpDevelop/image2.png)

I will skip the Low\-level warnings because there is a high rate of false positives among those\. About 40% of the Medium\- and High\-level warnings \(582 in total\) were found to be genuine errors or highly suspicious constructs, which corresponds to 233 warnings\. In other words, PVS\-Studio found an average of 0\.23 errors per 1000 lines of code\. This rate indicates a very high quality of SharpDevelop project's code\. Many of the other projects show much worse results\.

The new check revealed some of the bugs found and described by Andrey in his previous article, but most of the errors are new\. The most interesting ones are discussed below\.

## Analysis results

**Canonical Copy\-Paste bug**

This error deserves its own standard in the [International Bureau of Weights and Measures](https://en.wikipedia.org/wiki/International_Bureau_of_Weights_and_Measures)\. It is also a vivid example of how useful static analysis is and how dangerous Copy\-Paste can be\.

**PVS\-Studio diagnostic message:** [V3102](https://pvs-studio.com/en/docs/warnings/v3102/) Suspicious access to element of 'method\.SequencePoints' object by a constant index inside a loop\. CodeCoverageMethodTreeNode\.cs 52

```cpp
public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[0];  // <=
      ....
    }
    ....
  }
  ....
}
```

The zero\-index element of the collection is accessed at each iteration of the _for_ loop\. I included the code fragment immediately following the condition of the _if _statement on purpose to show where the line used in the loop body was copied from\. The programmer changed the variable name _firstSequencePoint_ to _sequencePoint_ but forgot to change the expression indexing into the elements\. This is what the fixed version of the construct looks like:

```cpp
public override void ActivateItem()
{
  if (method != null && method.SequencePoints.Count > 0) {
    CodeCoverageSequencePoint firstSequencePoint =  
      method.SequencePoints[0];
    ....
    for (int i = 1; i < method.SequencePoints.Count; ++i) {
      CodeCoverageSequencePoint sequencePoint = 
        method.SequencePoints[i];
      ....
    }
    ....
  }
  ....
}
```

**"Find the 10 differences", or another Copy\-Paste**

**PVS\-Studio diagnostic message:** [V3021](https://pvs-studio.com/en/docs/warnings/v3021/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless NamespaceTreeNode\.cs 87

```cpp
public int Compare(SharpTreeNode x, SharpTreeNode y)
{
  ....
  if (typeNameComparison == 0) {
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return -1;
    if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
      return 1;
  }  
  ....
}
```

Both _if_ blocks use the same condition\. I can't say for sure what exactly the correct version of the code should look like in this case; it has to be decided by the program author\.

**Late null check**

**PVS\-Studio diagnostic message:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'position' object was used before it was verified against null\. Check lines: 204, 206\. Task\.cs 204

```cpp
public void JumpToPosition()
{
  if (hasLocation && !position.IsDeleted)  // <=
    ....
  else if (position != null)
    ....
}
```

The _position_ variable is used without testing it for _null_\. The check is done in another condition, in the _else_ block\. This is what the fixed version could look like:

```cpp
public void JumpToPosition()
{
  if (hasLocation && position != null && !position.IsDeleted)
    ....
  else if (position != null)
    ....
}
```

**Skipped null check**

**PVS\-Studio diagnostic message:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'mainAssemblyList' object was used after it was verified against null\. Check lines: 304, 291\. ClassBrowserPad\.cs 304

```cpp
void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  mainAssemblyList.Assemblies.Clear();  // <=
  ....
}
```

The _mainAssemblyList_ variable is used without a prior null check, while such a check can be found in another statement in this fragment\. The fixed code:

```cpp
void UpdateActiveWorkspace()
{
  var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
  if ((mainAssemblyList != null) && (activeWorkspace != null)) {
    ....
  }
  ....
  if (mainAssemblyList != null) {
    mainAssemblyList.Assemblies.Clear();
  }  
  ....
}
```

**Unexpected sorting result**

**PVS\-Studio diagnostic message:** [V3078](https://pvs-studio.com/en/docs/warnings/v3078/) Original sorting order will be lost after repetitive call to 'OrderBy' method\. Use 'ThenBy' method to preserve the original sorting\. CodeCoverageMethodElement\.cs 124

```cpp
void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .OrderBy(item => item.Column);  // <=
}
```

This code will sort the _SequencePoints_ collection only by the _Column_ field, which doesn't seem to be the desired result\. The problem with this code is that the second call to the _OrderBy_ method will sort the collection without taking into account the results of the previous sort\. To fix this issue, method _ThenBy_ must be used instead of the second call to _OrderBy_:

```cpp
void Init()
{
  ....
  this.SequencePoints.OrderBy(item => item.Line)
                     .ThenBy(item => item.Column);
}
```

**Possible division by zero**

**PVS\-Studio diagnostic message:** [V3064](https://pvs-studio.com/en/docs/warnings/v3064/) Potential division by zero\. Consider inspecting denominator 'workAmount'\. XamlSymbolSearch\.cs 60

```cpp
public XamlSymbolSearch(IProject project, ISymbol entity)
{
  ....
  interestingFileNames = new List<FileName>();
  ....
  foreach (var item in ....)
    interestingFileNames.Add(item.FileName);
  ....
  workAmount = interestingFileNames.Count;
  workAmountInverse = 1.0 / workAmount;  // <=
}
```

If the _interestingFileNames_ collection is found to be empty, a division by zero will occur\. I can't suggest a ready solution for this code, but in any case, the authors need to improve the algorithm computing the value of the _workAmountInverse_ variable when the value of the _workAmount_ variable is zero\.

**Repeated assignment**

**PVS\-Studio diagnostic message:** [V3008](https://pvs-studio.com/en/docs/warnings/v3008/) The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 204, 201\. WixDialogDesigner\.cs 204

```cpp
void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;  // <=
    WorkbenchWindow.ActiveViewContent = this;
  } finally {
    ignoreDialogIdSelectedInTextEditor = false;  // <=
  }
}
```

The _ignoreDialogIdSelectedInTextEditor_ variable will be assigned with the value _false_ regardless of the result of executing the _try_ block\. Let's take a closer look at the variable declarations to make sure there are no "pitfalls" there\. This is the declaration of _ignoreDialogIdSelectedInTextEditor_:

```cpp
bool ignoreDialogIdSelectedInTextEditor;
```

And here are the declarations of _IWorkbenchWindow_ and _ActiveViewContent_:

```cpp
public IWorkbenchWindow WorkbenchWindow {
  get { return workbenchWindow; }
}
IViewContent ActiveViewContent {
  get;
  set;
}
```

As you can see, there are no legitimate reasons for assigning another value to the _ignoreDialogIdSelectedInTextEditor _variable\. Perhaps the correct version of this construct should use the _catch _keyword instead of _finally_:

```cpp
void OpenDesigner()
{
  try {
    ignoreDialogIdSelectedInTextEditor = true;
    WorkbenchWindow.ActiveViewContent = this;
  } catch {
    ignoreDialogIdSelectedInTextEditor = false;
  }
}
```

**Incorrect search of a substring**

**PVS\-Studio diagnostic message:** [V3053](https://pvs-studio.com/en/docs/warnings/v3053/) An excessive expression\. Examine the substrings '/debug' and '/debugport'\. NDebugger\.cs 287

```cpp
public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/debugport") ||  // <=
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}
```

This code uses a serial search looking for the substring "/debug" or "/debugport" in the _systemStartOptions_ string\. The problem with this fragment is that the "/debug" string is itself a substring of "/debugport", so finding "/debug" makes further search of "/debugport" meaningless\. It's not a bug, but it won't harm to optimize the code:

```cpp
public bool IsKernelDebuggerEnabled {
  get {
    ....
    if (systemStartOptions.Contains("/debug") ||
     systemStartOptions.Contains("/crashdebug") ||
     systemStartOptions.Contains("/baudrate")) {
      return true;
    }
    ....
  }
}
```

**Exception\-handling error**

**PVS\-Studio diagnostic message:** [V3052](https://pvs-studio.com/en/docs/warnings/v3052/) The original exception object 'ex' was swallowed\. Stack of original exception could be lost\. ReferenceFolderNodeCommands\.cs 130

```cpp
DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw ex;  // <=
    }
  }
  ....
}
```

Executing the _throw ex_ call will result in overwriting the stack of the original exception, as the intercepted exception will be generated anew\. This is the fixed version:

```cpp
DiscoveryClientProtocol DiscoverWebServices(....)
{
  try {
    ....
  } catch (WebException ex) {
    if (....) {
      ....
    } else {
      throw;
    }
  }
  ....
}
```

**Using an uninitialized field in a class constructor**

**PVS\-Studio diagnostic message:** [V3128](https://pvs-studio.com/en/docs/warnings/v3128/) The 'contentPanel' field is used before it is initialized in constructor\. SearchResultsPad\.cs 66

```cpp
Grid contentPanel;
public SearchResultsPad()
{
  ....
  defaultToolbarItems = ToolBarService
    .CreateToolBarItems(contentPanel, ....);  // <=
  ....
  contentPanel = new Grid {....};
  ....
}
```

The _contentPanel_ field is passed as one of the arguments to the _CreateToolBarItems _method in the constructor of the _SearchResultsPad_ class\. However, this field is not initialized until it has been used\. It's not necessarily an error, given that the possibility of the _contentPanel _variable with the value of _null_ is taken into account in the body of the _CreateToolBarItems_ method and further on in the stack\. This code still looks very suspicious and needs to be examined by the authors\.

As I have already said, this article discusses far not all the bugs found by PVS\-Studio in this project but only those that seemed interesting to me\. The project authors are welcome to contact us to get a temporary license key for a more thorough analysis of their code\.

## Conclusion

PVS\-Studio did well again and revealed new interesting bugs during the second check of SharpDevelop project\. It means the analyzer knows how to do its job and can help make the world a bit better\.

Remember that you, too, can join us at any time by taking the opportunity of checking your own projects with the [free](https://pvs-studio.com/en/blog/posts/0457/) version of PVS\-Studio static analyzer\.

You can download PVS\-Studio [here](https://pvs-studio.com/en/pvs-studio/)\.

Please [email](https://pvs-studio.com/en/about-feedback/) us if you have any questions regarding the purchase of a commercial license\. You can also contact us to ask for a temporary license for deeper exploration of PVS\-Studio without the [limitations](https://pvs-studio.com/en/docs/manual/0009/) of the demo version\.