DEV Community: Nikita Lipilin

Top 10 bugs found in C# projects in 2021

Nikita Lipilin — Thu, 30 Dec 2021 10:30:07 +0000

In 2021 we published several articles and showed you errors found in open-source projects. The year 2021 ends, which means it's time to present you the traditional top 10 of the most interesting bugs. Enjoy!

A little introduction

As in the 2020 article, we ranged warnings according to the following principles:

there's a high probability that an error is present in code;
this error must be interesting, rare and unusual;
the warnings in the list must be diverse — you don't want to read about the same errors, right?

We must admit that there were few articles about C# projects check. The warnings in this list are often from the same projects. Somehow it happened that most of the warnings were taken from articles about DNN and PeachPie.

On the other hand, the errors found this year don't look alike — all the warnings were issued by different diagnostics!

With a heavy heart I crossed out warnings that were good but less interesting than others. Sometimes I had to cross out warnings for the sake of top variety. So, if you like reviews of the analyzer warnings, you can look at other articles. Who knows, maybe you'll be impressed by something I haven't written about. Comment with your own top 10 – I'll happily read them :).

10th place. Time's not changing

We start our top with a warning from the PeachPie article:

using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}

PVS-Studio warnings:

V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1232
V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1239

These warning say the same thing, so I decided to unite them.

The analyzer says that the call results should be written somewhere. Otherwise they just don't make sense. Methods like AddHours don't change the source object. Instead, they return a new object, which differs from the source one by the number of hours written in the argument call. It's hard to say how severe the error is, but the code works incorrectly.

Such errors are often related to strings, but sometimes you can meet them when working with other types. They happen because developers misunderstand the work of the "changing" methods.

9th place. The fourth element is present, but you'd better get an exception

The 9th place is for the warning from the Ryujinx article:

public uint this[int index]
{
  get
  {
    if (index == 0)
    {
      return element0;
    }
    else if (index == 1)
    {
      return element1;
    }
    else if (index == 2)
    {
      return element2;
    }
    else if (index == 2)   // <=
    {
      return element3;
    }

    throw new IndexOutOfRangeException();
  }
}

PVS-Studio warning: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 30. ZbcSetTableArguments.cs 26

Obviously, everything will be fine until someone wants to get the third element. And if they do so, an exception is thrown. That's okay, but why is there a never-running block with element3?

Surprisingly, situations caused by typos with numbers 0,1,2 are frequent in developing. There's a whole article about that — I highly recommend you read it. And we're moving on.

8th place. Useful Debug.WriteLine call

I took this warning from the PeachPie article mentioned above. It's fascinating that the code looks completely normal and not suspicious at all:

public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;

  ....
}

What's wrong with it? Everything looks okay. Assignments are made, then an overload of Debug.WriteLine is called. As a first argument, this overload takes... Wait! What's the correct order of the arguments here?

Well, let's look at the Debug.WriteLine declaration:

public static void WriteLine(string format, params object[] args);

According to the signature, the format string should be passed as the first argument. In the code fragment, the first argument is "MAILER", and the actual format goes into the args array, which doesn't affect anything at all.

PVS-Studio warns that all formatting arguments are ignored: V3025: Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st, 2nd, 3rd, 4th, 5th. Mail.cs 25

As a result, the output will simply be "MAILER" without any other useful information. But we'd like to see it :(.

7th place. Just one more question

The 7th place is again for the warning from PeachPie.

Often developers miss null checks. A particularly interesting situation is when a variable was checked in one place and wasn't in another (where it can also be null). Maybe developers forgot to do that or just ignored it. We can only guess whether the check was redundant or we need to add another check somewhere in code. The checks for null don't always require comparison operators: for example, in the code fragment below the developer used a null-conditional operator:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

Warning V3105: The 'tinfo' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Objects.cs 189

The developer thinks that the Type.GetTypeFromHandle(caller) call can return null. That's why they used "?." to call GetPhpTypeInfo. According to the documentation, it's possible.

Yes, "?." saves from one exception. If the GetTypeFromHandle call does return null, then null is also written to the tinfo variable. However, if we try to access the BaseType property, another exception is thrown. When I looked through the code, I came to the conclusion that another "?" is missing: return tinfo*?*.BaseType?.Name;

However, only developers can fix this problem. That's exactly what they did after we sent them a bug report. Instead of an additional null check they decided to explicitly throw an exception if GetTypeFromHandle returns null:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  // cannot be null; caller is either default or an invalid handle
  var t =    Type.GetTypeFromHandle(caller) 
          ?? throw new ArgumentException("", nameof(caller));

  var tinfo = t.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

We had to format the code for this article. You can find this method by following the link.

6th place. Week that lasted a day

Sometimes it seems that time slows down. You feel like a whole week has passed, but it's been only one day. Well, on the 6th place we have a warning from the DotNetNuke article. The analyzer was triggered by the code where a week contains only one day:

private static DateTime CalculateTime(int lapse, string measurement)
{
  var nextTime = new DateTime();
  switch (measurement)
  {
    case "s":
      nextTime = DateTime.Now.AddSeconds(lapse);
      break;
    case "m":
      nextTime = DateTime.Now.AddMinutes(lapse);
      break;
    case "h":
      nextTime = DateTime.Now.AddHours(lapse);
      break;
    case "d":
      nextTime = DateTime.Now.AddDays(lapse);   // <=
      break;
    case "w": 
      nextTime = DateTime.Now.AddDays(lapse);   // <=
      break;
    case "mo":
      nextTime = DateTime.Now.AddMonths(lapse);
      break;
    case "y":
      nextTime = DateTime.Now.AddYears(lapse);
      break;
  }
  return nextTime;
}

PVS-Studio warning: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118

Obviously, the function should return DateTime that corresponds to some point in time after the current one. Somehow it happened that the 'w' letter (implying 'week') is processed the same way as 'd'. If we try to get a date, a week from the current moment, we'll get the next day!

Note that there's no error with changing immutable objects. Still, it's weird that the code for branches 'd' and 'w' is the same. Of course, there's no AddWeeks standard method in DateTime, but you can add 7 days :).

5th place. Logical operators and null

The 5th place is taken by one of my favorite warnings from the PeachPie article. I suggest that you first take a closer look at this fragment and find an error here.

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         &&    langVersion.Major > 7 
            || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

Where's the problem?

I think you've easily found an error here. Indeed easy, if you know where to look :). I have to admit that I tried to confuse you and changed formatting a bit. In fact, the logical construction was written in one line.

Now let's look at the version formatted according to operator priorities:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

PVS-Studio warning V3080: Possible null dereference. Consider inspecting 'langVersion'. AnalysisFacts.cs 20

The code checks that the passed langVersion parameter is not null. The developer assumed that null could be passed when we call the IsAutoloadDeprecated method. Does the check save us?

No. If the langVersion variable is null, the value of the first part of the expression is false. When we calculate the second part, an exception is thrown.

Judging by the comment, either the priorities of operators were mixed up, or developers simply put the bracket incorrectly. By the way, this and other errors are gone (I believe) — we sent a bug report to the developers, and they quickly fixed them. You can see a new version of the IsAutoloadDeprecated function here.

4th place. Processing a non-existent page

We are almost close to the finalists. But before that — the 4th place. And here we have the warning from the last article about Umbraco. What do we have here?

public ActionResult<PagedResult<EntityBasic>> GetPagedChildren(....
                                                               int pageNumber,
                                                               ....)
{
  if (pageNumber <= 0)
  {
    return NotFound();
  }
  ....
  if (objectType.HasValue)
  {
    if (id == Constants.System.Root &&
        startNodes.Length > 0 &&
        startNodes.Contains(Constants.System.Root) == false &&
        !ignoreUserStartNodes)
    {
      if (pageNumber > 0)  // <=
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }
      IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, 
                                                  startNodes).ToArray();
      if (nodes.Length == 0)
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }

      if (pageSize < nodes.Length)
      {
        pageSize = nodes.Length; // bah
      }

      var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
      {
        Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
      };
      return pr;
    }
  }
}

PVS-Studio warning: V3022 Expression 'pageNumber > 0' is always true. EntityController.cs 625

So, pageNumber is a parameter that isn't changing inside the method. If its value is less than or equal to 0, we exit from the function. Further on, the code checks whether pageNumber is greater than 0.

Here we have a question: what value should we pass to pageNumber to make conditions pageNumber <= 0 and pageNumber > 0 false?

Obviously, there's no such value. If check pageNumber <= 0 is false, then pageNumber > 0 is always true. Is it scary? Let's look at the code after the always-true check:

if (pageNumber > 0)
{
  return new PagedResult<EntityBasic>(0, 0, 0);
}

IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, 
                                            startNodes).ToArray();
if (nodes.Length == 0)
{
  return new PagedResult<EntityBasic>(0, 0, 0);
}

if (pageSize < nodes.Length)
{
  pageSize = nodes.Length; // bah
}

var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
{
  Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
};
return pr;

Since the check in the beginning of this fragment is always true, the method always exits. And what about the code below? It contains a bunch of meaningful operations, but none of them is ever executed!

