PVS-Studio for Java is sent to the path. Next stop - Elasticsearch

Not for the first year, the PVS-Studio team keeps a blog about checking open-source projects with the same name static code analyzer. To date, more than 300 projects have been verified, and more than 12,000 cases have been written to the database of errors found. Initially, the analyzer was implemented to test C and C ++ code, then C # language support appeared. Therefore, among the proven projects, a large part (> 80%) falls on C and C ++. More recently, Java has been added to the supported languages, which means that the doors to the new world are opening in front of PVS-Studio, and it's time to supplement the database with errors from Java projects.

Java world is huge and diverse, so the eyes diverge when choosing a project to test a new analyzer. Ultimately, the choice fell on the Elasticsearch full-text search engine and analytics. This is a fairly successful project, and in successful projects it is doubly or even more pleasant to find mistakes. So, what defects did PVS-Studio for Java detect? On the result of the test and will be discussed in the article.

Superficial familiarity with Elasticsearch

Elasticsearch is an open source scalable full-text search and analysis engine. It allows you to store large amounts of data, to conduct among them a quick search and analytics (almost in real time). As a rule, it is used as a basic mechanism / technology that provides applications with complex functions and search requirements.

Among the major sites that use Elasticsearch are Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox.
')
I think with acquaintance enough.

How was it

There were no problems with checking. The sequence of actions is quite simple and did not take much time:

• I downloaded Elasticsearch from GitHub ;
• I used the Java analyzer launch instruction and launched the analysis;
• Received a report analyzer, analyzed it and identified interesting cases.

Now let's get to the bottom line.

Caution! NullPointerException is possible

V6008 Null dereference of 'line'. GoogleCloudStorageFixture.java (451)

private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { .... // Reads the body line = reader.readLine(); byte[] batchedBody = new byte[0]; if ((line != null) || (line.startsWith("--" + boundary) == false)) // <= { batchedBody = line.getBytes(StandardCharsets.UTF_8); } .... }); .... } 

The error in this code snippet is that if you could not read the string from the buffer, then the startsWith method in the if statement condition will throw a NullPointerException exception. Most likely, this is a typo, and when writing the condition I meant the operator && instead of || .

V6008 Potential null dereference of 'followIndexMetadata'. TransportResumeFollowAction.java (171), TransportResumeFollowAction.java (170), TransportResumeFollowAction.java (194)

 void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null // <= ? .... : null; validate(request, leaderIndexMetadata, followIndexMetadata, // <= leaderIndexHistoryUUIDs, mapperService); .... } 

Another warning from the V6008 diagnostic. Now the gaze riveted the object followIndexMetadata . The start method takes several input arguments, among which is our suspect. After that, based on the verification of our object for null , a new object is formed that participates in the further logic of the method. Checking for null tells us that followIndexMetadata can still come from the outside by a null object. Well, look further.

Next, the validation method is called with pushing through a set of arguments (again, among which is the object in question). And if you look at the implementation of the validation method, then everything falls into place. Our potential null object is passed as the third argument to the validate method, where it is unconditionally dereferenced. As a result, a potential NullPointerException.

 static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex, // <= ....) { .... Map<String, String> ccrIndexMetadata = followIndex.getCustomData(....); // <= if (ccrIndexMetadata == null) { throw new IllegalArgumentException(....); } .... }} 

What arguments the start method is actually called in is unknown. It is quite possible that all arguments are checked somewhere before the method call, and no dereferencing of the null object threatens us. But, you see, such implementation of the code still looks rather unreliable and deserves attention.

V6060 The 'node' reference was used against it. RestTasksAction.java (152), RestTasksAction.java (151)

 private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); .... // Node information. Note that the node may be null because it has // left the cluster between when we got this response and now. table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4)); table.addCell(node == null ? "-" : node.getHostAddress()); table.addCell(node.getAddress().address().getPort()); table.addCell(node == null ? "-" : node.getName()); table.addCell(node == null ? "-" : node.getVersion().toString()); .... } 

