Sergey Vasiliev

Nov 28 2018

Tags:

#CSharp

The Fastest Reports in the Wild West - and a Handful of Bugs...

Nov 28 2018

Author: Sergey Vasiliev

What was checked?
Tpyos and whatnot
Conclusion

Microsoft is not the only company to go open source recently - other companies are following this trend too. This brings us, the developers of PVS-Studio, another wonderful opportunity to test our analyzer and see if it can find any interesting bugs to report to the project authors. Today we're going to look into the code of a project developed by a Fast Reports company.

What was checked?

FastReport is a report generator developed by Fast Reports. It is written in C# and compatible with .NET Standard 2.0+. The source code was recently uploaded to GitHub from where I downloaded it for analysis.

Reports can include text, pictures, lines, shapes, tables, barcodes, and so on. They can be one-page or multipage and include, besides the data, a cover page and a back page. The data can be obtained from XML, CSV, Json, MS SQL, MySql, Oracle, Postgres, MongoDB, Couchbase, RavenDB, SQLite.

There are different ways to create report templates: from code, as an XML file, using an online designer tool, or using FastReport Designer Community Edition.

If necessary, the libraries can be downloaded as NuGet packages.

For more details about the project and its features see the GitHub-page.

I didn't have any problems building the project. I built it in Visual Studio 2017 and then checked using the PVS-Studio plugin.

PVS-Studio is a static analyzer detecting bugs in code written in C, C++, C#, and Java. C# code can be analyzed using a special plugin for Visual Studio or from the command line using the utility PVS-Studio_Cmd.exe. You can also set up analysis on the build server or import analysis results into SonarQube.

Now, let's see what interesting bugs we've got this time.

Since the project is fairly small, there aren't going to be many typos and suspicious fragments. We'll take a look at the bugs pointed out by the analyzer and even try to reproduce some of them.

Tpyos and whatnot

Take a look at this method:

public override string ToString() {
  if (_value == null) return null;
  return this.String;
}

PVS-Studio warning: V3108 It is not recommended to return 'null' from 'ToSting()' method. Variant.cs 1519

Yes, returning null from overridden ToString() is not an error in itself but it's bad style anyway. This is pointed out, among other sources, in Microsoft documentation: Your ToString() override should not return Empty or a null string. Developers, not expecting that null will be returned as a ToString() returned value, might be unpleasantly surprised to find out that during the execution of the code given below, an exception ArgumentNullException will be thrown (on condition that an extension method for IEnumerable<T> is called).

Variant varObj = new Variant();
varObj.ToString().Contains(character);

You could pick on this example as a synthetic one, but it doesn't make any difference.

More than that, this code includes the following comments:

/// <summary>
/// Returns <see cref="String"/> property unless the value 
    on the right
/// is null. If the value on the right is null, returns "".
/// </summary>
/// <returns></returns>

Oops. Returning null instead of "".

Let's move on.

The library has a class called FastString, which is described as "Fast alternative of StringBuilder". This class actually contains a field of type StringBuilder. The constructors of FastString call the Init method to initialize the respective field.

Here is the code of one of the constructors:

public FastString()
{
  Init(initCapacity);
}

And here is the code of the Init method:

private void Init(int iniCapacity)
{
  sb = new StringBuilder(iniCapacity);
  //chars = new char[iniCapacity];
  //capacity = iniCapacity;
}

The field sb can be accessed using the StringBuilder property:

public StringBuilder StringBuilder
{
  get { return sb;  }
}

FastString has a total of 3 constructors:

public FastString();
public FastString(int iniCapacity);
public FastString(string initValue);

You already saw the body of the first constructor, so it should be clear what other two constructors do. Now, watch carefully. What do you think the following code does?

FastString fs = new FastString(256);
Console.WriteLine(fs.StringBuilder.Capacity);

Here's the answer:

Didn't expect that? Let's take a look at the body of the constructor in question:

public FastString(int iniCapacity)
{
  Init(initCapacity);
}

Our regular readers must have developed a good eye for defects like this. As for the analyzer, it definitely has a good eye (nose, logic - call it whatever you like) for them, and it did spot the bug: V3117 Constructor parameter 'iniCapacity' is not used. FastString.cs 434

By sheer luck, constant field initCapacity exists in the class. Thus, the Init method is called with it instead of iniCapacity, and the typo remains undetected.

private const int initCapacity = 32;

When using similar names, you should be very, very careful. You can find similar typos in projects regardless of the language in which they are written: C, C++, C#, Java.

Since we started talking about typos, let's make up a simple example and see what it will do:

static void Main(string[] args)
{
  TextObject textObj = new TextObject();
  textObj.ParagraphFormat = null;

  Console.WriteLine("Ok");
}

By now, you may suspect that it won't simply print "Ok".

But what exactly? Well, how do you like this one:

The problem is in the ParagraphFormat property and the use of similar names:

public ParagraphFormat ParagraphFormat
{
  get { return paragraphFormat; }
  set { ParagraphFormat = value; }
}