It looks suspicious. If pageNumber is less than or equal to 0, the default result is returned – NotFound(). Seems logical. However, if the parameter is greater than 0, we get... something that looks like the default result – new PagedResult(0, 0, 0). And how do we get a normal result? Unclear :(.

3d place. The rarest error

So, here are the finalists. The third place is for the V3122 diagnostic that haven't detected errors in open-source projects for a long time. Finally, in 2021 we checked DotNetNuke and found even 2 warnings V3122!

So, I present you the 3d place:

public static string LocalResourceDirectory
{
  get
  {
    return "App_LocalResources";
  }
}
private static bool HasLocalResources(string path)
{
  var folderInfo = new DirectoryInfo(path);

  if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
  {
    return true;
  }
  ....
}

PVS-Studio warning: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644

The developers convert the path value to lowercase. Then, they check whether it ends in a string that contains uppercase characters – "App_LocalResources" (the literal returned from the LocalResourceDirectory property). Obviously, this check always returns false and everything looks suspicious.

This warning reminds me that no matter how many errors we've seen, there's always something that can surprise us. Let's go further :).

2d place. There is no escape

The second place is for an excellent warning from the ILSpy article written in the beginning of 2021:

private static void WriteSimpleValue(ITextOutput output,
                                     object value, string typeName)
{
  switch (typeName)
  {
    case "string":
      output.Write(  "'"
                   + DisassemblerHelpers
                      .EscapeString(value.ToString())
                      .Replace("'", "\'")                   // <=
                   + "'");
      break;
    case "type":
    ....
  }
  ....
}

V3038 The '"'"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772

Seems like the developer wanted to replace all single quote character occurrences with a string consisting of two characters: a backslash and a single quote character. However, due to peculiarities with escape sequences, the second argument is just a single quote character. Therefore, no replacing here.

I came up with two ideas:

the developers forgot to put the '@' character before the second string. This character would just allow saving '\' as a separate character;
They should have put an additional '\' before the first one in the second argument. The first one would escape the second, which means the final string would have only one '\'.

1st place. The phantom menace

So, we have finally reached the most interesting and unusual error of 2021. This error is from the DotNetNuke article mentioned above.

What's even more interesting, the error is primitive, but the human eye misses errors like this one without static analysis tools. Bold statement? Well then, try to find an error here (if there is one, of course):

private void ParseTemplateInternal(...., string templatePath, ....)
{
  ....
  string path = Path.Combine(templatePath, "admin.template");
  if (!File.Exists(path))
  {
    // if the template is a merged copy of a localized templte the
    // admin.template may be one director up
    path = Path.Combine(templatePath, "..\admin.template");
  }
  ....
}

Well, how's it going? I won't be surprised if you find an error. After all, if you know it exists, you'll quickly see it. And if you didn't find — well, no surprise either. It's not so easy to see a typo in the comment — 'templte' instead of 'template' :).

Joking. Of course there is a real error the disrupts the program's work. Let's look at the code again:

private void ParseTemplateInternal(...., string templatePath, ....)
{
  ....
  string path = Path.Combine(templatePath, "admin.template");
  if (!File.Exists(path))
  {
    // if the template is a merged copy of a localized templte the
    // admin.template may be one director up
    path = Path.Combine(templatePath, "..\admin.template");
  }
  ....
}

PVS-Studio warning: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538

Here we have two operations to construct a path (the Path.Combine call). The first one is fine, but the second one is not. Clearly, in the second case, the developers wanted to take the 'admin.template' file not from the templatePath directory, but from the parent one. Alas! After they added ..\, the path became invalid since an escape sequence was formed: ..\admin.template.

Looks like the previous warning, right? Not exactly. Still, the solution is the same: add '@' before the string, or an additional '\'.

0 place. "lol' vs Visual Studio

Well, since the first element of the collection has index 0, our collection should also have 0 place!

Of course, the error here is special, going beyond the usual top. And yet it's worth mentioning, since the error was found in the beloved Visual Studio 2022. And what does static analysis have to do with it? Well, let's find an answer to it.

My teammate, Sergey Vasiliev, found this problem and described it in article "How Visual Studio 2022 ate up 100 GB of memory and what XML bombs had to do with it". Here I'll briefly describe the situation.

In Visual Studio 2022 Preview 3.1, a particular XML file added to a project makes the IDE lag. Which means everything will suffer along with this IDE. Here's an example of such a file:

<?xml version="1.0"?>
<!DOCTYPE lolz [
 <!ENTITY lol "lol">
 <!ELEMENT lolz (#PCDATA)>
 <!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
 <!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
 <!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
 <!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
 <!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
 <!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
 <!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
 <!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
 <!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
 <!ENTITY lol10 "&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;">
 <!ENTITY lol11 
   "&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;">
 <!ENTITY lol12 
   "&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;">
 <!ENTITY lol13 
   "&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;">
 <!ENTITY lol14 
   "&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;">
 <!ENTITY lol15 
   "&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;">
]>
<lolz>&lol15;</lolz>

As it turned out, Visual Studio was vulnerable to an XEE attack. Trying to expand all these lol entities, and IDE froze and ate up enormous amount of RAM. In the end, it just ate up all memory possible :(.

This problem was caused by the use of an insecurely configured XML parser. This parser allows DTD processing and doesn't set limitations on entities. My advice: don't read external files from unknown source with an XML parser. This will lead to a DoS attack.

Static analysis helps find such problems. By the way, PVS-Studio has recently introduced a new diagnostic to detect potential XEE — V5615.

We sent Visual Studio a bug report about that, and they fixed it in the new version. Good job, Microsoft! :)

Conclusion

Unfortunately, in 2021 we haven't written so many articles about real project checks. On the other hand, we wrote a number of other articles related to C#. You can find the links in the end of this article.

It was easy to choose interesting warnings for this top. But it wasn't easy to choose the 10 best ones since there were much more of them.

Rating them was also a hell of a task — the top is subjective, so don't take the places too close to heart :). One way or another, all these warnings are serious and once again remind us that we're doing the right thing.

Are you sure that your code doesn't have such problems? Are you sure that the errors don't hide between the lines? Perhaps, you can never be sure about that with a large project. However, this article shows that small (and not very small) errors can be found with static analyzer. That's why I invite you to try PVS-Studio on your projects.

Well, that's all. Happy New Year and see you soon!

Interesting articles in 2021

I have collected several articles that you can catch up with during long winter evenings :).

OWASP Top Ten and Software Composition Analysis (SCA)

Nikita Lipilin — Fri, 22 Oct 2021 14:50:24 +0000

One of the priority areas for PVS-Studio development is to cover categories from the OWASP Top Ten 2017 in the C# analyzer. We also plan to cover the Top Ten 2021 in the future. The most unusual for us is the A9:2017 category - Using Components with Known Vulnerabilities. This category has the A6 position in the preliminary version of OWASP 2021. The rule implementation for this category is an important task for our analyzer. It allows us to classify PVS-Studio as an SCA (Software Composition Analysis) tool. Which approach to implementation should we choose? Let's figure it out!

Using Components with Known Vulnerabilities

The A9 threat category (It turned into A6 in the preliminary OWASP 2021 version) is dedicated to using components with known vulnerabilities. These are the components that have the corresponding entries in the CVE database. CVE (Common Vulnerabilities and Exposures) is a database of records about real-life vulnerabilities in software, hardware, service components, etc.

A9 is quite atypical from the point of view of its coverage in PVS-Studio. That's because the existing analyzer architecture is designed to search for errors in the code itself. The architecture uses syntax trees, semantic model, various technologies like data-flow analysis and others. These technologies were generally sufficient to implement diagnostic rules that would cover certain categories from the OWASP Top Ten 2017.

For example, on the base of the existing data-flow mechanism we implemented taint analysis and various related diagnostic rules:

V5608 searches for SQL Injection;
V5609 searches for Path Traversal/Directory Traversal;
V5610 searches for potential XSS vulnerabilities;
and others.

Each of these rules searches for potential vulnerabilities in code and works by traversing a syntax tree. At the same time, they correspond to one or more OWASP Top Ten 2017 categories. You can find the full list of correspondences here.

The situation with A9 is completely different. From the point of view of C# projects, the rule implementation for A9 is a check of all the project dependency libraries for CVE. In other words, for each dependency, we need to check whether there is a corresponding entry in the CVE database.

This task goes far beyond the usual syntax tree traversal and the study of code semantics. However, we are determined to cover this category. Besides, it is very important that the implementation of the A9 rule would let PVS-Studio position the analyzer as an SCA solution.

Software Composition Analysis

In general, SCA tools are designed to check the project for problematic dependencies.

For example, if a project depends on an open source library, it is extremely important to take into account the license under which this library is distributed. Terms of use violations can cause huge damage to the business.

Another possible problem is the presence of vulnerabilities in the library. In the context of SCA, we are talking about known vulnerabilities — CVE. It's almost impossible to determine the use of a dependency that contains a non-recorded vulnerability :) It is not difficult to guess that if we use a library with a (publicly known) vulnerability, we can make a product vulnerable to various attacks.

Besides, using libraries whose maintenance was discontinued is a dangerous approach. Potentially, these dependencies also contain vulnerabilities. However, developers most likely don't know about them. Fixing such vulnerabilities is out of question — no one is going to do that.

SCA and PVS-Studio

We are gradually coming to the main question — how to implement the SCA functionality? First, we need to say that we are going to develop these features within the coverage of the A9:2017 category (Using Components with Known Vulnerabilities). Thus, we are going to search for dependencies with known vulnerabilities in the first place. However, the PVS-Studio analyzer already has diagnostic rules that warn developers about copyleft licenses:

V1042 for C++;
V3144 for C#;
V6071 for Java.

It is possible that over time we will implement other SCA features.

Detecting components with known vulnerabilities consists of two parts. The first step is to obtain all (both direct and transitive) project dependencies and then search for the CVEs that match them. The first part of this plan seems simple. The second part, though, is more difficult.

At the moment, we plan to implement the specified functionality for the C# analyzer. It's easy to obtain the list of dependencies for a C# project. Roslyn helps us a lot — our analyzer is built on its base. To be more precise, the main factor is the use of the same build platform (MSBuild) and a compiler for all C# projects. At the same time Roslyn is closely related to MSBuild. This makes obtaining the dependencies list trivial.

Since the ecosystem of C++ and Java is much more diverse, obtaining the dependencies list is going to be more difficult. We'll do this another time :).

Well, we got the dependencies from the project. How do we understand which of them have vulnerabilities? Besides, we need to keep in mind that the vulnerability may be relevant only for specific library versions. Obviously, we need some kind of database, where the dependencies, versions, and the corresponding CVEs would be stored.

The main question of implementation: how to find (or, perhaps, create) a database that allows us to compare the available information about project dependencies with specific CVE? The answer on that question depends on the tools you use.

Using a CPE open database

The first option we have studied is the approach used in OWASP Dependency Check. The approach is simple — for each dependency, this utility searches for a corresponding identifier in the CPE (Common Platform Enumeration) database. In fact, the CPE database is a list with information about products, their versions, vendors, and so on. To implement SCA, we must obtain CPE and CVE correspondences. Thus, getting a CVE list is just searching for the corresponding entry in the CPE database.

You can find the CPE database and CVE compliance on the official website National Vulnerability Database. One of the ways to get the necessary information is to use the Rest API. It's described here. For example, the following query allows us to get the first 20 elements of the CPE database including corresponding CVEs:

https://services.nvd.nist.gov/rest/json/cpes/1.0?addOns=cves

Below is an example of CPE for ActivePerl:

{
  "deprecated": false,
  "cpe23Uri": "cpe:2.3:a:activestate:activeperl:-:*:*:*:*:*:*:*",
  "lastModifiedDate": "2007-09-14T17:36Z",
  "titles": [
              {
                "title": "ActiveState ActivePerl",
                "lang": "en_US"
              }
            ],
  "refs": [],
  "deprecatedBy": [],
  "vulnerabilities": [ "CVE-2001-0815", "CVE-2004-0377" ]
}

The most important part here is the "cpe23Uri" value. It contains important information for us in a certain format, and, of course, "vulnerabilities" (although they are not a part of the CPE list). For simplicity we read the "cpe23Uri" string as

cpe:2.3:a:<vendor>:<product>:<version>:<update>:...

According to the specification, a hyphen in place of one of the fragments means logical "NA" value. As far as I understand, this can be interpreted as "the value is not set". The "*" character put in place of a fragment means "ANY".

When we implement a CPE-based solution, the main difficulty is to find the right element for each dependency. The problem here is that the library name (obtained when we parsed the project links) may not match the corresponding CPE entry. For example, the CPE list has entries with the following "cpe23Uri":

cpe:2.3:a:microsoft:asp.net_model_view_controller:2.0:*:*:*:*:*:*:*
cpe:2.3:a:microsoft:asp.net_model_view_controller:3.0:*:*:*:*:*:*:*
cpe:2.3:a:microsoft:asp.net_model_view_controller:4.0:*:*:*:*:*:*:*
cpe:2.3:a:microsoft:asp.net_model_view_controller:5.0:*:*:*:*:*:*:*
cpe:2.3:a:microsoft:asp.net_model_view_controller:5.1:*:*:*:*:*:*:*

After processing the entries, the analyzer concludes that they are all related to various versions of a product with the name "asp.net_model_view_controller" released by a company called Microsoft. All these entries correspond to a vulnerability with the CVE-2014-4075 identifier. However, the library in which the vulnerability was discovered is called "System.Web.Mvc". Most likely we'll get this name from the list of dependencies. In CPE, the name of the product is "Microsoft ASP.NET Model View Controller".

Besides, we need to take into account the vendor, whose identifier is an integral part of the CPE entries. There are also problems with this – the actual dependency does not always provide the necessary information in any form suitable for parsing. Not to mention the compliance of this information with any entry from CPE.

You can guess that the similar problems arise with the library version.

Another problem is that many records in the database are not relevant when we look for matches. Let's take as an example the entry given at the beginning of this section:

cpe:2.3:a:activestate:activeperl

ActivePerl is a distribution of the Perl language from ActiveState. The probability that something like this would be a dependency on a C# project... Well, is low. There are a lot of "unnecessary" (in the context of analyzing C# projects) entries. It's hard to say how we can teach the analyzer to distinguish them from the useful ones.

Despite the mentioned problems, the CPE-based approach can still be effective. Its implementation should be much trickier than a simple pair of string comparison. For example, the OWASP Dependency Check works in an interesting way. For each dependency, this tool collects evidence strings that can correspond to the vendor, product and version values from the desired CPE.

Using GitHub Advisory

We found another approach to searching for CVEs. We investigate GitHub Advisory to find the entries that correspond to the dependency we need to check. GitHub Advisory is a vulnerability database (CVE) discovered in open source projects that are stored on GitHub. The full list of positions is available here.

After we got acquainted with CPE, we understood that the method of recording data is extremely important when we choose the data source. We have to admit that in this case GitHub Advisory is much more convenient than CPE. Perhaps, this database was originally created for being used by various SCA tools. Anyway, various solutions like GitHub SCA and SCA by Microsoft use this database.

For programmatic access to GitHub Advisory, we need to use GraphQL. It's a powerful technology, but we must note that it's much easier to understand Rest API. Nevertheless, worn out by GitHub's GraphQL Explorer, I finally managed to make a query that outputs almost what I wanted. Namely, it outputs a list of packages and corresponding CVEs. Here's one of the elements I received:

{
  "identifiers": [
    {
      "value": "GHSA-mv2r-q4g5-j8q5",
      "type": "GHSA"
    },
    {
      "value": "CVE-2018-8269",
      "type": "CVE"
    }
  ],
  "vulnerabilities": {
    "nodes": [
      {
        "package": {
          "name": "Microsoft.Data.OData"
        },
        "severity": "HIGH",
        "vulnerableVersionRange": "< 5.8.4"
      }
    ]
  }
}

Obviously, I did not make the most optimal query, so I got a little extra information at the output.

If you're an expert in GraphQL, please write in the comments how you would construct a query that allows you to get a list of matches of this form: (package name, version) => CVE list.

Anyway, the query result clearly indicates the package name — the one that corresponds to this dependency in NuGet. The package name corresponds to CVE, and versions, for which the vulnerabilities are relevant. I'm sure that with a better understanding of this topic, we could easily create a utility that would automatically download all the necessary information.

We must say that selecting packages specifically for NuGet is a useful feature. In many cases (if not all) we would like to look for entries that correspond to a particular dependency among those packages. More specifically, we would like to do it without all the stuff for Composer, pip, etc.

Alas, but this solution has its flaws. At the time of writing this article, the GitHub Advisory had 4753 entries and only 140 NuGet packages. In comparison to the CPE database that contains more than 700 000 entries, this collection doesn't look that impressive. Although we must note that not all CPEs have corresponding CVEs. Plus the description implies that the GitHub Advisory database will contain information about vulnerabilities of GitHub-stored projects only. This awfully narrows the sample.

Nevertheless, the convenience of presenting vulnerabilities in this database at least makes us think about using it, if not as the main, then at least as one of the auxiliary data sources.

Our own database

Powerful SCA tools, such as Black Duck and Open Source Lifecycle Management, form and use their own databases. These databases, judging by the description, contain even more information than the National Vulnerability Database. Obviously, such databases present information in the most convenient form for the relevant tools.

Working on this direction, we have to transform the public data found about vulnerable components into some form convenient for our analyzer. We only need to find data convenient for such a transformation. Most likely, all SCA tools have their own databases of vulnerable components. However, not all of them contain information about vulnerabilities that are not in NVD or some other public source. One of the important distinguishing features of powerful SCA solutions is that they build their custom base that surpasses similar bases of other tools. Therefore, when working on the SCA implementation in PVS-Studio, we will take into account the need to expand our vulnerability base in the future.

Places where vulnerable components are used

It may seem that the implementation of the SCA functionality in PVS-Studio will require creation of something fundamentally new, without the possibility of using any of our existing developments. And, frankly, not in vain. The fact is that dependency analysis is a brand-new functionality and PVS-Studio doesn't have anything like this right now.

However, we have an idea how we can use the existing architecture to enhance our SCA implementation. Instead of simply making the analyzer trigger at the presence of a link to an unsafe library, we will try to look for its use in code. We have plenty of ready-made mechanisms for this :).