Here another diagnostic rule worked, and the problem is the same: NullPointerException . The rule is: “Guys, what are you doing? How so? Oh, trouble! Why do you first use an object, and then in the next line of code check it for null ?! ”. So it turns out here the dereference of the zero object. Alas, even the comment of one of the developers did not help.

V6060 The 'cause' reference was used before it was verified against null. StartupException.java (76), StartupException.java (73)

 private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString(); // <= consumer.accept(message); if (cause != null) { // <= // walk to the root cause while (cause.getCause() != null) { cause = cause.getCause(); } .... } .... } 

Here we must take into account that the getCause method of the Throwable class may return null . Further, the problem discussed above is repeated, and there is no point in explaining something in detail.

Meaningless terms

V6007 Expression 's.charAt (i)! =' \ T '' is always true. Cron.java (1223)

 private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) { // intentionally empty } return i; } 

The function in question returns the index of the first space, starting at index i . What's wrong? We have a warning from the analyzer that s.charAt (i)! = '\ T' is always true, which means that the expression will always be true (s.charAt (i)! = '' || s.charAt (i)! = '\ t') . Is it so? I think you yourself can easily verify this by substituting any character.

As a result, this method will always return an index equal to s.length () , which is not true. I dare to suggest that this method lies slightly above all the fault:

 private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) { // intentionally empty } return i; } 

This method was implemented, then copied and, making minor changes, got our wrong method findNextWhiteSpace . The method was corrected, corrected, but not corrected. To correct the situation, you must use the && operator instead of || .

V6007 Expression 'remaining == 0' is always false. PemUtils.java (439)

 private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) { // <= break; } .... } .... } 

From the condition of the cycle copied <keyLength it can be noted that copied will always be less than keyLength . Hence, a comparison for equality of the variable remaining with 0 is meaningless and will always give a false result, and therefore the conditional exit from the loop will not be performed. Is this code worth removing, or is it still necessary to revise the logic of behavior? I think only developers will be able to dot the i.

V6007 Expression 'healthCheckDn.indexOf (' = ')> 0' is always false. ActiveDirectorySessionFactory.java (73)

 ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... } 

Again, meaningless expression. According to the condition that the lambda expression returns the string variable healthCheckDn , the string healthCheckDn must be both empty and the string containing the '=' character not in the first position. Fuh, sort of sorted out. And as you correctly understood, it is impossible. We will not understand the logic of the code, leave it to the discretion of the developers.

I gave only some erroneous examples, but in addition there were plenty of cases of the V6007 diagnostic triggered , which need to be considered separately and draw appropriate conclusions.

The method is small, but remote

 private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; } 

So, we have a tiny method from several lines. But the bugs are not asleep! Analysis of this method gave the following result:

1. V6007 Expression '(int) x <0' is always false. BCrypt.java (429)
2. V6025 Possibly index '(int) x' is out of bounds. BCrypt.java (431)

Problem N1. The expression (int) x <0 is always false (Yes, yes, again V6007 ). The variable x cannot be negative, since it is of type char . The type char is an unsigned integer. This cannot be called a real mistake, but, nevertheless, verification is redundant and can be removed.

Problem N2. Possible overrun of the array, resulting in an ArrayIndexOutOfBoundsException exception. Then a question arises that lies on the surface: “Stand, but what about the index check?”

So, we have a fixed-size array of 128 elements:

 private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 }; 

When the variable x enters the input of the char64 method, a check is made for the validity of the index. Where is the gap? Why is it still possible to exit the array?

Check (int) x> index_64.length is not entirely correct. If the input method char64 comes x with a value of 128, then the check will not protect against ArrayIndexOutOfBoundsException . Perhaps this never happens in reality. However, the check is written incorrectly, and the “greater” (>) operator must be replaced with “greater than or equal to” (> =).

Comparisons that tried

