Searching in a search: let’s check Elasticsearch
This is one of the biggest open-source Java projects. Many enterprises, including GitHub, Netflix, and Amazon, use Elasticsearch. It’s been six years since we’ve checked the project, so what new bugs could emerge in such a long time?
What will we discuss?
As the introduction goes, we checked the project six (!) years ago. You can read that article here.
The analyzer has issued thousands of warnings, but to avoid turning this article into a novel on how to configure and review warnings, let’s focus on the ones that caught our eye during a quick look.
First of all, a quick refresher on what Elastricsearch is: it’s one of the most successful search engines trusted by both enterprises and startups.
The flexibility of the engine doesn’t end there: it can run on a laptop and enable you to search within a single folder, or scale across multiple distributed servers and process petabytes of data.
Its large codebase (three million lines of Java code) that powers the project is also worth mentioning. Let’s move on to the check.
Disclaimer: code formatting (indentations, line breaks) may slightly differ from the original, but the main idea of it remains completely intact.
Typos
We’ll start with one of the developers’ worst enemies: typos.
Field by field
Do you write getters often? We hope not, because it’s one of the least interesting things to do. Code generators or handy tools like Lombok save us from boilerplate code.
private final boolean isRunning;
private final boolean isAsync;
….
public boolean isRunning() {
return isRunning;
}
public boolean isAsync() {
return isRunning;
}Let’s look at the `isRunning` and `isAsync` variables, or rather at their getters. The `#isRunning()` getter returns the `isRunning` value, and `#isAsync()` returns… the same `isRunning` value. The second getter should return `isAsync`, but the code author made a simple but dangerous typo.
The PVS-Studio warning: V6091 Suspicious getter implementation. The ‘isAsync’ field should probably be returned instead. EsqlQueryResponse.java 180
This wasn’t the only instance, we’ve found another getter that doesn’t work as expected:
boolean isDirectory;
boolean isSymbolicLink;
....
@Override
public boolean isDirectory() {
return isDirectory;
}
@Override
public boolean isSymbolicLink() {
return isDirectory;
}Here, the code author mixed up the `isDirectory` variable with `isSymbolicLink`.
PVS-Studio issued the same warning: V6091 Suspicious getter implementation. The ‘isSymbolicLink’ field should probably be returned instead. DockerFileAttributes.java 67
“equals” == “==”
Newcomers to Java may ask a fairly logical question, “What is the difference between `equals` and `==`?” Let’s look at an example:
private void assertValidJsonInput(String content) {
if (testResponse && ("js" == language || "console-result" == language)
&& null == skip) {
.....
}
}The PVS-Studio analyzer issues the following warnings for the code above:
V6013 Strings ‘“js”’ and ‘language’ are compared by reference. Possibly an equality comparison was intended. SnippetBuilder.java 232
V6013 Strings ‘“console-result”’ and ‘language’ are compared by reference. Possibly an equality comparison was intended. SnippetBuilder.java 232
One might ask, “What’s wrong, though? I’ve written code like this before, and it worked fine”. The reason it may seem to work lies in the string pool, which reduces the number of string instances to avoid recurrent memory allocation. However, there’s no guarantee that any given string will be in the pool. It’s possible for two strings with the same content to refer to different instances, and a comparison by reference (`==`) will return `false`. So, using `equals` to compare strings is recommended.
While reference comparison may work with strings, it will never yield the correct result with collections (unless we include comparing the same instance to itself).
private final List<Pipe> values;
....
@Override
public final Pipe resolveAttributes(AttributeResolver resolver) {
List<Pipe> newValues = new ArrayList<>(values.size());
for (Pipe v : values) {
newValues.add(v.resolveAttributes(resolver));
}
if (newValues == values) { // <=
return this;
}
return replaceChildrenSameSize(newValues);
}Here, the newly created `newValues` object is compared to the `values` field, which is always `false`. Most likely, the code author wanted to check whether the new list contains the same items as the existing one, so as not to overwrite it unnecessarily.
For this error, the analyzer issued the following warning: V6013 Objects ‘newValues’ and ‘values’ are compared by reference. Possibly an equality comparison was intended. ConcatFunctionPipe.java 41
I’m right, and I’ve left it behind
Code completion is deeply rooted in the development process. Along with the benefits, it also brings a hefty number of new typos. Let’s take a look at one of those:
public void testReadSlices() throws IOException {
....
try (StreamInput input1 = bytesReference.streamInput();
StreamInput input2 = bytesReference.streamInput()) {
for (int i = 0; i < refs; i++) {
boolean sliceLeft = randomBoolean();
BytesReference left = sliceLeft ?
input1.readSlicedBytesReference()
: input1.readBytesReference();
if (sliceLeft && bytesReference.hasArray()) {
assertSame(left.array(), bytesReference.array()); // <=
}
boolean sliceRight = randomBoolean();
BytesReference right = sliceRight ?
input2.readSlicedBytesReference()
: input2.readBytesReference();
assertEquals(left, right);
if (sliceRight && bytesReference.hasArray()) {
assertSame(right.array(), right.array()); // <=
}
}
}If we compare the first highlighted line of code with the second, we can assume that the second one implied the following:
assertSame(right.array(), bytesReference.array());However, the original code compares the result of the `array()` function for the same `right` object.
The PVS-Studio warning: V6009 Function ‘assertSame’ receives an odd argument. The ‘right.array()’ argument was passed several times. AbstractBytesReferenceTestCase.java 713
Let’s say it’s equal
We’ve already discussed the difference between `equals` and `==`. Now, we’ll take a closer look at `equals`, or rather dive into possible errors in its implementation:
@Override
public boolean equals(Object obj) {
....
KeyedFilter other = (KeyedFilter) obj;
return Objects.equals(keys, other.keys)
&& Objects.equals(timestamp, other.timestamp)
&& Objects.equals(tiebreaker, other.tiebreaker)
&& Objects.equals(child(), other.child())
&& isMissingEventFilter == isMissingEventFilter; // <=
}Developers forgot to put `other` before the second `isMissingEventFilter` here. Due to this small typo, the `isMissingEventFilter` parameter is completely ignored in the comparison.
Here’s the PVS-Studio warning: V6001 There are identical sub-expressions ‘isMissingEventFilter’ to the left and to the right of the ‘==’ operator. KeyedFilter.java 116
Take a look at another typo in the `equals` implementation:
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
IndexError that = (IndexError) o;
return indexName.equals(that.indexName)
&& Arrays.equals(shardIds, that.shardIds)
&& errorType == that.errorType
&& message.equals(that.message)
&& stallTimeSeconds == stallTimeSeconds; // <=
}Similar to the previous `equals`, here they forgot to add the `that` prefix to the second `stallTimeSeconds`.
Notably, in both cases, the typo occurred in the last line, which is a real development phenomenon. You can read the article on this “last line effect” if you want to dig deeper.
The PVS-Studio analyzer issued the following warning for this code: V6001 There are identical sub-expressions ‘stallTimeSeconds’ to the left and to the right of the ‘==’ operator. IndexError.java 147
Do you like math? What about math in code?
Regardless of your answers, wouldn’t you agree that it’s easy to make a typo in such math code? Take a look at one of such cases:
private static LinearRing parseLinearRing(
ByteBuffer byteBuffer,
boolean hasZ, boolean coerce
) {
....
double[] lons = new double[length];
double[] lats = new double[length];
....
if (linearRingNeedsCoerced(lats, lons, alts, coerce)) { // <=
lons = coerce(lons);
lats = coerce(lats);
if (hasZ) {
alts = coerce(alts);
}
}
....
}What’s wrong with this code, though? To find the answer, let’s take a look at the arguments of the `linearRingNeedsCoerced` method.
private static boolean linearRingNeedsCoerced(
double[] lons, double[] lats,
double[] alts,
boolean coerce
) {
assert lats.length == lons.length
&& (alts == null || alts.length == lats.length);
assert lats.length > 0;
if (coerce == false) {
return false;
}
final int last = lons.length - 1;
return lons[0] != lons[last]
|| lats[0] != lats[last]
|| (alts != null && alts[0] != alts[last]);
}After comparing the passed and expected arguments, we can conclude that devs mixed up `lons` and `lats`. By chance, this code doesn’t cause an error. However, any change in how these arguments are checked could cause the method to behave incorrectly.
The PVS-Studio warning: V6029 Possible incorrect order of arguments passed to method: ‘lats’, ‘lons’. WellKnownBinary.java 370
Null is null. What’s next, though?
Let’s look at this code fragment:
public static void assertDeepEquals(ElasticsearchException expected,
ElasticsearchException actual) {
do {
if (expected == null) {
assertNull(actual);
} else {
assertNotNull(actual);
}
assertEquals(expected.getMessage(), actual.getMessage());
....
}
....
}When `expected == null`, it’s checked whether `actual` is also `null`. That should’ve been the end of the similarity check, but it wasn’t. Object properties are checked next, and if `actual` proves to be `null`, a null-pointer dereference is possible.
This is the warning the analyzer issues for the code snippet: V6008 Potential null dereference of ‘expected’. ElasticsearchExceptionTests.java 1354
Shall we check? Nope
There’s a lot to keep track of during the development process, so it’s easy to miss a little thing like the one in the code below:
private FinishFunction(....) {
....
for (int p = 1; p < fn.getParameters().size(); p++) {
VariableElement param = fn.getParameters().get(p);
if (p == 0) { // <=
if (false == TypeName.get(param.asType()).equals(workType)) {
throw new IllegalArgumentException(
"First argument of "
+ declarationType + "#" + fn.getSimpleName()
+ " must have type " + workType
);
}
continue;
}
....
}
invocationPattern = pattern.append(")").toString();
}Here, `for` starts at index one, but the body includes a type check for the first element. Most likely, the code author just forgot to adjust the loop boundaries — resulting in the check that will never be executed.
The PVS-Studio analyzer always remembers such subtleties and gives a warning if a developer forgets: V6007 Expression ‘p == 0’ is always false. MvEvaluatorImplementer.java 456
A committer of commits once committed an overcommit
Without further explanation, let’s take a look at the code below:
public void testDLS() throws Exception {
....
int numDocs = scaledRandomIntBetween(32, 128);
int commitAfter = scaledRandomIntBetween(1, numDocs);
....
for (int doc = 1; doc <= numDocs; doc++) {
....
if (doc % 11 == 0) {
iw.deleteDocuments(new Term("id", id));
} else {
if (commitAfter % commitAfter == 0) { // <=
iw.commit();
}
valuesHitCount[valueIndex]++;
}
}
....
}In this snippet, you may notice that the remainder of dividing `commitAfter` by itself (which is always zero) is compared to 0. As a result, this condition is always true, which causes `iw.commit()` to be executed after every document is created.
PVS-Studio issues the following warning for the suspicious operation: V6001 There are identical sub-expressions ‘commitAfter’ to the left and to the right of the ‘%’ operator. SecurityIndexReaderWrapperIntegrationTests.java 157
What’s going on in this code? The `commitAfter` variable is randomly generated in the range from 1 to `numDocs`. Then, the loop iterates over all numbers from 1 to `numDocs`, and if the remainder of dividing `commitAfter` is zero, the created documents are committed. The division remainder here is responsible for committing every `commitAfter` elements. The fixed condition looks like this:
doc % commitAfter == 0Never let it go
Let’s look at two code fragments:
@Override
public void setAutoCommit(boolean autoCommit) throws SQLException {
checkOpen();
if (autoCommit == false) {
new SQLFeatureNotSupportedException("Non auto-commit is not supported");
}
}
private String getProperty(String propertyName) {
final String[] settings = getConnectString().split(";");
for (int i = 0; i < settings.length; i++) {
String setting = settings[i].trim();
if (setting.length() > 0) {
final int idx = setting.indexOf('=');
if (idx == -1 || idx == 0 || idx == settings[i].length() - 1) {
new IllegalArgumentException("Invalid connection string: " // <=
+ getConnectString());
}
if (propertyName.equals(setting.substring(0, idx))) {
return setting.substring(idx + 1);
}
}
}
return null;
}You’ve probably noticed that both fragments have one thing in common: an exception is created but thrown (away), so it’s left there high and dry until the JIT compiler realizes that line is dead code.
It’s worth noting that the first error has been around for a long time — we already covered it in the first Elasticsearch check.
PVS-Studio issued two warnings respectively:
V6006 The object was created but it is not being used. The ‘throw’ keyword could be missing. JdbcConnection.java 93
V6006 The object was created but it is not being used. The ‘throw’ keyword could be missing. AzureStorageSettings.java 352
To become the best version just a copy of yourself
Let’s look at an error caused by a variable overlap:
private final PatternSet patternSet = new PatternSet().include("**/*.class");
....
public void setPatternSet(PatternSet patternSet) {
patternSet.copyFrom(patternSet);
}The code author likely intended the above setter to copy the state of the passed `patternSet` to the class field, but the object ends up copying its own state due to the missing `this` keyword.
Let’s look at the warning that the analyzer issued for this suspicious method call: V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the ‘copyFrom’ method. CheckForbiddenApisTask.java 157
Ctrl+C, Ctrl+V, Ctrl+V, Ctrl+V, Ctr… Oops
At first glance, the following code contains a minor error: a redundant check for `null`. Is that it, though? I invite you to try finding a more serious typo — the name of the section is the clue.
private void test(Snippet test) {
setupCurrent(test);
if (test.continued()) {
/* Catch some difficult to debug errors with // TEST[continued]
* and throw a helpful error message. */
if (previousTest == null
|| previousTest.path().equals(test.path()) == false) {
throw new InvalidUserDataException("// TEST[continued] "
+ "cannot be on first snippet in a file: " + test);
}
if (previousTest != null && previousTest.testSetup()) {
throw new InvalidUserDataException("// TEST[continued] "
+ "cannot immediately follow // TESTSETUP: " + test);
}
if (previousTest != null && previousTest.testSetup()) {
throw new InvalidUserDataException("// TEST[continued] "
+ "cannot immediately follow // TEARDOWN: " + test);
}
}
....
}It’s great if you caught the typo, and if you didn’t, we’ll walk through it.
Take a look at the conditions in the last two `if` blocks — they’re identical. Could it be done on purpose, though? Now, let’s look at the `then` blocks — specifically at the exception messages that are thrown there. They’re different. Most likely, the code author wanted to check two different conditions, but the copy-paste turned the code into what it is now.
We can assume that the second condition should’ve looked like this (let’s not forget that we can remove the `null` check):
if (previousTest.testTearDown()) {
....
}Let’s also take a look at the warnings the PVS-Studio analyzer issued for this code snippet:
V6007 Expression ‘previousTest != null’ is always true. RestTestsFromDocSnippetTask.java 247
V6007 Expression ‘previousTest != null’ is always true. RestTestsFromDocSnippetTask.java 249
A check, and another check, and another one
It’s worth checking the input data, especially if it came from a user. In this section, we’ll look at the checks that have gone too far.
A token or a token?
Let’s start with a strange check of a current token.
protected BulkByScrollTask.Status doParseInstance(
XContentParser parser
) throws IOException {
XContentParser.Token token;
if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
token = parser.nextToken(); // <=
} else {
token = parser.nextToken(); // <=
}
ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
token = parser.nextToken();
ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
return innerParseStatus(parser);
}We’ll break down the above method. The `parser` object is passed in, we check its current token. If it’s the start of the object, we move to the next one, but if it’s not… we do the same! The most interesting thing is that right after that, we check whether this token is also the start of an object!
Could this odd behavior be the result of multiple people editing the code? Unlikely. This method showed up in a single commit and has never been changed since.
The PVS-Studio warning: V6004 The ‘then’ statement is equivalent to the ‘else’ statement. BulkByScrollTaskStatusTests.java 189
You can’t see the lines. Now you still can’t see them
Let’s take a look at another redundant check:
@Override
protected StringBuilder contentToWKT() {
final StringBuilder sb = new StringBuilder();
if (lines.isEmpty()) { // <=
sb.append(GeoWKTParser.EMPTY);
} else {
sb.append(GeoWKTParser.LPAREN);
if (lines.size() > 0) { // <=
sb.append(ShapeBuilder
.coordinateListToWKT(lines.get(0).coordinates)
);
}
for (int i = 1; i < lines.size(); ++i) {
sb.append(GeoWKTParser.COMMA);
sb.append(ShapeBuilder
.coordinateListToWKT(lines.get(i).coordinates)
);
}
sb.append(GeoWKTParser.RPAREN);
}
return sb;
}The code contains an obviously redundant `lines.size() > 0` check. Since we’ve already checked whether `lines` is empty, this execution branch will only be executed if there’s at least one element.
The PVS-Studio warning: V6007 Expression ‘lines.size() > 0’ is always true. MultiLineStringBuilder.java 81
A parenthesis? Are you sure about that?
Now we’ll poke around in the code a bit. First, let’s take a look at the following method:
private static List<Coordinate> parseCoordinateList(
StreamTokenizer stream,
final boolean ignoreZValue,
final boolean coerce
) throws IOException, ElasticsearchParseException {
....
while (nextCloserOrComma(stream).equals(COMMA)) {
....
if (isOpenParen && nextCloser(stream).equals(RPAREN) == false) { // <=
throw new ElasticsearchParseException(
"expected: " + RPAREN
+ " but found: " + tokenString(stream),
stream.lineno()
);
}
}
return coordinates.build();
}Unless you know the Elasticsearch code by heart, the error here probably won’t be obvious.
The PVS-Studio analyzer warning: V6007 Expression ‘nextCloser(stream).equals(RPAREN) == false’ is always false. GeoWKTParser.java 164
Now we know what the error is, but why is the result of the `nextCloser` function always equivalent to `RPAREN`? Let’s take a look at the method body:
private static String nextCloser(StreamTokenizer stream)
throws IOException, ElasticsearchParseException {
if (nextWord(stream).equals(RPAREN)) {
return RPAREN;
}
throw new ElasticsearchParseException(
"expected: " + RPAREN
+ " but found: " + tokenString(stream),
stream.lineno()
);
}Turns out that `nextCloser` either returns `RPAREN` or throws an exception! Also, note that it’s identical to the one in the method that triggered the warning.
We can simplify the code:
private static List<Coordinate> parseCoordinateList(
StreamTokenizer stream,
final boolean ignoreZValue,
final boolean coerce
) throws IOException, ElasticsearchParseException {
....
while (nextCloserOrComma(stream).equals(COMMA)) {
....
if (isOpenParen) {
nextCloser(stream);
}
}
return coordinates.build();
}Emptiness x2
Here’s an easy way to combine lists:
public static <T> List<T> combine(
List<? extends T> left,
List<? extends T> right
) {
if (right.isEmpty()) {
return (List<T>) left;
}
if (left.isEmpty()) {
return (List<T>) right;
}
List<T> list = new ArrayList<>(left.size() + right.size());
if (left.isEmpty() == false) { // <=
list.addAll(left);
}
if (right.isEmpty() == false) { // <=
list.addAll(right);
}
return list;
}Having checked whether the lists are empty, we don’t need to do it again when filling the union. I should note that this can’t be called an error, but it’s possible to write simpler and more compact code here.
PVS-Studio detected redundant checks and issued two warnings:
V6007 Expression ‘left.isEmpty() == false’ is always true. CollectionUtils.java 33
V6007 Expression ‘right.isEmpty() == false’ is always true. CollectionUtils.java 36
Do you have a substring? Do you? Let me check!
Let’s speed up and look at another not-so-obvious case:
@TaskAction
public void checkDependencies() {
....
for (File file : licensesDirAsFile.listFiles()) {
String name = file.getName();
if (name.endsWith("-LICENSE") || name.endsWith("-LICENSE.txt")) {
// TODO: why do we support suffix of LICENSE *and* LICENSE.txt??
licenses.put(name, false);
} else if (name.contains("-NOTICE")
|| name.contains("-NOTICE.txt")) { // <=
notices.put(name, false);
} else if (name.contains("-SOURCES")
|| name.contains("-SOURCES.txt")) { // <=
sources.put(name, false);
}
}
....
}This code works correctly, but we also can simplify it by removing redundant checks. Let’s walk through the logic, and to make sure there’s no confusion, we’ll use a specific example.
We have the `”FILENAME-NOTICE.txt”` string. Does it contain the `”-NOTICE.txt”` substring? And what about `”-NOTICE “`? Both checks are true. So, there’s no point in checking `”-NOTICE.txt”` and `”-NOTICE”` separately.
We have the exact same situation with the `”-SOURCES.txt”` and `”-SOURCES”`.
Take a look at the analyzer warnings:
V6007 Expression ‘name.contains(“-NOTICE.txt”)’ is always false. DependencyLicensesTask.java 228
V6007 Expression ‘name.contains(“-SOURCES.txt”)’ is always false. DependencyLicensesTask.java 230
That’s the way the cookie crumbles
To wrap up the section, let’s take a look at the code from one of the many tests:
public void testMinimumPerNode() {
int negativeShardsPerNode = between(-50_000, 0);
try {
if (frequently()) {
clusterAdmin().prepareUpdateSettings(....)
.setPersistentSettings(
Settings.builder()
.put(shardsPerNodeKey, negativeShardsPerNode)
.build()
)
.get();
} else {
clusterAdmin().prepareUpdateSettings(....)
.setPersistentSettings(
Settings.builder()
.put(shardsPerNodeKey, negativeShardsPerNode)
.build()
)
.get();
}
fail("should not be able to set negative shards per node");
} catch (IllegalArgumentException ex) {
....
}
}The PVS-Studio warning: V6004 The ‘then’ statement is equivalent to the ‘else’ statement. ClusterShardLimitIT.java 52
To see why `then` is equivalent to `else`, we’ll turn to Git History. The method appeared about six years ago. The `If`-`else` statement still made sense back then: settings could be set in two ways: by default, and by `setTransientSettings`. However, some time later, developers marked the latter method as deprecated. They replaced it with `setPersistentSettings`, but failed to notice that `then` and `else` became identical.
Is that how it works?
Let’s start by looking at a method for getting a random number with a random type (`double` or `float`):
protected Number randomNumber() {
/*
* The source parser and doc values round trip will both reduce
* the precision to 32 bits if the value is more precise.
* randomDoubleBetween will smear the values out across a wide
* range of valid values.
*/
return randomBoolean() ?
randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true)
: randomFloat();
}It seems that nothing unusual is happening: the method randomly returns either `float` or `double`.
The PVS-Studio warning: V6088 Result of this expression will be implicitly cast to ‘double’. Check if program logic handles it correctly. ScaledFloatFieldMapperTests.java 505
But how!? Could the issue be that `#randomFloat()` actually returns `double`? We’ll take a look at it, too:
public static float randomFloat() {
return random().nextFloat();
}Everything’s correct here as well. Could there be something wrong with the return type of `#randomDoubleBetween`?
public static double randomDoubleBetween(
double start, double end,
boolean lowerInclusive
) {
double result = 0.0;
....
return result;
}Is the analyzer wrong? We can assure you that it’s not. Assuming the & return type is a reference, the primitive value of the ternary operator is evaluated first, and it undergoes conversion in numeric contexts (`float` is cast to `double`). Only then the resulting value is boxed.
It’s also worth noting that Elasticseach has plenty of similar errors. But this isn’t about sloppy coding, the behavior is indeed unpredictable. A static analyzer that knows about such subtleties in implementation can save the day.
To fix this, we can convert the ternary operator to the regular `if`-`else`.
if (randomBoolean()) {
return randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true);
} else {
return randomFloat();
}Here’s an example of a similar error:
public static Number truncate(Number n, Number precision) {
....
return n instanceof Float ? result.floatValue() : result;
}The PVS-Studio analyzer issues the following warning: V6088 Result of this expression will be implicitly cast to ‘double’. Check if program logic handles it correctly. Maths.java 122
Let’s summarize
Kudos to the devs for developing and maintaining a project as large as Elasticsearch — with over three million lines of Java code, it’s a tremendous undertaking. However, we’re all human, and we all make mistakes that we can sometimes overlook.
In this article, we’ve shown only a small part of all the analyzer warnings, but the project has more. So, to avoid breaking the review into too many parts, we’ve highlighted the most noteworthy bugs (in our opinion).
If you’d like to try PVS-Studio, you can download the trial version. For open-source projects, we have a free licensing option.