In my opinion, if the library is not even used, the analyzer still should warn about its presence among the dependencies. And if the library capabilities are somehow applied in the code, then the analyzer should issue a warning of the highest level. So far these are just thoughts.

As you see, we haven't decided what implementation approach to use. We haven't resolved some issues about it, too. For example: if we use a library with a vulnerability many times in project, should the analyzer issue a warning for each place of use? Or will the user drown in warnings? Should the analyzer issue one warning per file or should we simply raise the level if it detects the use of such library?

We have a lot of such questions about this solution. That's why we would like to know — how would YOU like to see SCA in PVS-Studio? How should an effective tool for finding problematic vulnerabilities work? What level should warnings have? Should we try to find other information sources about vulnerabilities? Should the analyzer trigger at transitive (indirect) dependencies?

Anyway, we are waiting for your comments. Thank you for your attention!

Is PHP compilable?! PVS-Studio searches for errors in PeachPie

Nikita Lipilin — Tue, 17 Aug 2021 09:24:49 +0000

PHP is widely known as an interpreted programming language used mainly for website development. However, few people know that PHP also has a compiler to .NET – PeachPie. But how well is it made? Will the static analyzer be able to find actual bugs in this compiler? Let's find out!

It's been a while since we posted articles on the C# projects check using PVS-Studio... And we still have to make the 2021 Top list of bugs (by the way, 2020 Top 10 bugs, you can find here)! Well, we need to mend our ways. I am excited to show you a review of the PeachPie check results.

To begin with, let me tell you a little about the project. PeachPie is a modern, open-source PHP language compiler and runtime for the .NET Framework and .NET. It is built on top of the Microsoft Roslyn compiler platform and is based on the first-generation Phalanger project. In July 2017, the project became a member of the .NET Foundation. The source code is available in the GitHub repository.

By the way, our C# analyzer also makes extensive use of the Roslyn capabilities, so in a way, PeachPie and PVS-Studio have something in common :). We've worked with Roslyn before. Moreover, we wrote a whole article about the basics of working with this platform.

To check PeachPie, we had to install the analyzer, open the project in Visual Studio or Rider and run the analysis using the PVS-Studio plugin. For more details, see the documentation.

It was entertaining to check such a large and serious project. I hope you'll also enjoy my review of the bugs found in PeachPie. Have fun reading!

WriteLine problems

Well, let's start with an easy one :) Sometimes bugs can appear in the most unexpected and at the same time the simplest places. For example, an error may even appear in an easy WriteLine function call:

public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;

  ....
}

The V3025 warning: Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st, 2nd, 3rd, 4th, 5th. Mail.cs 25

You would think, what has gone wrong? Everything seems to be fine. Wait a minute, though! What argument should pass the format?

Well, let's take a look at the *Debug.WriteLine *declaration:

public static void WriteLine(string format, params object[] args);

