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:
- NSA, Ghidra and the unicorns
- PVS-Studio for Java under the hood: diagnostics development
- Checking the XMage code and why special rare cards for the Dragon's Maze collection are not available
- Checking WildFly - JavaEE Application Server
- Unicorns at Your Safety: Examining the Bouncy Castle Code
- Big / Bug Data: Analyzing Apache Flink Source Code
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"
Source: Big / Bug Data: Analyzing Apache Flink
V6001 Source Code 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 error due to inattention: the processedData field is compared to itself. Due to this error, comparison of objects of type CheckpointStatistics will sometimes give false positives. But the main danger of this typo is that equals is very actively used in collections, and incorrect implementation of this method can lead to very strange behavior, which will take a huge amount of time to debug.
I'd like to point out that it's common for developers to make mistakes in comparison functions. A colleague of mine even wrote a long article " Evil Lives in Comparison Functions " with many examples and explanations.
Ninth place: "Unreachable code"
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;
....
}
}
}
The switch branch for value i == 0x0822 (2082) was unreachable. How did it happen?
If we pay attention to the condition of the loop 1 << height , where height is always equal to 10 , then everything will immediately fall into place. According to the condition of the loop, the counter i in the for loop cannot be more than 1024 (1 << 10). Naturally, the execution of the considered switch branch will never happen.
Eighth place: "Annotated method"
Source: Under the Hood PVS-Studio for Java: 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 derives 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 when 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 are out of sync after the call after .
Seventh place: "Expectation / Reality"
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 is not null,
- the string representation of modelNode is 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. This is because the modelNode is checked for equality with the default value in the last line of the method.
The string representation of 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 the modelNode value when it equals the default value ( attribute.getDefaultValue () ).
Sixth place: "Copy-paste-oriented programming"
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, like last year ( Top 10 errors in 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, 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, playerA got the same card twice. Because of this, the play area is Zone.GRAVEYARDPlayer playerB left without testing. A detailed description of the error can be found in the article itself.
Fifth place: "Abnormal distribution"
Source: Big / Bug Data: analyzing the source code of Apache Flink
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();
}
The bug was discovered in the partition utility method , which splits the passed elements collection into numBuckets collections. The essence of the error is that the index of the bucket collection into which each item in question is to be placed has a constant value (0). The reason for this is that the developer forgot to increment the index variable at each iteration of the loop.
As a result, the partition method will always return an elements collection wrapped in another collection. And this is hardly intended behavior.
Fourth place: "Time bomb"
Source: NSA, Ghidra and 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 the selectedNode from processSelection () when selectedNode == null , you will immediately find that we are in for an imminent NullPointerException . This is what the analyzer warns us about.
But, after studying a little code, the author of the article came to the conclusion that program execution will never encounter a NullPointerException, since processSelection () is called in only two places, before calling which selectedNode is explicitly checked for null.
Regardless, this code is a time bomb, because another developer might see that the method explicitly handles the selectedNode == null case and decide that this is a valid value, which will then crash the application.
Third place: "Always false"
Source: Checking the XMage code and why the 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"
Source: NSA, Ghidra and 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;
}
The indexOf method always returns a non-negative number. And all because the author of the method, in the absence of the desired newName, by mistake returns 0, 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 issue 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?"
Source: Under the Hood PVS-Studio for Java: Diagnostics Development .
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 using the menu key , if it did not exist yet. But checking the wrong variable ruined the whole idea, cutting a loophole for the NullPointerException. The method will throw an exception when the menu key is absent in the dictionary , and the item value that they wanted to add is not 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. Articles about checks will begin immediately from the first month of 2021, and if you are impatient, download the analyzer and check open-source projects yourself.
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 .