Sergey Khrenov

Jul 19 2017

Tags:

#CSharp #OS

We Continue Exploring Tizen: C# Components Proved to be of High Quality

Jul 19 2017

Author: Sergey Khrenov

Introduction
Selecting Test Data
Analysis Results
- Internal C# Tizen Components
- Tizen Components on the Xamarin.Forms Base
Statistics
Conclusions
Additional Links

This time I go back again to the check of the Tizen project. In my recent post "Experiment of Bug Detection in the Code of C #Components of Tizen" in our blog, I analyzed the code of C# superficially and came to a conclusion that it makes sense to check the whole code of C# components of this project for errors using PVS-Studio and write the article about it. Right away, I would like to share with you the results of the work that I have done. I shall tell at once that PVS-Studio analyzer showed itself not on the bright side on C# code. Anyway, first things first: let's see what the analyzer found, and then we will deal with statistics and make conclusions.

Introduction

Recently, my colleague Andrey Karpov has published two epic articles about the code analysis of Tizen project, written in C and C++:

When I noticed that the Tizen project includes the code in C#, I felt like doing a similar article on the checking of components written in this language. Unfortunately, this time the analyzer missed a chance to show us the outstanding achievements, but let us take it slow and examine the issue in detail.

Selecting Test Data

The open-source code is available for download. The repository contains about 1 000 projects, each of them consists of the archive with the source code and the supporting files. It is not always possible to understand what is inside by the archive filenames or by the description. Therefore, a download, an extract and a review of archives had to be done for the entire repository.

In a previous article I gave the total number of C# source code files (4 929, excluding *.Designer.cs) and lines of code in them (about 691 000), that contained in the Tizen project. Now we need a more detailed analysis. For starters, we shall try to find the files with the .sln or .csproj extension. The availability of these files will allow us to undertake an analysis in IDE Visual Studio, making the work easier.

So, while searching, 227 solutions (*.sln) and 166 projects C# (*.csproj) were found. From the solutions files I chose the ones, which included C# projects. There were only three appropriate solutions:

Tizen.Xamarin.Forms.Extension.sln
Xamarin.Forms.Tizen.sln
Xamarin.Forms.sln

The first two solutions are the Tizen extension of the third-party component Xamarin.Forms, and the third one contains the component itself. Just over a year ago, we wrote the article about the check of Xamarin.Forms. In my work I will take into account these results and try to find new bugs.

Further on, after deleting files of the (*.csproj) projects, included in these solutions, I got 107 C# projects, that have not been connected with any solutions. Almost all of them are in top-level folders with the names of "csapi-*" type. After deleting 11 test projects as well as 9 projects, that had unsupported Visual Studio format from this number, I had got 87 projects left. I tested each of them separately.

For fairness of research I decided to separate the results, obtained for the internal C# components (those 87 projects), from the results of the check of components based on Xamarin.Forms. At first, I did not want to consider Xamarin.Forms, but, on reflection, I concluded, that once Tizen uses this component to its goals, then Xamarin.Forms bugs could influence Tizen.

I also will not describe the errors that I have already given in the previous article.

Analysis Results

Internal C# Tizen Components

During the check of this part of Tizen project the PVS-Studio analyzer generated 356 warnings, 18 of which are of High level of certainty, 157- of Medium level of certainty and 181 - of Low level of certainty. Approximately 325 000 lines of code were analyzed.

I did not consider the warnings of the Low level of certainty, as the percentage of false positives at this level is usually very large. Unfortunately, this time many false positives are not only at the Low level. Among 175 warnings of High and Medium levels, I found just 12 errors. Let's have a look at the most interesting of the detected errors.

PVS-Studio warning: V3008 The '_scanData' variable is assigned values twice successively. Perhaps, this is a mistake. Check lines: 138, 137. Tizen.Network.Bluetooth BluetoothLeAdapter.cs 138

CWE-563. Assignment to Variable without Use ('Unused Variable')

internal BluetoothLeDevice(BluetoothLeScanData scanData)
{
  _scanData = new BluetoothLeScanData ();
  _scanData = scanData;
  ....
}

The field _scanData is assigned a value twice. It looks very strange. Just in case, we will have a look at the BluetoothLeScanData class declaration and its constructor. Perhaps, the call of the constructor contains some additional actions. The class is small, so I will write it in one piece after formatting the original code:

internal class BluetoothLeScanData
{
  internal string RemoteAddress { get; set; }
  internal BluetoothLeDeviceAddressType AddressType { get; set; }
  internal int Rssi { get; set; }
  internal int AdvDataLength { get; set; }
  internal byte[] AdvData { get; set; }
  internal int ScanDataLength { get; set; }
  internal byte[] ScanData { get; set; }
}

As we can see, the class does not contain the explicitly defined default constructor, apparently, the double value assignment to the field _scanData is an error.

PVS-Studio warning: V3009 It's odd that this method always returns one and the same value of '0'. Tizen.Applications.WidgetApplication WidgetType.cs 47

CWE-393. Return of Wrong Status Code

private int OnCreate(....)
{
  WidgetBase b = Activator.CreateInstance(ClassType) as WidgetBase;
  ....  
  if (b == null)
    return 0;
  ....  
  return 0;
}

The method always returns 0, regardless of the result of its work. Probably, the error can be corrected, for example, like this:

private int OnCreate(....)
{
  WidgetBase b = Activator.CreateInstance(ClassType) as WidgetBase;
  ....  
  if (b == null)
    return 0;
  ....  
  return 1;
}

PVS-Studio warnings:

V3022 Expression '!LeftBoundIsForward' is always false. clipper_library clipper.cs 838
V3022 Expression '!LeftBoundIsForward' is always true. clipper_library clipper.cs 863

CWE-570/CWE-571 Expression is Always False/True

private TEdge ProcessBound(TEdge E, bool LeftBoundIsForward)
{
  ....
  if (LeftBoundIsForward)
  {
    ....
    if (!LeftBoundIsForward) Result = Horz.Prev;
    ....
  }
  else
  {
    ....
    if (!LeftBoundIsForward) Result = Horz.Next;
    ....
  }
  ....
}

This fragment of code contains two similar verifications at once. At the same time, in the first case the variable Result will never get the value Horz.Prev, and in the second case the same variable Result will always get the value Horz.Next. The author must carefully review the code and fix the bug himself.

PVS-Studio warning: V3022 Expression 'e.OutIdx >= 0' is always true. clipper_library clipper.cs 3144

CWE-571 Expression is Always True

private void DoMaxima(TEdge e)
{
  ....
  if(....)
  {
    ....
  } else if( e.OutIdx >= 0 && eMaxPair.OutIdx >= 0 )
  {
    if (e.OutIdx >= 0) AddLocalMaxPoly(e, eMaxPair, e.Top);
    ....
  }
  ....
}

Another fragment of code with an erroneous check. Perhaps, if in the condition e.OutIdx >= 0 && eMaxPair.OutIdx >= 0 the operator "||" was used, the check of e.OutIdx >= 0 in the attached block if, would make sense. Now it looks suspicious.

PVS-Studio warning: V3110 Possible infinite recursion inside 'InsertBefore' method. ElmSharp Toolbar.cs 288

CWE-674 Uncontrolled Recursion

public ToolbarItem InsertBefore(ToolbarItem before, string label)
{
  return InsertBefore(before, label);
}

The call of the InsertBefore method generates an infinite recursion. Probably, a bug appeared due to a call of the wrong overload of the method. In the code there is another InsertBefore method:

public ToolbarItem InsertBefore(ToolbarItem before, string label,
  string icon)
{
  ....
}

Perhaps, these are all the interesting bugs in this section. There are also several suspicious fragments of code, but I will not dwell on them. Code from Samsung Electronics, written in C#, shows good quality. Why am I so sure that the checked code has authorship of Samsung? Because each of the scanned files contained "Copyright (c) 2016 Samsung Electronics Co., Ltd All Rights Reserved" in its header comment.

Tizen Components on the Xamarin.Forms Base

The extension of Xamarin.Forms used in Tizen, contains approximately 242 000 lines of code. During its check the PVS-Studio analyzer generated 163 warnings, 10 of them of High level of certainty, 46 - of Medium level, and 107 - of Low level (not considered).

As promised, I will try to find errors that have not been described in the previous article about the check of Xamarin.Forms. By the way, some of the errors described in the article, were not found during a new check. Apparently, they were corrected after the authors' acquaintance with the article.

Despite the small number of generated warnings, I managed to find some new bugs.

PVS-Studio warning: V3095 The 'context' object was used before it was verified against null. Check lines: 16, 18. Xamarin.Forms.Xaml XamlServiceProvider.cs 16