The format string should be passed as the first argument, and the first argument in the code is "MAILER". Obviously, the developer mixed up the methods and passed the arguments incorrectly.

Same cases in switch

This section is devoted to warnings associated with performing the same actions in different case branches:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.DisallowNullAttribute":
        result |= FlowAnalysisAnnotations.DisallowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullAttribute":
        result |= FlowAnalysisAnnotations.MaybeNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute":
        if (TryGetBoolArgument(attr, out bool maybeNullWhen))
        {
          result |= maybeNullWhen ? FlowAnalysisAnnotations.MaybeNullWhenTrue
                                  : FlowAnalysisAnnotations.MaybeNullWhenFalse;
        }
        break;
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
    }
  }
}

This fragment contains if not an error, then at least a strange thing. How quickly can you find it?

However, don't waste your time, the analyzer found everything for us:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      ....
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;              // <=
        break;
    }
  }
}

The V3139 warning: Two or more case-branches perform the same actions. ReflectionUtils.Nullability.cs 170

Isn't it strange that two different cases are handled the same way? In fact, no, this happens quite often. However, there are 2 peculiarities.

Firstly, it's worth noting that there is more graceful way to treat two different cases in the same way. You can rewrite the above fragment as follows:

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
}

However, developers often neglect this convenient method and prefer copy-paste. Therefore, the presence of two identical branches doesn't seem so terrible. The fact that the FlowAnalysisAnnotations enumeration has, among others, the FlowAnalysisAnnotations.NotNull *value is much more suspicious. This value seems to be used when the "System.Diagnostics.CodeAnalysis.NotNullAttribute"* value is processed:

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.NotNull;              // <=
    break;
}

Immutable DateTime

Developers often make mistakes because they don't understand how the features of the "modifying" methods work. Here is the bug found in PeachPie:

using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}

The PVS-Studio warnings:

V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1232
V3010 The return value of function 'AddHours' is required to be utilized. DateTimeFunctions.cs 1239

The analyzer reports that the results of calls should be recorded somewhere – otherwise, they do not make any sense. The fact is, methods like AddHours don't change the original object – instead, a new object is returned, and it differs from the original one accordingly. It's difficult to say how critical this mistake is, but it's clear that the code fragment doesn't work correctly.

Try-methods with peculiarities

Try-methods are often super convenient for developing apps in C#. The best-known try-methods are int.TryParse, Dictionary.TryGetValue, etc. Usually, these methods return a flag that indicates the operation success. The result is written to the out parameter. The PeachPie developers decided to implement their try-methods which were supposed to work the same way. What came of it? Let's look at the following code:

internal static bool TryParseIso8601Duration(string str,
                                             out DateInfo result,
                                             out bool negative)
{
  ....
  if (pos >= length) goto InvalidFormat;

  if (s[pos++] != 'P') goto InvalidFormat;

  if (!Core.Convert.TryParseDigits(....))
    goto Error;

  if (pos >= length) goto InvalidFormat;

  if (s[pos] == 'Y')
  {
    ....

    if (!Core.Convert.TryParseDigits(....)) 
      goto Error;

    if (pos >= length) goto InvalidFormat;
  }
  ....
  InvalidFormat:
  Error:

    result = default;
    negative = default;
    return false;
}

This method is shortened for readability. You can find the full method by clicking the link. Core.Convert.TryParseDigits is called many times in the method. In cases when such a call returns false, the thread of execution jumps to the *Error *label, which is logical.

On the Error label,* default values are assigned to out-parameters. Then, the method returns *false. Everything looks logical – the *TryParseIso8601Duration *method behaves exactly like standard try-methods. Well... At least, it's what it looks like. In fact, it's not like that :(.

As I mentioned earlier if Core.Convert.TryParseDigits *returns *false, the code jumps to the Error *label, where the bug/issue handling is performed. However, here's the trouble – the analyzer reports that *TryParseDigits never returns false:

The V3022 warning: Expression '!Core.Convert.TryParseDigits(s, ref pos, false, out value, out numDigits)' is always false. DateTimeParsing.cs 1440

If the negation of the call result is always false, then the call always returns true. What a specific behavior for the try-method! Is the operation always successful? Let's finally look at TryParseDigits:

public static bool TryParseDigits(....)
{
  Debug.Assert(offset >= 0);

  int offsetStart = offset;
  result = 0;
  numDigits = 0;

  while (....)
  {
    var digit = s[offset] - '0';

    if (result > (int.MaxValue - digit) / 10)
    {
      if (!eatDigits)
      {
        // overflow
        //return false;
        throw new OverflowException();
      }

      ....

      return true;
    }

    result = result * 10 + digit;
    offset++;
  }

  numDigits = offset - offsetStart;
  return true;
}

The method does always return true. But the operation may fail – in this case, an exception of the OverflowException type is thrown. As for me, this is clearly not what you expect from a try-method :). By the way, there is a line with return false, but it is commented out.

Perhaps, the use of an exception here is somehow justified. But according to the code, it seems that something went wrong. TryParseDigits and TryParseIso8601Duration using it are supposed to work like the usual try-methods – return false in case of failure. Instead, they throw unexpected exceptions.

Default argument value

The following analyzer message is simpler, but it also points to a rather strange code fragment:

private static bool Put(Context context,
                        PhpResource ftp_stream,
                        string remote_file,
                        string local_file,
                        int mode,
                        bool append,
                        int startpos)
{ .... }

public static bool ftp_put(Context context,
                           PhpResource ftp_stream,
                           string remote_file,
                           string local_file,
                           int mode = FTP_IMAGE,
                           int startpos = 0)
{
    return Put(context,
               ftp_stream,
               remote_file,
               local_file,
               mode = FTP_IMAGE, // <=
               false,
               startpos);
}

The V3061 warning: Parameter 'mode' is always rewritten in method body before being used. Ftp.cs 306

The ftp_put method accepts a number of parameters as input, one of the parameters is mode. It has a default value, but when it's called, clearly, you can set another value. However, this doesn't affect anything – mode is always overwritten, and the Put method always receives the value of the FTP_IMAGE constant.

It's difficult to say why everything is written this way – the construct seems meaningless. An error is most likely to be here.

Copy-paste sends greetings

The following code fragment looks like a copy-paste victim:

public static PhpValue filter_var(....)
{
  ....
  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_PRIV_RANGE)
  {
    throw new NotImplementedException();
  }

  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_RES_RANGE)
  {
    throw new NotImplementedException();
  }
  ....
}

The V3127 warning: Two similar code fragments were found. Perhaps, this is a typo and 'NO_RES_RANGE' variable should be used instead of 'NO_PRIV_RANGE' Filter.cs 771

It seems, the second condition had to be written this way:

(flags & (int)FilterFlag.NO_RES_RANGE) == (int)FilterFlag.NO_RES_RANGE

Anyway, this option looks more logical and clear.

Just an extra check in the if statement

Let's diversify our article with usual redundant code:

internal static NumberInfo IsNumber(....)
{
  ....
  int num = AlphaNumericToDigit(c);

  // unexpected character:
  if (num <= 15)
  {
    if (l == -1)
    {
      if (   longValue < long.MaxValue / 16 
          || (   longValue == long.MaxValue / 16 
              && num <= long.MaxValue % 16))         // <=
      {
        ....
      }
      ....
    }
    ....
  }
  ....
}

The V3063 warning: A part of conditional expression is always true if it is evaluated: num <= long.MaxValue % 16. Conversions.cs 994

Firstly, I'd like to say that the function code is significantly shortened for readability. Click on the link to see the full *IsNumber *source code – but let me warn you – it's not easy to read. The function contains more than 300 code lines. It seems to go beyond accepted "one screen" :).

Let's move on to the warning. In the outer block the value of the num variable is checked – it must be less than or equal to 15. In the inner block num is checked – it must be less than or equal to long.MaxValue % 16. In doing so, the value of this expression is 15 – it's easy to check. The code turns out to check twice that num is less than or equal to 15.

This warning hardly indicates a real bug – someone just wrote an extra check. Maybe it was done on purpose – for example, to ease the reading of this exact code. Although the use of some variable or constant to store the comparison result seems to be an easier option. Anyway, the construct is redundant, and it's the static analyzer duty to report this.

Could there be null?

Developers often miss checks for null. The situation is particularly interesting when a variable was checked in one place of the function, and in another (where it can still be null) – they forgot or didn't find it necessary. And here we can only guess whether the check was redundant or there was a lack of it in some places. Null checks do not always involve the use of comparison operators – for example, the code fragment below shows that the developer used the null-conditional operator:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

The V3105 warning: The 'tinfo' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Objects.cs 189

According to the developer, the Type.GetTypeFromHandle(caller) call can return null – that's why "?." was used to call GetPhpTypeInfo. The documentation proves that it's possible.

Yay, "?." saves from one exception. If the GetTypeFromHandle call returns null, then the tinfo variable* is also assigned *null. But when you try to access the BaseType property, another exception is thrown. Most likely, the last line misses another "?":

return tinfo?.BaseType?.Name;

Fatal warning and exceptions

Get ready, in this part you will find a real investigation...

Here we have another warning related to null check. The triggering turned out to be much more exciting than it looked at first glance. Take a look at the code fragment:

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context));
  }

  return context.HashAlgorithm;
}

The V3125 warning: The 'context' object was used after it was verified against null. Check lines: 3138, 3133. Hash.cs 3138

Yes, the variable is checked for null, and then the property access without any verification occurs. However, look what happens if the variable value is null:

PhpException.ArgumentNull(nameof(context));

It appears that if the context *is equal to *null, the execution thread doesn't get to the HashAlgorithm property access. Therefore, this code is safe. Is it a false positive?

Of course, the analyzer can make mistakes. However, I know that PVS-Studio can handle such situations – the analyzer should have known that at the time of accessing HashAlgorithm, the context variable cannot be equal to null.

But what exactly does the PhpException.ArgumentNull call do? Let's take a look:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

Hmm, looks like something is thrown. Pay attention to the first argument of the call — PhpError.Warning. Hmm, well, let's move on to the Throw method:

public static void Throw(PhpError error, string formatString, string arg0)
{
  Throw(error, string.Format(formatString, arg0));
}

