Top 10 Java project bugs in 2020

image1.png

The New Year is inexorably approaching – and, therefore, the time has come to take stock. Continuing the tradition, we went through our articles on checking Java projects from the open-source world this year and made a rating of the 10 most interesting bugs.

Over the past year, we (PVS-Studio’s Java team) have sorted out errors from five open-source projects in our articles and talked quite a bit about our inner workings:

We suggest that the reader first read these articles and make his own personal rating, then compare it with ours and say that we are wrong :).

Tenth place: “Deceptive equality”

A source: Big / Bug Data: Analyzing Apache Flink Source Code

V6001 There are identical sub-expressions ‘processedData’ to the left and to the right of the ‘==’ operator. CheckpointStatistics.java (229)

@Override
public boolean equals(Object o) 
{
  ....
  CheckpointStatistics that = (CheckpointStatistics) o;
  return id == that.id &&
    savepoint == that.savepoint &&
    triggerTimestamp == that.triggerTimestamp &&
    latestAckTimestamp == that.latestAckTimestamp &&
    stateSize == that.stateSize &&
    duration == that.duration &&
    alignmentBuffered == that.alignmentBuffered &&
    processedData == processedData &&                // <=
    persistedData == that.persistedData &&
    numSubtasks == that.numSubtasks &&
    numAckSubtasks == that.numAckSubtasks &&
    status == that.status &&
    Objects.equals(checkpointType, that.checkpointType) &&
    Objects.equals(
      checkpointStatisticsPerTask, 
      that.checkpointStatisticsPerTask);
}

A simple and very offensive mistake due to inattention: the field processedData compares with oneself. Because of this error, comparing objects like CheckpointStatistics will sometimes give a false positive result. But the main danger of this typo is that equals It is very actively used in collections, and an incorrect implementation of this method can lead to very strange behavior, which will take a huge amount of time to debug.

I want to point out that it is common for developers to make mistakes in comparison functions. My colleague even wrote a great article “Evil lives in comparison functions“with many examples and explanations.

Ninth place: “Unreachable code”

A source: Unicorns at Your Safety: Examining the Bouncy Castle Code

V6019 Unreachable code detected. It is possible that an error is present. XMSSTest.java (170)

public void testSignSHA256CompleteEvenHeight2() {
    ....
    int height = 10;
    ....
    for (int i = 0; i < (1 << height); i++) {
        byte[] signature = xmss.sign(new byte[1024]);
        switch (i) {
            case 0x005b:
                assertEquals(signatures[0], Hex.toHexString(signature));
                break;
            case 0x0822:
                assertEquals(signatures[1], Hex.toHexString(signature));
                break;
            ....
        }
    }
}

Branch switch for meaning i == 0x0822 (2082) turned out to be unattainable. How did it happen?

If you pay attention to the loop condition 1 << heightwhere height always 10, then everything will fall into place at once. According to the loop condition, the counter i in a loop for cannot be more than 1024 (1 << 10). Naturally, the execution of the considered branch switch will never happen.

Eighth place: “Annotated method”

A source: PVS-Studio for Java under the hood: diagnostics development

V6009 Collection is empty. The call of the ‘clear’ function is senseless. MetricRepositoryRule.java (90)

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

Some of our diagnostics rely heavily on the method annotation mechanism. Annotations provide additional information to the analyzer about the methods used, for example:

  • Is this a clean method
  • What are the restrictions on the arguments,
  • Returned result,
  • … and so on.

The analyzer deduces some annotations from the source code itself, some we add manually (for example, for the methods of the standard library). The story of this error began with the fact that we did not fully annotate the Map # clear method. After we noticed and fixed this, new triggers appeared on our test projects, among which was our interesting case.

At first glance, clearing the dictionary again is not a mistake. And we would even think that this is a randomly duplicated string if we did not pay attention to the class fields:

private final Map<String, Metric> metricsByKey = new HashMap<>();
private final Map<Long, Metric> metricsById = new HashMap<>();

The class has two fields with similar names metricsById and metricsByKey… This suggests that the author of the code wanted to clear both dictionaries, but … this did not happen. Thus, the two dictionaries that store related data will be out of sync after calling after

Seventh place: “Expectation / Reality”

A source: Checking WildFly – JavaEE Application Server

V6058 The ‘equals’ function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java (563)

