PVS-Studio for Java under the hood: developing diagnostics

For a change, today we will tell you a little about the process of developing and finalizing diagnostic rules for PVS-Studio Java. Let’s see why the old analyzer triggers don’t float too much from release to release, and the new ones are not too crazy. And we will spoil a little “what are the plans of the javists” and show a couple of beautiful (and not so) errors found using diagnostics from the next release.

Diagnostics and SelfTester development process

Naturally, each new diagnostic rule starts with an idea. And since the Java analyzer is the youngest direction in PVS-Studio’s development, we basically steal these ideas from the C / C ++ and C # departments. But not everything is so bad: we also add rules that were invented on our own (including by users – thanks!), So that later the same departments would steal them from us. The cycle, as they say.

The very implementation of the rules in the code in most cases turns out to be quite a pipeline task. You create a file with several synthetic examples, mark with your hands where errors should be, and with a debugger at the ready you run through the syntax tree until you get bored and cover all the invented cases. Sometimes the rules turn out to be absurdly simple (for example, V6063 consists of literally a few lines), and sometimes you have to think about the logic long enough.

However, this is just the beginning. As you know, we do not particularly like synthetic examples, since they very poorly reflect the type of analyzer triggers in real projects. By the way, most of these examples in our unit tests are drawn from real projects – it’s almost impossible to invent all possible cases on your own. And unit tests also allow us not to lose triggers on examples from the documentation. There were precedents, yes, only shh.

So, the positives in real projects must be found in some way first. And you also need to somehow check that:

  • The rule will not fall for open-source madness, where “interesting” solutions are common;
  • There are no obvious false positives (or there are very, very few of them, and we cannot do anything with them for one reason or another);
  • New manually added annotations for data-flow (the mechanism for analyzing the data flow) did not break anything and brought something interesting;
  • After the changes in the kernel, the analyzer did not fall apart into the same open-source madness;
  • The new rule does not increase project analysis time by over 9000%;
  • We have not lost the “good” positives we used to have;
  • Anything else like that.

In general, here, like a knight on a horse (a little limping, but we are working on it), SelfTester comes to the fore. Its main and only task is to automatically check a bunch of projects and show which triggers have been added, disappeared or changed relative to the “reference” in the version control system. Provide diffs for the analyzer report and show the corresponding code in projects, in short. Currently SelfTester for Java is testing 62 open-source projects of bearded versions, among which are, for example, DBeaver, Hibernate and Spring. One full run of all projects takes 2-2.5 hours, which is undoubtedly painful, but nothing can be done.

In the screenshot above, the “green” projects are those in which nothing has changed. Each diff in “red” projects is reviewed manually and, if correct, is confirmed by the same “Approve” button. By the way, the analyzer distribution kit will be built only if SelfTester gives a pure green result. In general, this is how we maintain the consistency of results between different versions.

In addition to maintaining the consistency of results, SelfTester allows us to get rid of a huge number of false positives even before the release of diagnostics. A typical development pattern looks like this:

  • The most general implementation of the synthetic test rule, not necessarily complete. For example, “find all uses of a double-checked locking pattern” when looking for invalid implementations of this pattern;
  • Complete run of SelfTester, usually at night;
  • By diffs, several signs of false positives are selected and implemented, which are added to unit tests;
  • SelfTester run on projects that still have triggers;
  • Repeat points 3-4, while there are false positives;
  • Final review of diffs, their approval, preparation of documentation (the one on the site);
  • Adding diagnostics, new reference and documentation to master.

Fortunately, full SelfTester runs are quite rare, and you don’t have to wait “2-2.5 hours” very often. From time to time, luck bypasses, and triggers appear on large projects like Sakai and Apache Hive – it’s time to drink coffee, drink coffee and drink coffee. You can also study the documentation, but this is not for everybody.

“Why do we need unit tests, since there is such a magic tool?”

And then the tests are significantly faster. A few minutes – and there is already a result. They also allow you to see exactly which part of the rule fell off. And, besides, not always all admissible operations of any rule are caught in SelfTester projects, but their operability must also be checked.

New problems in old acquaintances

Initially, this section of the article began with the words “The versions of projects in SelfTester are quite old, so most of the errors presented have most likely already been fixed.” However, when I decided to make sure of this, I was in for a surprise. Every single mistake remained in place. Everything. This means two things:

  • These errors are not super critical for the application to work. Many of them, by the way, are in the test code, and incorrect tests can hardly be called consistent.
  • These errors are found in infrequently used files of large projects, which developers hardly go to. Because of this, the incorrect code is doomed to lie there for a very long time: most likely, until some critical bug occurs because of it.

For those wishing to dig deeper, there will be links to specific versions that we are checking.

PS The above does not mean that static analysis catches only harmless errors in unused code. We check the release (and almost release) versions of projects – in which the developers and testers (and sometimes, unfortunately, users) found the most relevant bugs by hand, which is long, expensive and painful. You can read more about this in our article “Errors that static code analysis cannot find, because it is not used“.

Apache Dubbo and blank menu

Github

Diagnostics “V6080 Consider checking for misprints. It’s possible that an assigned variable should be checked in the next condition“has already been released in version 7.08, but has not appeared in our articles yet, so it’s time to fix it.