Basically, there's nothing interesting here, take a look at another *Throw *overloading:

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

And here is what we are looking for! It turns out that under the hood of this entire system there is PhpFatalErrorException. The exception seems to be thrown occasionally.

Firstly, it's worth looking at the places where the handlers of the OnError event are registered. They can throw exceptions too – that would be a little unexpected, but you never know. There are a few handlers, and they all are related to logging the corresponding messages. One handler is in the PhpHandlerMiddleware file:

PhpException.OnError += (error, message) =>
{
  switch (error)
  {
    case PhpError.Error:
      logger.LogError(message);
      break;

    case PhpError.Warning:
      logger.LogWarning(message);
      break;

    case PhpError.Notice:
    default:
      logger.LogInformation(message);
      break;
  }
};

Two another handlers are in the PhpException class:

// trace output
OnError += (error, message) =>
{
  Trace.WriteLine(message, $"PHP ({error})");
};

// LogEventSource
OnError += (error, message) =>
{
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    LogEventSource.Log.HandleFatal(message);
  }
  else
  {
    LogEventSource.Log.HandleWarning(message);
  }
};

Thus, event handlers do not generate any exceptions. So, let's go back to the Throw method.

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

As everything is clear with OnError, let's take a closer look at the condition:

(error & (PhpError)PhpErrorSets.Fatal) != 0

The error parameter stores the value of the PhpError enumeration. Earlier, we noticed that the error parameter receives PhpError.Warning. An exception is thrown if the result of applying "bitwise AND" to the error *and *PhpErrorSets.Fatal is non-zero.

The *PhpErrorSets.Fatal *value is a "union" of the *PhpError *enumeration elements created by the "bitwise OR" operation:

Fatal =   PhpError.E_ERROR | PhpError.E_COMPILE_ERROR
        | PhpError.E_CORE_ERROR | PhpError.E_USER_ERROR

Below you can see the values of all the enumeration elements mentioned earlier:

E_ERROR = 1,
E_WARNING = 2,
E_CORE_ERROR = 16,
E_COMPILE_ERROR = 64,
E_USER_ERROR = 256,
Warning = E_WARNING

The error & (PhpError)PhpErrorSets.Fatal operation returns a non-zero value only if the error parameter has one of the following values or a combination of them:

PhpError.E_ERROR,
PhpError.E_COMPILE_ERROR,
PhpError.E_CORE_ERROR,
PhpError.E_USER_ERROR

If the error parameter contains the PhpError.Warning *value that equals *PhpError.E_WARNING, the result of the "bitwise AND" operation is zero. Then the condition for throwing PhpFatalErrorException is not met.

Let's go back to the PhpException.ArgumentNull method:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

We found out that when the PhpError.Warning *value is passed, there is no exception. Perhaps, the developer didn't want the exception to be thrown in cases when an unexpected *null is passed. It's just...

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context)); // no exceptions
  }

  return context.HashAlgorithm; // context is potential null
}

If PhpException.ArgumentNull does not throw an exception (which is unexpected), then when we access the HashAlgorithm property, NullReferenceException occurs anyway!

You might ask: should an exception be thrown or not? If it should, then it makes more sense to use the same PhpFatalErrorException. If no one expects an exception here, then you need to correctly process the null *value of the *context parameter. For example, you can use "?.". Anyway, the analyzer dealt with this situation and even helped to understand the issue.

Another extra check? An exception again!

The last case proves that expecting an exception, you can get an unexpected null. The fragment below shows the opposite case:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    return node.Value;
  else
    return PhpValue.Null;
}

The V3022 warning: Expression 'node != null' is always true. Datastructures.cs 432

Well, there's no null here, then so be it! Why grumble? However, usually null is expected in cases when something is wrong. The code shows that this is exactly the case. But the analyzer insists that there couldn't be null.

You might think, it's all about the *Debug.Assert *call in this case. For better or for worse, this call doesn't affect the analyzer warnings.

If it's not about Debug.Assert, then what is it about? Why does the analyzer "thinks" that node is never equal to null? Let's take a look at the GetNodeAtIndex method, that returns the value written to node:

private LinkedListNode<PhpValue> GetNodeAtIndex(PhpValue index)
{
  return GetNodeAtIndex(GetValidIndex(index));
}

Well, we go deeper. Take a look at the GetNodeAtIndex method called here:

private LinkedListNode<PhpValue> GetNodeAtIndex(long index)
{
  var node = _baseList.First;
  while (index-- > 0 && node != null)
  {
    node = node.Next;
  }

  return node ?? throw new OutOfRangeException();
}

Look! It seems that the method could return null... No such luck! If the loop is terminated, and node is equal to null, an exception is thrown. This way, no null can be returned.

In case of an unexpected situation, the GetNodeAtIndex method doesn't return null, as expected in the offsetGet method code:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset); // potential null expected

  Debug.Assert(node != null);

  if (node != null) // always true
    return node.Value;
  else
    return PhpValue.Null; // unreachable
}

When a developer reviews this method, they can easily get deceived. According to the code fragment, it seems that the correct value or PhpValue.Null is returned. In fact, this method can throw an exception.

The unexpected behavior of only one method in the call chain leads to unexpected behavior of all these methods – such a troublemaker! This example illustrates how useful static analysis is. It finds such problems automatically.

By the way, there is a similar problem in the *offsetSet *method from the same class:

public void offsetSet(PhpValue offset, PhpValue value)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    node.Value = value;
}

The V3022 warning: Expression 'node != null' is always true. Datastructures.cs 444

Assignments and reassignments

Why don't we take a little break from all these investigations and have a cup of coffee?

While we're drinking coffee, let's take a look at a simple warning that indicates a weird code fragment:

internal StatStruct(Mono.Unix.Native.Stat stat)
{
  st_dev = (uint)stat.st_dev;
  st_ctime = stat.st_ctime_nsec;
  st_mtime = stat.st_mtime_nsec;
  st_atime = stat.st_atime_nsec;
  st_ctime = stat.st_ctime;
  st_atime = stat.st_atime;
  //stat.st_blocks;
  //stat.st_blksize;
  st_mtime = stat.st_mtime;
  st_rdev = (uint)stat.st_rdev;
  st_gid = (short)stat.st_gid;
  st_uid = (short)stat.st_uid;
  st_nlink = (short)stat.st_nlink;
  st_mode = (FileModeFlags)stat.st_mode;
  st_ino = (ushort)stat.st_ino;
  st_size = stat.st_size;
}

The PVS-Studio warnings:

V3008 The 'st_ctime' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. StatStruct.cs 78
V3008 The 'st_atime' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 79, 77. StatStruct.cs 79

Looks like the developer got tangled in all these assignments and made a typo somewhere. Due to this, st_ctime and st_atime fields receive the values twice – and the second value is not the same as the first one.

It's an error, isn't it? But that's no fun! I suggest you practice your skills and search for deeper meaning. Then try to explain in the comments why everything is the way it is.

Meanwhile, let's keep going :)

These immutable strings...

At the very beginning of this article, when you were reading about the first warnings, we mentioned the immutability of DateTime structure instances. The following warnings remind us of a similar strings feature:

public TextElement Filter(IEncodingProvider enc,
                          TextElement input,
                          bool closing)
{
  string str = input.AsText(enc.StringEncoding);

  if (pending)
  {
    if (str.Length == 0) str = "\r";
    else if (str[0] != '\n') str.Insert(0, "\r"); // <=
  }

  str = str.Replace("\r\n", "\n");
  if (str.Length != 0)
  {
    pending = str[str.Length - 1] == '\r';

    if (!closing && pending) str.Remove(str.Length - 1, 1); // <=
  }


  return new TextElement(str);
}

The PVS-Studio warnings:

V3010 The return value of function 'Insert' is required to be utilized. Filters.cs 150
V3010 The return value of function 'Remove' is required to be utilized. Filters.cs 161