// Send value to RESTEasy only if it's not null, empty string, or the 
// default value.
private boolean isTransmittable(AttributeDefinition attribute,
                                ModelNode modelNode) {
  if (modelNode == null || ModelType
      .UNDEFINED.equals(modelNode.getType())) {
    return false;
  }
  String value = modelNode.asString();
  if ("".equals(value.trim())) {
    return false;
  }
  return !value.equals(attribute.getDefaultValue());        // <=
}

Noticing the comment that precedes the method, you might expect the method to return true, if:

  • modelNode not null,
  • string representation modelNode not empty,
  • modelNode Is not the default.

Despite the author’s comment and, at first glance, correct logic, the behavior of the method will differ. The reason for this is to check modelNode for equality with the default value in the last line of the method.

String representation modelNode is compared to an object of type ModelNode, and, as you might guess, such a comparison will always return a negative result due to type incompatibility.

Consequences of the error: unexpected permission to send a value modelNode, when it is equal to the default (attribute.getDefaultValue ()).

Sixth place: “Copy-paste-oriented programming”

A source: Checking the XMage code and why special rare cards for the Dragon’s Maze collection are not available

V6072 Two similar code fragments were found. Perhaps, this is a typo and ‘playerB’ variable should be used instead of ‘playerA’. SubTypeChangingEffectsTest.java (162), SubTypeChangingEffectsTest.java (158), SubTypeChangingEffectsTest.java (156), SubTypeChangingEffectsTest.java (160)

