# PVS\-Studio for Java отправляется в путь\. Следующая остановка \- Elasticsearch

Далеко не первый год команда PVS\-Studio ведет блог о проверках open\-source проектов одноименным статическим анализатором кода\. На сегодняшний момент проверено более 300 проектов, а в базу найденных ошибок выписано более 12000 случаев\. Изначально анализатор был реализован для проверки C и C\+\+ кода, далее появилась поддержка языка C\#\. Поэтому среди проверенных проектов большая часть \(\> 80%\) приходится именно на C и C\+\+\. Совсем недавно к поддерживаемым языкам прибавился Java, а это значит, что перед PVS\-Studio открываются двери в новый мир, и пора дополнять базу ошибками из Java проектов\.

![0621_ElasticSearch_ru/image1.png](https://import.viva64.com/docx/blog/0621_ElasticSearch_ru/image1.png)

Java мир огромен и многообразен, поэтому глаза разбегаются при выборе проекта для испытания нового анализатора\. В конечном итоге, выбор пал на движок полнотекстового поиска и аналитики Elasticsearch\. Это достаточно успешный проект, а в успешных проектах находить ошибки вдвойне, а то и втройне приятнее\. Так что, какие же дефекты обнаружил PVS\-Studio для Java? О результате проверки и пойдет речь в статье\.

## Поверхностное знакомство с Elasticsearch

[Elasticsearch](https://www.elastic.co/elasticsearch/) \- это масштабируемый полнотекстовый поисковый и аналитический движок с открытым исходным кодом\. Он позволяет хранить большие объемы данных, проводить среди них быстрый поиск и аналитику \(почти в режиме реального времени\)\. Как правило, он используется в качестве базового механизма/технологии, которая обеспечивает работу приложений со сложными функциями и требованиями к поиску\.

Среди крупных сайтов, использующих Elasticsearch, отмечаются Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox\.

Думаю, со знакомством хватит\.

## Как всё было

С проверкой не возникло проблем\. Последовательность действий достаточно простая и не заняла много времени:

* Скачал Elasticsearch с [GitHub](https://github.com/elastic/elasticsearch);
* Воспользовался [инструкцией](https://pvs-studio.ru/ru/docs/manual/6703/) по запуску Java анализатора и запустил анализ;
* Получил отчет анализатора, проанализировал его и выделил интересные случаи\.

 Теперь давайте переходить к сути\.

## Осторожно\! Возможен NullPointerException

[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Null dereference of 'line'\. GoogleCloudStorageFixture\.java\(451\)

```cpp
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);
      }
    ....
  });
  ....
}
```

Ошибка в рассматриваемом фрагменте кода в том, что, если не смогли прочитать строку из буфера, то вызов метода _startsWith_ в условии оператора _if_ приведёт к выбросу исключения _NullPointerException_\. Скорее всего, это опечатка, и при написании условия имелся в виду оператор _&&_ вместо _\|\|_\.

[V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'followIndexMetadata'\. TransportResumeFollowAction\.java\(171\), TransportResumeFollowAction\.java\(170\), TransportResumeFollowAction\.java\(194\)

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

Еще одно предупреждение от [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) диагностики\. Теперь пристальный взгляд приковал объект _followIndexMetadata_\. Метод _start_ принимает несколько аргументов на вход, среди которых и наш подозреваемый\. После чего на основании проверки нашего объекта на _null_ формируется новый объект, который участвует в дальнейшей логике метода\. Проверка на _null_ говорит нам, что _followIndexMetadata_ все же может прийти извне нулевым объектом\. Хорошо, смотрим дальше\.

Дальше вызывается метод валидации с проталкиванием множества аргументов \(опять же, среди которых есть рассматриваемый объект\)\. И если посмотреть реализацию метода валидации, то все встает на свои места\. Наш потенциальный нулевой объект третьим аргументом передается в метод _validate_, где безусловно разыменовывается\. Как итог \- потенциальный _NullPointerException\._

```cpp
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(....);
  }
  ....
}}
```

С какими аргументами вызывается метод _start_ на самом деле \- неизвестно\. Вполне возможно, проверка всех аргументов производится где\-то там перед вызовом метода, и никакого разыменования нулевого объекта нам не грозит\. Но, согласитесь, что такая реализация кода всё же выглядит достаточно ненадежной и заслуживает внимания\.

[V6060](https://pvs-studio.ru/ru/docs/warnings/v6060/) The 'node' reference was utilized before it was verified against null\. RestTasksAction\.java\(152\), RestTasksAction\.java\(151\)

```cpp
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());
  ....
}
```

Здесь сработало другое диагностическое правило, а проблема всё та же: _NullPointerException_\. Правило гласит: "Ребят, что вы делаете? Да как же так? Ох, беда\! Зачем вы сначала используете объект, а потом в следующей строчке кода проверяете его на _null_?\!"\. Так и получается здесь разыменование нулевого объекта\. Увы, не помог даже комментарий одного из разработчиков\. 

[V6060](https://pvs-studio.ru/ru/docs/warnings/v6060/) The 'cause' reference was utilized before it was verified against null\. StartupException\.java\(76\), StartupException\.java\(73\)

```cpp
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();
        }
        ....
    }
    ....
}
```

Здесь надо принять во внимание, что метод _getCause_ класса _Throwable_ может вернуть _null_\. Дальше рассматриваемая выше проблема повторяется, и подробно что\-то разъяснять нет смысла\. 

## Бессмысленные условия

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 's\.charAt\(i\) \!\= '\\t'' is always true\. Cron\.java\(1223\)

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

Рассматриваемая функция возвращает индекс первого пробела, начиная с индекса _i_\. Что не так? Мы имеем предупреждение от анализатора, что _s\.charAt\(i\) \!\= '\\t'_ всегда истина, а значит, всегда истина будет и выражение _\(s\.charAt\(i\) \!\= ' ' \|\| s\.charAt\(i\) \!\= '\\t'\)_\. Так ли это?  Думаю, вы сами с легкостью сможете в этом убедиться, подставив любой символ\.

В итоге, этот метод будет возвращать всегда индекс, равный _s\.length\(\)_, что не верно\. Я осмелюсь предположить, что всему виной этот чуть выше расположенный метод: 

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

Этот метод реализовали, потом скопировали и, внеся небольшие правки, получили наш ошибочный метод _findNextWhiteSpace_\. Метод корректировали, корректировали, да не скорректировали\. Для исправления ситуации необходимо использовать оператор _&&_ вместо _\|\|_\.

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'remaining \=\= 0' is always false\. PemUtils\.java\(439\)

```cpp
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;
    }
    ....
  }
  ....
}
```

Из условия цикла _copied < keyLength_ можно отметить, что _copied_ всегда  будет меньше _keyLength_\. Отсюда, сравнение на равенство переменной _remaining_ с 0 бессмысленно и всегда будет давать ложный результат, в связи с чем выход из цикла по условию не будет осуществлен\. Этот код стоит удалить, или всё же надо пересмотреть логику поведения? Думаю, только разработчики смогут расставить все точки над i\.

[V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'healthCheckDn\.indexOf\('\='\) \> 0' is always false\. ActiveDirectorySessionFactory\.java\(73\)

```cpp

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

Снова бессмысленное выражение\. Согласно условию, чтобы лямбда\-выражение вернуло строковую переменную _healthCheckDn_, строка _healthCheckDn_ должна быть одновременно и пустой, и строкой, содержащей символ '\=' не на первой позиции\. Фух, вроде разобрались\. И как вы правильно поняли, это невозможно\. Не будем разбираться в логике кода, оставим на усмотрение разработчикам\.

Я привел только некоторые ошибочные примеры, но помимо этого нашлось предостаточно случаев срабатывания диагностики [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/), которые нужно рассматривать отдельно и делать соответствующие выводы\. 

## Метод маленький, да удаленький

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

Итак, имеем малюсенький метод из нескольких строк\. Но баги не дремлют\! Анализ этого метода дал следующий результат:

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

Проблема N1\. Выражение _\(int\)x < 0_ всегда ложно \(Да, да, опять [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/)\)\. Переменная _x_ не может быть отрицательной, так как она типа _char_\. Тип _char_ представляет собой беззнаковое целое число\. Это нельзя назвать настоящей ошибкой, но, тем не менее, проверка избыточна и её можно удалить\.

Проблема N2\. Возможный выход за границы массива, приводящий к исключению _ArrayIndexOutOfBoundsException_\. Тогда напрашивается вопрос, который лежит на поверхности: "Стойте, а как же проверка индекса?"

Итак, у нас имеется массив фиксированного размера из 128 элементов:

```cpp
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
};
```

Когда на вход метода _char64_ поступает переменная _x_, производится проверка на валидность индекса\. Где же брешь? Почему все еще возможен случай выхода за пределы массива? 

Проверка _\(int\)x \> index\_64\.length_ не совсем корректна\. Если на вход метода _char64_ придет _x_ со значением 128, то проверка не защитит от _ArrayIndexOutOfBoundsException_\. Возможно, такое никогда не происходит в реальности\. Тем не менее, проверка написана неправильно, и надо заменить оператор "больше" \(\>\) на "больше или равно" \(\>\=\)\.

## Сравнения, которые пытались

[V6013](https://pvs-studio.ru/ru/docs/warnings/v6013/) Numbers 'displaySize' and 'that\.displaySize' are compared by reference\. Possibly an equality comparison was intended\. ColumnInfo\.java\(122\)



```cpp
....
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);
}
```

Некорректность тут в том, что сравниваются объекты _displaySize_ типа _Integer_ через оператор _\=\=_, то есть сравниваются по ссылке\. Вполне возможен сценарий, что будут сравниваться объекты _ColumnInfo_, у которых поля _displaySize_ имеют разные ссылки, но одинаковое содержимое\. И в таком случае сравнение даст нам отрицательный результат, в то время как мы ожидали истину\.

Рискну предположить, что такое сравнение могло быть результатом неудачного рефакторинга, и изначально поле _displaySize_ имело тип _int_\.  

[V6058](https://pvs-studio.ru/ru/docs/warnings/v6058/) The 'equals' function compares objects of incompatible types: Integer, TimeValue\. DatafeedUpdate\.java\(375\)

```cpp
....
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())) // <=
    && ....)
}
```

И снова некорректное сравнение объектов\. Теперь сравнивают объекты, у которых типы несовместимы \(_Integer_ и _TimeValue_\)\. Результат такого сравнения очевиден, и это всегда false\. Видно, что однотипно сравниваются поля класса между собой, необходимо менять только имена полей\. Так вот, разработчик решил ускорить процесс написания кода copy\-paste'ом, но этим самым и наградил себя багом\. В классе реализован геттер для поля _scrollSize_, поэтому для исправления ошибки правильным решением будет использование соответствующего метода: _datafeed \.getScrollSize\(\)_ \.

Рассмотрим еще пару примеров ошибок без каких\-либо пояснений\. Проблема и так очевидна\.

[V6001](https://pvs-studio.ru/ru/docs/warnings/v6001/) There are identical sub\-expressions 'tookInMillis' to the left and to the right of the '\=\=' operator\. TermVectorsResponse\.java\(152\)

```cpp
@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](https://pvs-studio.ru/ru/docs/warnings/v6009/) Function 'equals' receives an odd argument\. An object 'shardId\.getIndexName\(\)' is used as an argument to its own method\. SnapshotShardFailure\.java\(208\)

```cpp
@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();
}
```

## Разное

[V6006](https://pvs-studio.ru/ru/docs/warnings/v6006/) The object was created but it is not being used\. The 'throw' keyword could be missing\. JdbcConnection\.java\(88\)

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

Баг очевиден и не требует разъяснения\. Разработчик создал исключение, но никак его дальше не пробрасывает\. Такое анонимное исключение успешно создастся, а также успешно и, самое главное, бесследно уничтожится\. Причина \- отсутствие оператора _throw_\.  

[V6003](https://pvs-studio.ru/ru/docs/warnings/v6003/) The use of 'if \(A\) \{\.\.\.\.\} else if \(A\) \{\.\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. MockScriptEngine\.java\(94\), MockScriptEngine\.java\(105\)

```cpp
@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)) 
  ....
}
```

В конструкции множественного _if\- else_ одно из условий повторяется дважды, поэтому ситуация требует компетентного обзора кода\.

[V6039](https://pvs-studio.ru/ru/docs/warnings/v6039/) There are two 'if' statements with identical conditional expressions\. The first 'if' statement contains method return\. This means that the second 'if' statement is senseless\. SearchAfterBuilder\.java\(94\), SearchAfterBuilder\.java\(93\)

```cpp
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(....);
  }
  ....
}
```

Дважды подряд используется одно и то же условие\. Второе условие лишнее, или все же вместо _Boolean_ нужно использовать другой тип?   

[V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) Function 'substring' receives an odd arguments\. The 'queryStringIndex \+ 1' argument should not be greater than 'queryStringLength'\. LoggingAuditTrail\.java\(660\)

```cpp
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)); // <=
  }
  ....
}
```

Сразу же рассмотрим ошибочный сценарий, который может вызвать исключение _StringIndexOutOfBoundsException_\. Исключение возникнет тогда, когда _request\.uri\(\)_ вернет строку, которая содержит символ '\#' раньше, чем '?'\. На такой случай никаких проверок в методе нет, и, если такое все же случится, то не избежать беды\. Возможно, такого никогда не произойдет из\-за различных проверок объекта _request_ вне метода, но надеяться на это, по\-моему, идея не из лучших\.

## Заключение

PVS\-Studio на протяжении многих лет помогает находить дефекты в коде коммерческих и бесплатных open\-source проектов\. Совсем недавно к поддержке анализируемых языков прибавился Java\. И одним из первых испытаний для нашего новичка стал активно развивающийся Elasticsearch\. Надеемся, что эта проверка окажется полезной для проекта и интересной для читателей\.

Чтобы PVS\-Studio для Java быстро адаптировался в новом для себя мире, нужны новые испытания, новые пользователи, активная обратная связь и клиенты :\)\. Так что, предлагаю, не откладывая, [скачать](https://pvs-studio.ru/ru/pvs-studio/download/) и испытать на своем рабочем проекте наш анализатор\!