Everything is simple and clear – we wanted to modify the string, but something... went wrong :(.

or throw != or null

Recently, we analyzed a case when a developer expected the function* *to return *null *but got an exception instead. Here is something similar but simpler:

public static bool stream_wrapper_register(....)
{
  // check if the scheme is already registered:
  if (   string.IsNullOrEmpty(protocol)
      || StreamWrapper.GetWrapperInternal(ctx, protocol) == null)
  {
    // TODO: Warning?
    return false;
  }

  var wrapperClass = ctx.GetDeclaredTypeOrThrow(classname, true);
  if (wrapperClass == null) // <=
  {
    return false;
  }

  ....
}

The V3022 warning: Expression 'wrapperClass == null' is always false. Streams.cs 555

Of course, you can analyze it in detail, but... The method's name says it all! GetDeclaredTypeOrThrow sort of hints that it's going to throw an exception if something goes wrong. Again, here's the thing – this behavior is also passed to the stream_wrapper_register *method. But the developer wanted this method to return *false. No such luck, here is an exception!

In fact, we've already encountered deceptive names before. Do you remember when the PhpException.ArgumentNull *method call didn't actually throw an exception? So, let's check whether *GetDeclaredTypeOrThrow throws an exception:

PhpTypeInfo GetDeclaredTypeOrThrow(string name, bool autoload = false)
{
  return GetDeclaredType(name, autoload) ??
         throw PhpException.ClassNotFoundException(name);
}

Well, the PeachPie developers didn't try to trick you here – it is a real exception :).

Strange 'while true'

In some cases, developers use the true *value as the *while loop continuation condition. It seems to be a normal thing to do – to exit the loop, you can use break,* return,* or exceptions. Actually, the loop that has some expression (instead of the *true *keyword) as a condition looks far more than weird. The value of this expression always has the *true *value:

public static int stream_copy_to_stream(...., int offset = 0)
{
  ....
  if (offset > 0)
  {
    int haveskipped = 0;

    while (haveskipped != offset)  // <=
    {
      TextElement data;

      int toskip = offset - haveskipped;
      if (toskip > from.GetNextDataLength())
      {
        data = from.ReadMaximumData();
        if (data.IsNull) break;
      }
      else
      {
        data = from.ReadData(toskip, false);
        if (data.IsNull) break; // EOF or error.
        Debug.Assert(data.Length <= toskip);
      }

      Debug.Assert(haveskipped <= offset);
    }
  }
  ....
}

The V3022 warning: Expression 'haveskipped != offset' is always true. Streams.cs 769

The haveskipped variable is declared before the loop. It is initialized with the 0 value. This value remains with it... until its death. Sounds gloomy but it is what it is. In fact, haveskipped is a constant. The value of the *offset *parameter also remains the same during the loop performance. And it remains the same in any place of the function (you can check it here).

Did the developer plan to make the loop continuation condition always true? Theoretically, it's possible. But take a closer look at the loop. The following assignment looks strange:

int toskip = offset - haveskipped;

What's the point, if haveskipped is always equal to 0?

Something is wrong with the loop. Either a serious mistake is made here, or all these *haveskipped *weird things are the remains of some old unaccomplished ideas.

data == null && throw NullReferenceException

Often, the use of incorrect operators in conditions leads to bugs. There's a similar situation in the PHP compiler:

public string ReadStringContents(int maxLength)
{
  if (!CanRead) return null;
  var result = StringBuilderUtilities.Pool.Get();

  if (maxLength >= 0)
  {
    while (maxLength > 0 && !Eof)
    {
      string data = ReadString(maxLength);
      if (data == null && data.Length > 0) break; // EOF or error.
      maxLength -= data.Length;
      result.Append(data);
    }
  }
  ....
}

The V3080 warning: Possible null dereference. Consider inspecting 'data'. PhpStream.cs 1382

The value of the data variable is checked in the loop. If the variable equals null and its Length property has a positive value, then the loop exit occurs. Clearly, it's impossible. Moreover, we have an exception when accessing the Length *variable that has the *null value. Here, the access deliberately takes place when data = null.

Given the developer's comment, I would rewrite the condition something like this:

data == null || data.Length == 0

However, it doesn't mean that this is the correct handling option – to fix this issue, it's better to do deep code analysis.

Wrong exception

There are also bugs that do not look so terrible but still may cause problems. For example, in the following fragment, copy-paste hits again:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

The V3013 warning: It is odd that the body of 'addGlob' function is fully equivalent to the body of 'addPattern' function (506, line 515). ZipArchive.cs 506

The addGlob function clearly isn't supported, so when the function is called, there is an exception indicating that the addGlob function isn't supported.

Believing me? Tricked you! There is no exception here. This is our old friend – PhpException:

public static class PhpException
{
  ....
  public static void FunctionNotSupported(string/*!*/function)
  {
    Debug.Assert(!string.IsNullOrEmpty(function));

    Throw(PhpError.Warning,
          ErrResources.notsupported_function_called,
          function);
  }
  ....
}

As we discussed earlier if the *Throw *method receives the *PhpError.Warning *value, there is no exception. But still, the appeared error is likely to be added to log or handled in some other way.

Let's go back to the original code fragment:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

The addGlob function is not supported and when it's called, the corresponding message is handled somehow – let's assume that it is added to the log. The addPattern *function isn't supported either, however, the corresponding message is still addressed to *addGlob.

Clearly, it's a copy-paste error. It's easy to fix – you just need to report about addPattern, and not about addGlob in the addPattern method:

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addPattern));
  return false;
}

Don't blame String.Join!

Sometimes developers forget the features of some functions. That's why they check wrong values. As a result, the check turns out to be meaningless, and there is no check where it has to be. It seems that the same thing happened to the getallheaders function:

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    if (headers != null)
    {
      var result = new PhpArray(16);

      foreach (var h in headers)
      {
        result[h.Key] = string.Join(", ", h.Value) ?? string.Empty;
      }

      return result;
    }
  }

  return null;
}

The V3022 warning: Expression 'string.Join(", ", h.Value)' is always not null. The operator '??' is excessive. Web.cs 932

It's pointless to use the "??" operator here since the string.Join *method never returns *null. But it can always throw ArgumentNullException (You're welcome!).

string.Join *throws an exception if the passed reference to the sequence equals *null. Therefore, it's safer to write this line something like that:

result[h.Key] = h.Value != null ? string.Join(", ",h.Value) : string.Empty;

Actually, I want to know, whether it's possible for Value *to be *null at all? Maybe, we don't have to check anything here. To figure it out, firstly, we need to understand where the headers collection came from.

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    ....
  }

  return null;
}

The headers value is taken from webctx.requestHeaders, and the webctx *value is taken from the *HttpPhpContext property of the ctx object. And the *HttpPhpContext *property... Just take a look at this:

partial class Context : IEncodingProvider
{
  ....
  public virtual IHttpPhpContext? HttpPhpContext => null;
  ....
}

This, apparently, is something left for later. If you look at the getallheaders method again, you see that it never works at all and simply returns null.

Believing me again? But the property is virtual! Therefore, to understand what the getallheaders method can return, you need to analyze descendants. Personally, I decided to stop at this point – I still have to show other warnings.

Tiny assignment in a long method

Long and complex methods are likely to contain bugs. Over time, it's difficult for developers to navigate in a large chunk of code, while it's always terrifying to change it. Programmers add new code, the old one remains the same. Somehow this incredible construct works, thankfully. So, no surprise, there is some weirdness in such code. For example, take a look at the inflate_fast method:

internal int inflate_fast(....)
{
  ....
  int r;
  ....
  if (c > e)
  {
    // if source crosses,
    c -= e; // wrapped copy
    if (q - r > 0 && e > (q - r))
    {
      do
      {
        s.window[q++] = s.window[r++];
      }
      while (--e != 0);
    }
    else
    {
      Array.Copy(s.window, r, s.window, q, e);
      q += e; r += e; e = 0;                     // <=
    }
    r = 0;                                       // <=
  }
  ....
}

The V3008 warning: The 'r' variable is assigned values twice successfully. Perhaps this is a mistake. Check lines: 621, 619. InfCodes.cs 621

To begin with, here is a link to full code. The method has more than two hundred lines of code with a bunch of nested constructs. It seems that it would be difficult to puzzle it out.

The warning is unambiguous – first, a new value is assigned to the r variable in the block, and then it is definitely overwritten with zero. It's hard to say what exactly is wrong here. Either the nullifying works somehow wrong, or the *r += e *construction is superfluous here.

null dereference in a boolean expression

Earlier, we discussed the case when an incorrectly constructed logical expression leads to an exception. Here is another example of such a warning:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

The V3080 warning: Possible null dereference. Consider inspecting 'langVersion'. AnalysisFacts.cs 20

The code checks that the passed langVersion *parameter doesn't equal *null. So, the developer assumed that null could be passed during the call. Does the check save you from an exception?

Unfortunately, if the langVersion variable equals null, the value of the first part of the expression is false. When the second part is calculated, an exception is thrown.

Generally, to improve readability we need to additionally format the code fragments to post in an article. This case isn't an exception – the expression considered earlier, in fact, was written as one line:

Given the comment, you can easily understand that either the operator precedence is mixed up here, or the bracket is misplaced. The method is most likely to look as follows:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         && (   langVersion.Major > 7 
             || langVersion.Major == 7 && langVersion.Minor >= 2);
}

That's it!

Actually, no. The analyzer issued about 5 hundred warnings for the entire project, and there are many curious ones left waiting for the investigation. Therefore, I still suggest you try PVS-Studio and see what else it may find in this or other projects. Who knows, maybe you'll manage to find some bugs that are even more exciting than all warnings that I've sorted out here :). Don't forget to mention the found warnings in the comments. The bugs you found may get into the 2021 Top 10!

Wish you good luck!

.NET Application Optimization: Simple Edits Speeded Up PVS-Studio and Reduced Memory Consumption by 70%

Nikita Lipilin — Wed, 16 Jun 2021 14:05:09 +0000

We know many ways to detect performance problems, such as extremely low speed and high memory consumption. Usually tests, developers, or testers detect such applications' drawbacks. In the worst case, users find weaknesses and report back. Alas, detecting defects is only the first step. Next, we should localize the problem. Otherwise, we won't solve it. Here comes a question - how to find weak points that lead to excessive memory consumption and slow down in a large project? Are there such at all? Maybe it's not about the application? So now you're reading a story how PVS-Studio C# developers encountered a similar problem and managed to solve it.

Infinite Analysis

It takes some time to analyze large C# projects. It's not a surprise, since PVS-Studio plunges deep in source code and uses an impressive set of technologies: inter-procedural analysis, data flow analysis, etc. But still analysis takes no longer than a few hours even for many large projects we find on github.

Take Roslyn, for example. More than 200 projects in its solution! Almost all of them are in C#. Each project contains far more than one file. In turn, in files we see far more than a couple of code lines. PVS-Studio checks Roslyn in about 1.5-2 hours. No doubts, some of our users' projects require much more time for a check. But cases of one-day checks are exceptional.

This is what happened to one of our clients. He wrote to our support team that his project's analysis hasn't completed in... 3 days! Something was clearly wrong. We couldn't leave a problem like this unaddressed.

Wait, What About Testing?!

Surely the reader has a logical question - why didn't you spot the problem at the testing stage? How did you let a client reveal it? Isn't PVS-Studio C# analyzer tested by developers?

But we do test it head to toe! Testing is part and parcel of the development process for us. We constantly check the analyzer for correct operation as a whole, the same as we do for its individual parts. Unit tests of diagnostic rules and internal functions are literally a half of the total C# analyzer source code. What's more, every night the analyzer checks a large set of projects. Then we check if the analyzer's reports are correct. We automatically track both the analyzer's speed and the amount of memory consumed. Developers instantly react to more or less significant deviations - detect and look into them.

Sad but true - this whole pack of tests didn't help to keep the user out of the problem. Taken aback by what happened, with no time for regrets, our developers immediately began to investigate the case.

Searching for Reasons

Dump

We suggested the problem may have been due to some peculiarities of our client's project. We knew this project was quite large and complex, but that information was not enough - we lacked details.

A memory dump of the analyzer process could be of help. What is dump? In short, a dump is a segment of data from RAM. It helps us to find out what data is loaded into the memory space of the PVS-Studio process. First of all, we were looking for any defects that could cause a severe slowdown in work.