@Test
public void testArcaneAdaptationGiveType() {
    addCard(Zone.HAND, playerA, "Arcane Adaptation", 1);
    addCard(Zone.BATTLEFIELD, playerA, "Island", 3);

    addCard(Zone.HAND, playerA, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerA, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    addCard(Zone.HAND, playerB, "Silvercoat Lion");
    addCard(Zone.BATTLEFIELD, playerB, "Silvercoat Lion");
    addCard(Zone.GRAVEYARD, playerA, "Silvercoat Lion");   // <=

    ....

    for (Card card : playerB.getGraveyard().getCards(currentGame)) {
        if (card.isCreature()) {
            Assert.assertEquals(card.getName() + " should not have ORC type",
                    false, card.getSubtype(currentGame).contains(SubType.ORC));
            Assert.assertEquals(card.getName() + " should have CAT type",
                    true, card.getSubtype(currentGame).contains(SubType.CAT));
        }
    }
}

This year, as well as last (Top 10 mistakes of 2019), the cool copy-paste error from the V6072 diagnostic rule deserves a place in the top ten.

The nature of the error lies in the fact that when a developer needs to do similar actions for different variables, then he copies the code written in good faith earlier and changes the name of the variable. But he does it not entirely in good faith and forgets to modify the variables.

This is exactly what happened in this code snippet. The author of the test imitated a game between players, scattering identical cards between them across the playing zones, but due to copy-paste to the player playerA the same card was dealt twice. Because of this, the play area Zone.GRAVEYARD player playerB was left without testing. A detailed description of the error can be found in the article itself.

Fifth place: “Abnormal distribution”

A source: Big / Bug Data: Analyzing Apache Flink Source Code

V6048 This expression can be simplified. Operand ‘index’ in the operation equals 0. CollectionUtil.java (76)

public static <T> 
Collection<List<T>> partition(Collection<T> elements, int numBuckets) 
{
  Map<Integer, List<T>> buckets = new HashMap<>(numBuckets);
  
  int initialCapacity = elements.size() / numBuckets;

  int index = 0;
  for (T element : elements) 
  {
    int bucket = index % numBuckets;                                 // <=
    buckets.computeIfAbsent(bucket, 
                            key -> new ArrayList<>(initialCapacity))

           .add(element); 
  }

  return buckets.values();
}

An error was found in the utility method partitionwhich splits the passed collection elements on numBuckets collections. The essence of the error is that the collection index bucket, in which they want to place each element in question, has a constant value (0). The reason for this is that the developer forgot to increment the variable. index at each iteration of the loop.

As a result, the method partition will always return a collection elementswrapped in another collection. And this is hardly intended behavior.

Fourth place: “Time bomb”

A source: NSA, Ghidra and the unicorns

V6008 Null dereference of ‘selectedNode’ in function ‘setViewPanel’. OptionsPanel.java (266)


private void processSelection(OptionsTreeNode selectedNode) {
  if (selectedNode == null) {
    setViewPanel(defaultPanel, selectedNode); // <=
    return;
  }
  ....
}
private void setViewPanel(JComponent component, OptionsTreeNode selectedNode) {
  ....
  setHelpLocation(component, selectedNode);
  ....
}
private void setHelpLocation(JComponent component, OptionsTreeNode node) {
  Options options = node.getOptions();
  ....
}

The above code snippet is clearly screwed up. If you follow selectedNode of processSelection ()when selectedNode == null, then you will immediately find that with such an outcome, the inevitable NullPointerException… This is what the analyzer warns us about.

But, having studied a little code, the author of the article came to the conclusion that program execution will never meet with NullPointerException, as processSelection() is called in only two places, before the call of which selectedNode explicitly checked for null.

Regardless, this code is a ticking time bomb, as another developer might see that the method is explicitly handling the case selectedNode == null, and decide that this is a valid value, which will then result in the application crash.

Third place: “Always false”

A source: Checking the XMage code and why special rare cards for the Dragon’s Maze collection are not available

V6007 Expression ‘filter.getMessage (). ToLowerCase (Locale.ENGLISH) .startsWith (“Each”)’ is always false. SetPowerToughnessAllEffect.java (107)

@Override
public String getText(Mode mode) {
  StringBuilder sb = new StringBuilder();
  ....
  if (filter.getMessage().toLowerCase(Locale.ENGLISH).startsWith("Each ")) {
    sb.append(" has base power and toughness ");
  } else {
    sb.append(" have base power and toughness ");
  }
  ....
  return sb.toString();
}

So who compares a lowercase string to a string that starts with an uppercase letter? Hence the always false result of checking the message.

The result of the defect is not critical, but also unpleasant: an illiterately composed text will appear somewhere.

Second place: “2-in-1”

A source: NSA, Ghidra and the unicorns

V6007 Expression ‘index> = 0’ is always true. ExternalNamesTableModel.java (105)

V6019 Unreachable code detected. It is possible that an error is present. ExternalNamesTableModel.java (109)

public void setValueAt(Object aValue, int row, int column) {
  ....
  int index = indexOf(newName);
  if (index >= 0) {                  // <=
    Window window = tool.getActiveWindow();
    Msg.showInfo(getClass(), window, "Duplicate Name",
                 "Name already exists: " + newName);
    return;
  }

  ExternalPath path = paths.get(row); // <=
  ....
}

private int indexOf(String name) {
  for (int i = 0; i < paths.size(); i++) {
    ExternalPath path = paths.get(i);
    if (path.getName().equals(name)) {
      return i;
    }
  }
  return 0;
}

Method indexOf always returns a non-negative number. And all due to the fact that the author of the method, in the absence of the desired newName returns 0 by mistake, not -1. Such an error leads to the fact that the program execution flow will always enter the then-branch of the conditional statement if (index> = 0), in which it will display a message about the existing newName and successfully exit the method, even when in reality newName was not found.

But that’s not all. Since the then-branch of the conditional statement terminates the execution of the method, things will never reach the code after the conditional statement.

The analyzer warns us about this.

First place: “Have we checked?”

A source: PVS-Studio for Java under the hood: developing diagnostics

V6080 Consider checking for misprints. It’s possible that an assigned variable should be checked in the next condition. Menu.java (40)

public class Menu
{
  private Map<String, List<String>> menus = new HashMap<String, List<String>>();

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

According to the author’s idea, it was supposed to create a collection by key menuif it hasn’t been there yet. But checking the wrong variable ruined the whole idea, opening a loophole for NullPointerException. The method will throw an exception when the key is in the dictionary menu will be absent and the value itemyou wanted to add won’t be null

Conclusion

Checks of open-source projects using PVS-Studio from year to year prove that such a line of protection as static code analysis must be present in development. No matter how expert you are, mistakes will surely find a loophole in your project, and there are many reasons for this: tired, blocked at work, or even distracted by cats. And if you work in a team, the number of opportunities to get errors in the code grows in proportion to the number of colleagues.

If you liked our review, then don’t wait until next year’s end. Inspection articles will start immediately from the first month of 2021, and if you are impatient – go ahead download analyzer and independently check open-source projects.

If you want to share this article with an English-speaking audience, please use the translation link: Maxim Stefanov. Top-10 Bugs in Java Projects in 2020.

Similar Posts

Leave a Reply

Your email address will not be published. Required fields are marked *