Menu.java:40

public class Menu
{
  private Map> menus = new HashMap>();

  public void putMenuItem(String menu, String item)
  {
    List items = menus.get(menu);
    if (item == null)                      // <=
    {
      items = new ArrayList();
      menus.put(menu, items);
    }
    items.add(item);
  }
  ....
}

A classic example of a “key-collection” dictionary and an equally classic typo. The developer wanted to create a collection corresponding to the key, if it does not already exist, but he mixed up the name of the variable and got not only the incorrect operation of the method, but also NullPointerException on the last line. For Java 8 and later, to implement such dictionaries, you should use the method computeIfAbsent:

public class Menu
{
  private Map> menus = new HashMap>();

  public void putMenuItem(String menu, String item)
  {
    List items = menus.computeIfAbsent(menu, key -> new ArrayList<>());
    items.add(item);
  }
  ....
}

Glassfish and double-checked locking

Github

One of the diagnostics that will be included in the next release is checking the correct implementation of the “double-checked locking” pattern. Glassfish turned out to be the record holder for detections from SelfTester projects: in total, PVS-Studio, using this rule, found 10 problem areas in the project. I suggest the reader to have some fun and look for two of them in the code snippet below. For help, you can refer to the documentation: “V6082 Unsafe double-checked locking“. Well, or, if you don’t want to at all, at the end of the article.

EjbComponentAnnotationScanner.java

public class EjbComponentAnnotationScanner
{
  private Set annotations = null;

  public boolean isAnnotation(String value)
  {
    if (annotations == null)
    {
      synchronized (EjbComponentAnnotationScanner.class)
      {
        if (annotations == null)
        {
          init();
        }
      }
      return annotations.contains(value);
    }
  }

  private void init()
  {
    annotations = new HashSet();
    annotations.add("Ljavax/ejb/Stateless;");
    annotations.add("Ljavax/ejb/Stateful;");
    annotations.add("Ljavax/ejb/MessageDriven;");
    annotations.add("Ljavax/ejb/Singleton;");
  }

  ....
}

SonarQube and data-flow

Github

Refinement of diagnostics is not only about direct modification of their code in order to catch more suspicious places or remove false positives. Manual marking of methods for data-flow also plays an important role in the development of the analyzer – for example, you can write that such and such a library method always returns non-null. When writing a new diagnostic, we accidentally discovered that we did not have a marked method Map # clear ()… Apart from the obviously stupid code that the diagnostics began to catch “V6009 Collection is empty. The call of the ‘clear’ function is senseless“we were able to find a great typo.

MetricRepositoryRule.java:90

protected void after()
{
  this.metricsById.clear();
  this.metricsById.clear();
}

At first glance, clearing the dictionary again is not a mistake. And we would even think that this is a randomly duplicated line, if our gaze had not dropped a little lower – literally to the next method.

protected void after()
{
  this.metricsById.clear();
  this.metricsById.clear();
}
public Metric getByKey(String key)
{
  Metric res = metricsByKey.get(key);
  ....
}

Exactly. The class has two fields with similar names metricsById and metricsByKey… I am sure that in the method after the developer wanted to clear both dictionaries, but either the auto-completion failed him, or he entered the same name by inertia. Thus, the two dictionaries that store related data will be out of sync after calling after

Sakai and empty collections

Github

Another new diagnostic that will be included in the next release is “V6084 Suspicious return of an always empty collection“. It’s easy enough to forget to add items to the collection, and especially when each item must be initialized first. From personal experience, such errors most often lead not to an application crash, but to strange behavior or lack of functionality.

DateModel.java:361

public List getDaySelectItems()
{
  List selectDays = new ArrayList();
  Integer[] d = this.getDays();
  for (int i = 0; i < d.length; i++)
  {
    SelectItem selectDay = new SelectItem(d[i], d[i].toString());
  }
  return selectDays;
}

By the way, the same class contains very similar methods without the same error. For example:

public List getMonthSelectItems()
{
  List selectMonths = new ArrayList();
  Integer[] m = this.getMonths();
  for (int i = 0; i < m.length; i++)
  {
    SelectItem selectMonth = new SelectItem(m[i], m[i].toString());
    selectMonths.add(selectMonth);
  }
  return selectMonths;
}

Plans for the future

Apart from various not very interesting internal things, we are thinking about adding diagnostics for the Spring Framework to the Java analyzer. It is not only the main bread for Javists, but also contains a lot of non-obvious moments on which one can stumble. We are not yet very sure in what form these diagnostics will eventually appear, when it will happen and whether it will happen at all. But we are sure that we need ideas for them and open-source projects using Spring for SelfTester. So, if there is something in mind - suggest (in comments or private messages, too)! And the more of this goodness we collect, the more priority will shift to it.

And finally, errors in the double-checked locking implementation from Glassfish:

  • The field is not declared 'volatile'.
  • The object is first published and then initialized.

Why is all this bad - again, you can look at documentation...

If you want to share this article with an English-speaking audience, please use the translation link: Nikita Lazeba. Under the Hood of PVS-Studio for Java: How We Develop Diagnostics.

Similar Posts

Leave a Reply