We asked the user to run the project analysis again, then wait a while, save the process dump and send it to us. No special programs or skills are needed for these actions - you can get the dump with a Task Manager.

If you can't open the dump file, it's of little use. Lucky for users, they don't have to deal with it :). As for us, we decided to review the dump data using Visual Studio. It is quite simple.

Open the project with application source files in Visual Studio.
In the top menu, click File->Open->File (or Ctrl+O).
Find the dump file and open it.

We see a window with different information about the process:

Mostly we'd like to know if we could switch to a kind of dump debugging mode. To do this, click Debug With Managed Only.

Note. If you'd like to learn more about opening dumps through Visual Studio for debugging, official documentation will definitely be of help.

So, we switched to the debugging mode. Debugging a dump file is a powerful mechanism. Still there are some limitations:

you can't resume the process, execute the code step-by-step and so on;
you can't use certain functions in the Quick Watch and Immediate Window. For example, the File.WriteAllText method call resulted in the exception "Caracteres no válidos en la ruta de acceso!". It is because the dump relates to the environment where it was taken.

We got a variety of data from the dump debugging. Below is a small part of data on the analysis process at the moment of taking the dump:

the number of files in the project: 1,500;
approximate analysis time: 24 hours;
the number of currently analyzed files at the moment: 12;
the number of files already checked: 1060.

We made some conclusions from working with the dump. The analyzer has checked most project files when the dump was taken. The slowdown became obvious by the end of the analysis. We had a hunch - factors leading to the slowdown may have accumulated.

Alas, we failed to figure out the reasons for the slowdown. There were no defects found, and the number of files in the project did not seem to be something out of the row. A similar project may be checked in about 2 hours.

Apart from the project size, structures' complexity also affects analysis time. We knew that many loops and high nesting levels lead to analysis slowdown. The dump file showed that the project did contain such fragments. But even the most complicated structure shouldn't have turned a two-hour analysis into... infinite!

Reproducing the Problem at Last

Using data from the dump, we realized that the analysis got stuck on specific files with complex code structure. We asked them from the client, hoping to reproduce the problem. This didn't happen when analyzing individual files.

We decided to go an extra mile and create our own test project with a lot of complex constructs. We had to reproduce the problem locally - this would greatly simplify further search for its solution.

We created our test project with the following specifications of the user's project:

the number of files;
the average file size;
the maximum level of nesting and complexity of the structures used.

With fingers crossed we ran the analysis and...

No slowdowns. After so much effort we were never able to reproduce the problem. The formed project kept completing successfully within normal times. No hangups, no errors, no defects. At this point one can think - maybe the user made fun of this?

We seemed to have tried everything and truth wouldn't come out. Actually we would be glad to deal with the slowdown problem! As well as to cope with it, please the client and congratulate ourselves. After all, our user's project mustn't hang up!

Customer support is a difficult job that sometimes require incredible tenacity. We kept digging. Over and over again we tried to reproduce the problem and suddenly... We did it.

The analysis couldn't complete on one of our colleague's computer. He was using the same analyzer version and the same project. What was the difference then?

Hardware was different. More precisely, RAM.

What Does This Have to Do with RAM?

Our automated tests run on a server with 32 GB of available RAM. Memory space varies on our employees' machines. It is at least 16GB, most have 32GB or more. The bug showed up on a laptop that had 8 GB of RAM.

Here comes a reasonable question - how does all this relate to our problem? We were solving the slowdown problem, not the one with high memory consumption!

In fact, the latter can really slow down the application. This occurs when the process lacks memory installed on the device. In such cases a special mechanism activates – memory paging (or "swapping"). When it works, part of the data from the RAM is transferred to the secondary storage (disk). If necessary, the system loads data from the disk. Thanks to this mechanism, applications can use more RAM than available on the system. Alas, this wizardry has its price.

It is remarkable reduction in the speed of work. Hard disk operations are much slower than working with RAM. It was swapping that slowed down the work of our analyzer hardest.

Basically, case solved. We could stop our investigation at this point. We could advise the user to increase the amount of available RAM and that's it. However, this would hardly satisfy the client, and we ourselves did not like this option at all. Therefore, we decided to delve into the issue of memory consumption in more detail.

Solving the Problem

dotMemory and Dominator Graph

We used the dotMemory app by JetBrains. This is a memory profiler for .NET. You can run it both directly from Visual Studio and as a separate tool. Among all features of dotMemory, we were most interested in profiling the analysis process.

Below is a window allowing you to attach to a process:

First, we need to start the appropriate process, then select it and start profiling with the "Run" button. A new window opens:

We can get a snapshot of memory status at any time. During the process, we can take several such snapshots - all of them will appear in the "Memory Snapshots" panel:

Next, we need to study the shot in detail. Click on its identifier to do this. In the opening window there are many different elements:

Official documentation provides more detailed information about working with dotMemory, including a detailed description of the data given here. The sunburst diagram was particularly interesting for us. It shows the hierarchy of dominators — objects that exclusively hold other objects in memory. Open the "Dominators" tab to go to it.

We did all these actions with the analysis process of the specially created test project. The dominator diagram for it looked like this:

The closer the element is to the center, the higher is the position of the corresponding class. For example, the only instance of the SemanticModelCachesContainer class is at a high level in the hierarchy of dominators. The diagram also shows child objects after the corresponding element. For example, in the picture you can see that the SemanticModelCachesContainer instance contains a link to ConcurrentDictionary within itself.

High-level objects were not particularly interesting - they did not take much space. The inside part was much more considerable. What objects multiplied so much that they started taking up so much space?

After an in-depth study of the data obtained, we finally discovered the cause of high memory consumption. The cache used by our data flow analysis mechanism was taking most of it.

Data-Flow Analysis evaluates possible variable values in different points of computer program. If a reference gets dereferenced and currently may be null, it is a potential error. The analyzer will report about it. This article will give you more details about this and other technologies used in PVS-Studio.

The cache stores calculated ranges of variable values to optimize operation. Unfortunately, this leads to a serious increase in the amount of memory consumed. Despite this, we can't remove the caching mechanism! Inter-procedural analysis will go much slower if we refuse from caching.

Then we can we do? Is it a dead end again?

They Are Not So Different

What do we have? Variable values are cached, and there are a lot of them. There are so many that the project is not checked even in 3 days. We still can't refuse caching these values. What if we somehow optimize the way they are stored?

We took a closer look at the values in the cache. PVS-Studio turned out to store a large number of identical objects. Here is an example. The analyzer can't evaluate values for many variables, because values may be any within their type constraints.

void MyFunction(int a, int b, int c ....)
{
  // a = ?
  // b = ?
  // c = ?
  ....
}

Each variable corresponded to its own value object. There was a whole bunch of such objects, but they did not differ from each other!

The idea came up instantly — we only had to get rid of duplication. True, the implementation would require us to make a large number of complex edits...

Well...Nope! In fact, it takes just a few:

a storage that will contain unique values of variables;
storage access mechanisms — adding new and retrieving existing elements;
handling some fragments related to new virtual values to the cache.

Changes in certain parts of the analyzer usually involved a couple of lines. The repository implementation didn't take long either. As a result, the cache began to store only unique values.

You probably know the approach I describe. What we did is an example of the famous Flyweight pattern. Its purpose is to optimize the work with memory. How does it work? We have to prevent the creation of element instances that have a common essence.

String internment comes to mind in this context as well. In fact, it is the same thing. If strings are the same in value, they will actually be represented by the same object. In C#, string literals intern automatically. For other strings, we can use String.Intern and String.IsInterned methods. Bit it's not that simple. Even this mechanism must be used wisely. If you're interested in the topic, the article "Hidden Reefs in String Pool, or Another Reason to Think Twice Before Interning Instances of String Class in C#" will be right for you.

Memory Gained

We made a few minor edits by implementing the Flyweight pattern. What about the results?

They were incredible! Peak RAM consumption during test project check decreased from 14.55 to 4.73 gigabytes. Such a simple and fast solution allowed to reduce memory consumption by about 68%! We were shocked and very pleased with the result. The client was excited as well - now the RAM of his computer was enough. This means the analysis began to take normal time.

True, the result was rewarding, but...

We Need More Optimizations!

Yes, we managed to reduce memory consumption. Yet initially we wanted to speed up the analysis! Well, our client did have a speed boost, just like other machines that lacked RAM. But we didn't get speed up on our high-capacity machines - we only reduced memory consumption. Since we got so deep in the rabbit hole... Why not continue?

dotTrace

So, we started looking for optimization potential. First of all, we were wondering — which parts of the app work the longest? Exactly what operations waste time?

dotTrace, a decent performance profiler for .NET applications, could give answers to our questions and provide a number of interesting features. This application's interface quite strongly resembles dotMemory:

Note. As with dotMemory, this article will not give a detailed guide how to use dotTrace work with this application. Documentation is here to help you with details. My story is about actions we made to discover optimization opportunities.

Using dotTrace, we ran an analysis of one large project. Below is the window example that displays real-time graphs of memory and CPU usage:

To start "recording" data about the application, press Start. By default, the data collection process starts immediately. After a while, click "Get Snapshot And Wait". A window with collected data opens. For example, for a simple console application, this window looks like this:

Here we have a lot of different information available. First of all, it is the working time of individual methods. It may also be useful to know the running time of threads. You can view the general report as well. To do this, click View->Snapshot Overview in the top menu or use the combination Ctrl+Shift+O.

Tired Garbage Collector

What did we find out with dotTrace? Once again we made sure that C# analyzer doesn't use even half of the CPU power. PVS-Studio C# is a multi-thread application. In theory, the load on the processor should be notable. Despite this, during analysis, the CPU load often fell to 13— 15% of the CPU's total power. Obviously we're working inefficiently. Why?

dotTrace showed us an amusing thing. It is not even the application itself that works most of the time. It is the garbage collector! A logical question arises - how is that?

The fact is that garbage collection was blocking analyzer threads. After the completed collection the analyzer does a little work. Then garbage collection starts again, and PVS-Studio "rests".