V6013 Numbers 'displaySize' and 'that.displaySize' are compared by reference. Possibly an equality comparison was intended. ColumnInfo.java (122)

 .... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize && // <= Objects.equals(table, that.table) && Objects.equals(name, that.name) && Objects.equals(esType, that.esType); } 

The incorrectness here is that the displaySize objects of the Integer type are compared through the == operator, that is, they are compared by reference. It is quite possible that the ColumnInfo objects will be compared, whose displaySize fields have different links, but the same content. And in this case, the comparison will give us a negative result, while we expected the truth.

I would venture to suggest that such a comparison could be the result of unsuccessful refactoring, and initially the displaySize field was of type int .

V6058 The 'equals' function types of incompatible types: Integer, TimeValue. DatafeedUpdate.java (375)

 .... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay())) // <= && ....) } 

Again, incorrect comparison of objects. Now compare objects whose types are incompatible ( Integer and TimeValue ). The result of this comparison is obvious, and it is always false. It can be seen that class fields are compared in the same type with each other, only the field names need to be changed. So, the developer decided to speed up the process of writing code with copy-paste, but by doing so he rewarded himself with a bug. A getter for the scrollSize field is implemented in the class , so to correct the error, the correct solution would be to use the appropriate method: datafeed .getScrollSize () .

Consider a couple of examples of errors without any explanation. The problem is already obvious.

V6001 There are identical sub-expressions of the operator. TermVectorsResponse.java (152)

 @Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis // <= && Objects.equals(termVectorList, other.termVectorList); } 

V6009 Function 'equals' receives an odd argument. An object 'shardId.getIndexName ()' is used. SnapshotShardFailure.java (208)

 @Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) && // <= Objects.equals(reason, that.reason) && Objects.equals(nodeId, that.nodeId) && status.getStatus() == that.status.getStatus(); } 

miscellanea

V6006 The object was not used. The 'throw' keyword could be missing. JdbcConnection.java (88)

 @Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } } 

The bug is obvious and does not require clarification. The developer created an exception, but does not forward it any further. Such an anonymous exception will be successfully created, and also successfully and, most importantly, destroyed without a trace. The reason is the absence of a throw statement.

V6003 The use of if (A) {....} else if (A) {....} pattern was detected. There is a possibility of logical error presence. MockScriptEngine.java (94), MockScriptEngine.java (105)

 @Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... } 

In the construction of a multiple if-else, one of the conditions is repeated twice, so the situation requires a competent review of the code.

V6039 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the statement is senseless. SearchAfterBuilder.java (94), SearchAfterBuilder.java (93)

 public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue; // <= if (values[i] instanceof Boolean) continue; // <= throw new IllegalArgumentException(....); } .... } 

The same condition is used twice in a row. Is the second condition superfluous, or is it necessary to use another type instead of Boolean ?

V6009 Function 'substring' receives an odd arguments. The 'queryStringIndex + 1' argument should not be greater than 'queryStringLength'. LoggingAuditTrail.java (660)

 LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,// <= queryStringLength)); // <= } .... } 

Immediately consider an error scenario that can throw a StringIndexOutOfBoundsException . An exception will occur when request.uri () returns a string that contains the character '#' earlier than '?'. In such a case, there are no checks in the method, and if this does happen, then trouble cannot be avoided. Perhaps this will never happen because of the various checks of the request object outside the method, but in my opinion, the idea is not the best.

Conclusion

Over the years, PVS-Studio has helped to find defects in the code of commercial and free open-source projects. More recently, Java has been added to support the languages ​​being analyzed. And one of the first tests for our newcomer was the actively developing Elasticsearch. We hope that this check will be useful for the project and interesting for readers.

For PVS-Studio for Java to quickly adapt to a new world, new tests, new users, active feedback and customers are needed :). So, I suggest, without delay, download and test our analyzer on your working draft!

If you want to share this article with an English-speaking audience, then please use the link to the translation: Maxim Stefanov. PVS-Studio for Java hits the road. Next stop is Elasticsearch