CWE-476 NULL Pointer Dereference

internal XamlServiceProvider(INode node, HydratationContext context)
{
  ....
  if (node != null && node.Parent != null
    && context.Values.TryGetValue(node.Parent,  // <=
    out targetObject))
    IProvideValueTarget = new XamlValueTargetProvider(....);
  if (context != null)  // <=
    IRootObjectProvider =
    new XamlRootObjectProvider(context.RootElement);
  ....
}

The variable context is firstly used, and then verified against null.

PVS-Studio warning: V3095 The 'type' object was used before it was verified against null. Check lines: 147, 149. Xamarin.Forms.Xaml ExpandMarkupsVisitor.cs 147

CWE-476 NULL Pointer Dereference

public INode Parse(....)
{
  ....
  var xmltype = new XmlType(namespaceuri, type.Name, null);  // <=
  
  if (type == null)
    throw new NotSupportedException();
  ....
}

Another example of a possible throwing of NullReferenceException exception. The variable type is used to create the instance of the XmlType class, and then is verified against null.

Other similar errors:

V3095 The 'e.NewElement' object was used before it was verified against null. Check lines: 32, 46. Xamarin.Forms.Platform.Tizen MasterDetailPageRenderer.cs 32
V3095 The 'e.NewItems' object was used before it was verified against null. Check lines: 557, 567. Xamarin.Forms.Core Element.cs 557
V3095 The 'e.OldItems' object was used before it was verified against null. Check lines: 561, 574. Xamarin.Forms.Core Element.cs 561
V3095 The 'part' object was used before it was verified against null. Check lines: 135, 156. Xamarin.Forms.Core BindingExpression.cs 135

PVS-Studio warning: V3125 The 'e.NewItems' object was used after it was verified against null. Check lines: 999, 986. Xamarin.Forms.Core TemplatedItemsList.cs 999

CWE-476 NULL Pointer Dereference

void OnProxyCollectionChanged(....)
{
  ....
  if (e.NewStartingIndex >= 0 && e.NewItems != null)  // <=
    maxindex = Math.Max(maxindex, e.NewStartingIndex +
      e.NewItems.Count);
  ....
  for (int i = e.NewStartingIndex; i < _templatedObjects.Count; i++)
    SetIndex(_templatedObjects[i], i + e.NewItems.Count);  // <=
  ....
}

Here is a reverse situation. The variable e.NewItems is verified against null before using for the first time. However, during the second use one forgot to do it.

Statistics

As my colleague, Andrey Karpov, wrote in one of the previous articles, the PVS-Studio analyzer detects about 0.4 errors on 1000 lines of C/C++ code in Tizen project. Let's calculate what we get for C# code.

In total, 567 000 lines of code in C# were checked.

In my opinion, only 15 fragments of code were found, about which one can say that that they contain errors.

It turns out that PVS-Studio detects 0.02 errors on 1000 lines of code. Or, in other words, it finds 1 error on 50 000 lines of code. It is not too much.

We have to admit that in this case the analyzer was unable to demonstrate its usefulness. It is hard to say why it happened so. When checking for other open source projects the analyzer often showed good results. For example, when checking the open-sourced Unity3D components, the density of detected bugs was 0.5 errors on 1000 lines of code, i.e. it was 25 times better.

Perhaps, the checked components now are of high quality or the analyzer cannot find these types of errors, inherent to this project. Maybe the reason for it is the complicated inner architecture of Tizen. Very often the verified projects do not contain all the necessary environment and many links are missing, which does not allow some diagnostics to work out. Some projects cannot be verified at all.

Conclusions

So, the result of the test was not such as I expected for this size of code. Frankly speaking: I intended to find at least a few hundred errors but I found only about fifteen.

However, it is important to say, that the PVS-Studio analyzer coped with the task to analyze the C# code of Tizen project. Therefore, it may be useful, if not now, then later, when new components, written using C#, will appear in Tizen. Potential benefit is confirmed by the huge number of errors that the analyzer has already found in other open source projects (see list of articles).

Moreover, as we are not tired of repeating, the single checks using the analyzer are not optimal, as bugs have already been laid in the version control system, which is bad. It is much more efficient to use the static analyzer regularly, which will correct errors when coding, before falling into a version control system, because in such case, the cost and complexity of fixing them is much lower.