We got the main point of the problem. The next step was to find places where memory allocates for new objects most actively. Then we had to analyze all found fragments and make optimization changes.

It's Not Our Fault, It's All Their DisplayPart!

The tracer showed that most often memory is allocated to objects of DisplayPart type. At the same time, they exist for a short time. This means they require frequent memory allocation.

We might opt out of using these objects if it weren't for one caveat. DisplayPart is not even mentioned in the source files of our C# analyzer! As it turns out, this type plays a special role in the Roslyn API we use.

Roslyn (or .NET Compiler Platform) is the basis of PVS-Studio C# analyzer. It provides us with ready-made solutions for a number of tasks:

converts a source file into a syntax tree;
a convenient way to traverse the syntax tree;
obtains various (including semantic) information about a specific node of the tree;
and others.

Roslyn is an open source platform. This made it easy to understand what *DisplayPart *is and why this type is needed at all.

It turned out that DisplayPart objects are actively used when creating string representations of so-called symbols. In a nutshell, a symbol is an object containing semantic information about some entity in the source code. For example, the method's symbol allows you to get data about the parameters of this method, the parent class, the return type, others. This topic is covered in more detail in the article "Introduction to Roslyn and its use in program development". I highly recommend reading it to everyone who is interested in static analysis, regardless of the preferred programming language.

We had to get string representations of some symbols, and we did so by calling the toString *method. A complex algorithm inside was actively creating objects of the *DisplayPart type. The problem was that the algorithm worked out every time we needed to get a string representation. That is, quite often.

Usually problem localization = 90% of its solution. Since ToString calls are so troublesome, maybe we shouldn't make them?

Sadly, we can't completely refuse to obtain string representations. So we decided to at least minimize the number of ToString calls from symbols.

The solution was simple — we began to cache the resulting string representations. Thus, the algorithm for obtaining a string representation worked out no more than once for each symbol. At least it worked so for a single thread. In our opinion, the best option is to use its own cache for each thread. This way we can do without threads synchronization, while some values' duplication was negligible.

The edit I described seemed very promising. Despite this, the change did not increase the CPU load much - it was only a few percent. However, PVS-Studio began to work much faster. One of our test projects was previously analyzed for 2.5 hours, and after edits - only 2. Acceleration by 20% made us really excited.

Boxed Enumerator

List.Enumerator objects used to traverse collections took the second place in the amount of memory allocated. The list iterator is a structure. This means it is created on the stack. Anyway, the tracing was showing that a great number of such objects were getting in a heap! We had to deal with it.

An object of value type can get into the heap due to boxing. Boxing implements when a value object casts to Object or an implemented interface. The list iterator implements the *IEnumerator *interface. Casting to this interface led to the iterator getting into the heap.

The GetEnumerator method is used to get the Enumerator object. We all know that this method is defined in the IEnumerable interface. Looking at its signature, we can notice that the return type of this method is IEnumerator. Does GetEnumerator call always lead to boxing?

Well...Nope! The GetEnumerator defined in the List class returns a structure:

Will there be boxing or not? The answer depends on the type of the reference from which GetEnumerator is called:

The resulting iterators are of the same value. Their distinction is that one is stored on the stack and the other – in a heap. Obviously, in the second case, the garbage collector is forced to do additional work.

The difference is small if such an Enumerator is created a couple of hundred times during the program operation. Speaking of an average project analysis, the picture is different. These objects are created millions or even tens of millions of times in our C# analyzer. In such cases, the difference becomes palpable.

Note. Generally, we don't call GetEnumerator directly. But quite often we have to use the foreach *loop. This loop gets the iterator "under the hood". If a *List reference is passed to foreach, the iterator used in foreach will be on the stack. Here is another case when foreach helps traverse an abstract IEnumerable. This way, the iterator will be in a heap, whereas foreach will work with the IEnumerator reference. The behavior above relates to other collections that contain GetEnumerator returning a value-type iterator.

Sure, we can't completely opt out of using IEnumerable. However, the analyzer code revealed many places where the method received an abstract IEnumerable as an argument, but still developers always pass a quite specific list.

Well, generalization is a good thing. Especially because a method that receives IEnumerable will be able to work with any collection, not with a particular one. Nonetheless, sometimes this approach demonstrates earnest drawbacks with no actual advantages.

And You, LINQ?!

Extension methods defined in System.Linq namespace are used to work with collections everywhere. Often enough, they really allow you to simplify the code. Almost every decent project comprises everybody's favorite methods Where, Select, others. PVS-Studio C# analyzer is no exception.

Well, the beauty and convenience of LINQ *methods cost us dearly. It cost so much, that we chose not to use them in favor of simple *foreach. How did it come out like that?

The main problem again was a huge number of objects implementing the *IEnumerator *interface. Such objects are created for each call of a *LINQ *method. Check out the following code:

List<int> sourceList = ....
var enumeration = sourceList.Where(item => item > 0)
                            .Select(item => someArray[item])
                            .Where(item => item > 0)
                            .Take(5);

How many iterators will we get when executing it? Let's count! Let's open System.Linq source file to get how it all works. Get them on github by link.

When you call Where, a WhereListIterator object will be created. It is a special version of the Where iterator optimized to work with List. There is a similar optimization for arrays. This iterator stores a reference to the list inside. When traversing the collection, WhereListIterator will save a list iterator within itself and use it when working. Since WhereListIterator is designed specifically for a list, the iterator won't cast to the IEnumerator type. *WhereListiterator *itself is a class, which means its instances will fall into the heap. Hence, the original iterator won't be on the stack anyway.

Calling Select will create an object of the Where*SelectListIterator* class. Obviously, it will be stored in the heap.

Subsequent Where and Take calls will result in iterators and allocated memory for them.

What do we get? Allocated memory for 5 iterators. The garbage collector will have to release it later.

Now look at the fragment written using foreach:

List<int> sourceList = ....
List<int> result = new List<int>();

foreach (var item in sourceList)
{
  if (item > 0)
  {
    var arrayItem = someArray[item];

    if (arrayItem > 0)
    {
      result.Add(arrayItem);

      if (result.Count == 5)
        break;
    }
  }
}

Let's analyze and compare approaches with foreach and LINQ.

Advantages of the option with LINQ calls:
- shorter, nicer and simpler to read;
- does not require a collection to store the result;
- values will be calculated only when accessing elements;
- in most cases, the accessed object stores only one element of the sequence.
Disadvantages of the option with LINQ-calls:
- memory in the heap allocates much more often: in the first example there are 5 objects, and in the second - only 1 (result list);
- repeated traverses of a sequence result in a repeated traversal calling all the specified functions. Cases where this behavior is actually useful are quite rare. Sure, one can use methods like ToList. But this negates the benefits of the LINQ-calls option (except for the first advantage).

As a whole, the shortcomings are not very weighty if the LINQ query is executed relatively infrequently. As for us, we are in a situation where this has happened hundreds of thousands and even millions of times. Besides, those queries were not as simple as in the example given.

With all this, we noticed that mostly we had no interest in delayed execution. It was either a ToList call for *LINQ *operations result. Or query code was executed several times during repeated traverses - which is undesirable.

Remark. *In fact, there is an easy way to implement delayed execution without unnecessary iterators. You might have guessed I was talking about the *yield keyword. With it, you can generate a sequence of elements, specify any rules and conditions to add elements to a sequence. For more information on the capabilities of yield in C#, as well as how it works internally, read the article "What Is yield and How Does It Work in C#? ".

Having carefully reviewed the analyzer code, we found many places where foreach is preferable to LINQ methods. This has significantly reduced the number of required memory allocation operations in the heap and garbage collection.

What Have We Got In the End?

Profit!

PVS-Studio optimization completed successfully! We have reduced memory consumption, considerably increased the analysis speed. By the way, some projects have increased speed by more than 20% and peak memory consumption decreased by almost 70%! And everything started with an incomprehensible client's story of how he could not check his project in three days! Still we'll keep optimizing the tool and finding new ways how to improve PVS-Studio.

Studying the problems took us much longer than solving them. But the story told happened a very long time ago. The PVS-Studio team can now solve such problems much faster. The main assistants in problem research are various tools such as tracer and profiler. In this article, I talked about our experience with dotMemory and dotPeek, but this does not mean that these applications are one of a kind. Please write in the comments what tools you use in such cases.

It's Not Over Yet

Yes, we did solve the client's problem and even speeded up the analyzer as a whole, but... It obviously works by far not as fast as it can. PVS-Studio is still not actively using processor power. The problem is not exactly the analysis algorithms — checking each file in a separate thread allows it to provide a fairly high level of concurrency. The main performance trouble of the C# analyzer is a garbage collector, which very often blocks the operation of all threads - this is how we get slowdowns. Even if the analyzer uses hundreds of cores, the operation speed will be reduced due to frequent blocking of threads by the collector. The latter can't use all available power in its tasks due to some algorithmic constraints.

This is not a stalemate, though. It's just another obstacle that we must overcome. Some time ago I got "secret information" about plans to implement the analysis process... in several processes! This will help bypass existing constraints. Garbage collection in one of the processes will not affect the analysis performed in the other. Such an approach will allow us to effectively use a large number of cores and use Incredibuild as well. By the way, a C++ analyzer already works in a similar way. It has long used distributed analysis.

Where Else Do Performance Issues Come From?

There is another noteworthy performance drawback. It is not about LINQ queries or something like that - it is common errors in code. "always true" conditions that make the method work longer, typos and others—all this affects both performance and the application as a whole.

Modern IDEs allow you to detect some problematic points. On the other hand, they review the code quite superficially. As a result, they detect only most obvious errors, like unused variable or parameter. Static analysis tools lend a hand in finding complex errors. Such tools immerse into the code much deeper, although it takes longer to do so. A static analyzer is able to find many different errors, including those that lead to problems with speed and memory consumption.

PVS-Studio is one of such analyzers. It uses enhanced technologies such as inter-procedural analysis or data flow analysis, which allow you to significantly increase the code reliability of any application. Here's another company priority - to support users, solve their issues and emerging problems. In some cases we even add new features at a client's request. Feel free to write to us on all issues that arise! Click the link to try the analyzer in action. Enjoy the usage!