The New Year is steadily approaching, therefore it's time to sum everything up. Continuing the tradition, we thought back to our articles about checking Java projects from the open-source world for this year and rated the top 10 most exciting bugs.
In 2020 we (the PVS-Studio's Java team) have reviewed errors from five open-source projects and also touched upon our inner workings in our articles:
- NSA, Ghidra, and Unicorns
- Under the Hood of PVS-Studio for Java: How We Develop Diagnostics
- Checking the Code of XMage, and Why You Won't Be Able to Get the Special Rare Cards of the Dragon's Maze Collection
- Checking WildFly, a JavaEE Application Server
- Unicorns on Guard for Your Safety: Exploring the Bouncy Castle Code
- Big / Bug Data: Analyzing the Apache Flink Source Code
First, we suggest that readers get to know these articles and make their personal rankings, then compare them with ours, and say if we are wrong :).
N10. Tricky equality
Source: Big / Bug Data: Analyzing the 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);
}Here's an example of a simple and very disappointing mistake which occurred due to inattention: the processedData field is compared to itself. Because of this mistake, comparison of the CheckpointStatistics type objects will sometimes give a false positive result. But the main danger of this typo lies in the fact 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.
For the record, it is common for developers to make mistakes in comparison functions. My colleague even wrote a whole article "The Evil within the Comparison Functions" with lots of examples and explanations.
N9. Unreachable code
Source: Unicorns on Guard for Your Safety: Exploring 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 the value i == 0x0822(2082) happens to be unreachable. How on earth did that happen?
If you pay attention to the 1 << height loop condition where height is always equal to 10, then everything immediately falls into place. According to the loop condition, the counter i in the for loop can't be greater than 1024 (1 << 10). Obviously, the execution of the switch branch in question will never happen.
N8. Annotated method
Source: Under the Hood of PVS-Studio for Java: How We Develop Diagnostics.
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, they show:
- If a method is pure,
- What restrictions are posed on arguments,
- Returned result,
- … and things like that.
Some annotations are derived by the analyzer itself from the source code, and some are added manually (for example, for the standard library methods). The story of this error began with the fact that we did not fully annotate the Map#clear method. After we noticed and fixed it, new warnings (including our peculiar case) appeared on our test projects.
At first glance, re-clearing a dictionary is not an error. And we would even think that this is a randomly duplicated string if we did not pay attention to 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 metricsByld and metricsByKey. This got us thinking that the author wanted to clear both dictionaries but… that did not happen. Thus, two dictionaries that store related data will be put out of sync after calling after.
N7. Expectation / reality
Source: Checking WildFly, a 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()); // <=
}According to the comment before the method, we can expect the method to return true, if:
- modelNode is not null,
- the string representation of modelNode is not empty,
- modelNode is not the default value.
Despite the author's comment and the logic that seems correct at first glimpse, the method's behavior will be different. The reason for this is the modelNode check for equality with the default value in the method's last line.
The modelNode string representation is compared to an object of ModelNode type, and as you might guess, such a comparison will always return a negative result due to type incompatibility.
Error consequences are as follows: an unexpected permission to send the modelNode value when it is equal to the default value (attribute.getDefaultValue()).
N6. Copy-paste-oriented programming
Source: Checking the Code of XMage, and Why You Won't Be Able to Get the Special Rare Cards of the Dragon's Maze Collection.
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));
}
}
}A cool copy-paste error from the V6072 diagnostic rule earns a place in the top 10 of this year. The same happened in 2019 (Top 10 errors for 2019).
The error's nature is as follows. When the developer needs to do similar actions for different variables, they copy the code, faithfully written previously, and change the variable's name. But they do this quite negligently and forget to modify the variables in some places.
That's exactly what happened in this code snippet. The test's author simulated the game between the players, scattering the same cards between them in the game zones, but because of the copy-paste, the playerA got the same card twice. Due to this, the game zone Zone.GRAVEYARD of the playerB was left without testing. A detailed description of the error can be found in the article itself.
N5. Non-normal distribution
Source: Big / Bug Data: Analyzing the 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();
}The error was found in the partition utility method, which splits the elements collection into numBuckets of collections. The essence of the error is that the bucket collection's index, in which each element in question is going 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 the elements collection wrapped in another collection. And this is hardly the intended behavior.
N4. A 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 certainly messed up. If you follow selectedNode from processSelection(), when selectedNode == null, you will immediately find that with this outcome, NullPointerException inevitably awaits us. This is exactly what the analyzer warns us about.
But after studying the code a little, the author of the article concluded that the program execution will never meet NullPointerException, since the processSelection() method is called in only two places, before calling which, selectedNode is explicitly checked for the null.
Despite this, such code is a time bomb, because another developer may notice that the method explicitly handles the case of selectedNode == null, and decide that this is a valid value, which will then result in the application crash.
N3. Always false
Source: Checking the Code of XMage, and Why You Won't Be Able to Get the Special Rare Cards of the Dragon's Maze Collection.
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();
}Who compares a lowercase string to a string that starts with a capital letter? Hence, we've got an always false result of the message's check.
The defect's result is not critical, but nasty as well: a poorly written text will appear somewhere.
N2. 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. This is due to the fact that the method's author returns 0 instead of -1 by mistake in case newName is absent in the collection. This error results in the program's execution flow always entering the then-branch of the if (index >= 0) conditional statement, in which it will generate the message about the existing newName and successfully exit the method, even though in reality newName was not found.
But it's not the end of the story. Since the then-branch of the conditional statement exits the method, the execution flow won't reach the code after the conditional statement.
This is exactly what the analyzer warns us about.
N1. Did we check the right one?
Source: Under the Hood of PVS-Studio for Java: How We Develop 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, this method was supposed to create a collection using the menu key, if there was not one yet. But a wrong variable check ruined the whole idea, cutting a loophole for NullPointerException. The method will throw the exception when the menu key is absent in the dictionary, and the item value that we wanted to add will not be null.
Conclusion
Each year PVS-Studio checks of open-source projects prove that such stage of protection as static code analysis must necessarily take place in development. No matter how skilled you are, mistakes will always find a loophole in your project, and there are many reasons for this: you may get tired, swamped with work, or even be distracted by cats. And if you work in a team, the number of possibilities for errors to get in the code increases proportionately to the number of colleagues.
If you enjoyed our review, then don't wait for the next year's end. Articles about checks will come out immediately at the beginning of 2021, but if you can't wait, feel free to download the analyzer and check open-source projects by yourself.