PVS-Studio warning: V3110 Possible infinite recursion inside 'ParagraphFormat' property. TextObject.cs 281

The ParagraphFormat property is a wrapper around the field paragraphFormat. Note that its get property accessor is correct, whereas the set property accessor has a sad typo in it: the value is written to that very property rather than the field, and you end up with recursion. Again, it's a bug that has to do with using similar names.

Here's another code fragment.

public override Run Split(float availableWidth, out Run secondPart)
{
  ....
  if (r.Width > availableWidth)
  {
    List<CharWithIndex> list = new List<CharWithIndex>();
    for (int i = point; i < size; i++)
      list.Add(chars[i]);
    secondPart = new RunText(renderer, word, style, list,
                             left + r.Width, charIndex);
    list.Clear();
    for (int i = 0; i < point; i++)
        list.Add(chars[i]);
    r = new RunText(renderer, word, style, list, left, charIndex);

    return r;
  }
  else
  {
    List<CharWithIndex> list = new List<CharWithIndex>();
    for (int i = point; i < size; i++)
        list.Add(chars[i]);
    secondPart = new RunText(renderer, word, style, list, 
                             left + r.Width, charIndex);
    list.Clear();
    for (int i = 0; i < point; i++)
        list.Add(chars[i]);
    r = new RunText(renderer, word, style, list, left, charIndex);
    return r;
  }
  ....
}

PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. HtmlTextRenderer.cs 2092

A bit of copy-paste here: no matter what the r.Width > availableWidth expression evaluates to, the code will do the same things in either branch. This can be fixed by either deleting the if statement or changing the logic of one of the branches.

public static string GetExpression(FindTextArgs args, 
                                   bool skipStrings)
{
  while (args.StartIndex < args.Text.Length)
  {
    if (!FindMatchingBrackets(args, skipStrings))
      break;
    return args.FoundText;
  }
  return "";
}

PVS-Studio warning: V3020 An unconditional 'return' within a loop. CodeUtils.cs 262

Because of the unconditional return statement at the end of the loop, it will iterate only once at most. Perhaps it has to do with bad refactoring, or it's just an unconventional way to do what could be done without using a loop.

private int FindBarItem(string c)
{
  for (int i = 0; i < tabelle_cb.Length; i++)
  {
    if (c == tabelle_cb[i].c)
      return i;
  }
  return -1;
}
internal override string GetPattern()
{
  string result = tabelle_cb[FindBarItem("A")].data + "0";

  foreach (char c in text)
  {
    int idx = FindBarItem(c.ToString());
    result += tabelle_cb[idx].data + "0";
  }
      
  result += tabelle_cb[FindBarItem("B")].data;
  return result;
}

PVS-Studio warning: V3106 Possible negative index value. The value of 'idx' index could reach -1. BarcodeCodabar.cs 70

This code is unsafe. The FindBarItem method will return the value -1 if it fails to find the element passed to it as an argument. The caller (method GetPattern) writes this value to the idx variable, which is then used as an index for the tabelle_cb array without any prior check. And if you use -1 to index into an array, you'll get an IndexOutOfRangeException.

Moving on.

protected override void Finish()
{
  ....
  if (saveStreams)
  {
    FinishSaveStreams();
  }
  else
  {
    if (singlePage)
    {
      if (saveStreams)
      {
        int fileIndex = GeneratedFiles.IndexOf(singlePageFileName);
        DoPageEnd(generatedStreams[fileIndex]);
      }
      else { .... }
      ....
     }
     ....
  }
  ....
}

PVS-Studio warning: V3022 Expression 'saveStreams' is always false. HTMLExport.cs 849

The given code with obtaining fileIndex value and call of DoPageEnd method will never be executed. The reason for it is that the result of the second expression saveStreams will always be false.

Those were the most interesting bugs for today (you didn't expect this one to be the size of the article on Mono, did you?). There were other warnings as well, but they didn't look cool enough to me to include in this article (some warnings are always left out).

To reliably interpret those other warnings, one should know the project, so ideally, the developers of FastReport should take a look at them for themselves. Those warnings include V3083 (unsafe invocation of event), V3022 (always true / false condition (in this case, mostly because of methods returning the same value)), V3072, V3073 (incorrect use of members implementing IDisposable interface), and so on.

If some of the warnings are irrelevant, you can do the following:

turn the corresponding diagnostic off if you know for sure that the pattern it detects is correct in your project;
mark the warnings as false positives if there aren't too many of them;
add the warnings to the suppression base if you don't feel like adding multiple comments to the code. But in that case, you'll need to make sure that everyone who uses the analyzer has access to the suppression base.

Conclusion

Even though there weren't many bugs to include in this article, I really enjoyed the "feel" of the warnings, watching the bugs that the analyzer gets mad at show up in real code.

I wish the authors of FastReport good luck with developing their project and fixing the bugs, and many kudos to them for taking this step towards the open-source community!

And you guys are welcome to try the analyzer with your code and see if it can find anything interesting.

All the best!

#CSharp