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

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

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

Elasticsearch ^[1] — это масштабируемый полнотекстовый поисковый и аналитический движок с открытым исходным кодом. Он позволяет хранить большие объемы данных, проводить среди них быстрый поиск и аналитику (почти в режиме реального времени). Как правило, он используется в качестве базового механизма/технологии, которая обеспечивает работу приложений со сложными функциями и требованиями к поиску.

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

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

Как всё было

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

• Скачал Elasticsearch с GitHub ^[2];
• Воспользовался инструкцией ^[3] по запуску Java анализатора и запустил анализ;
• Получил отчет анализатора, проанализировал его и выделил интересные случаи.

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

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

V6008 ^[4] 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);
      }
    ....
  });
  ....
}

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

V6008 ^[4] 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);
  ....
}

Еще одно предупреждение от V6008 ^[4] диагностики. Теперь пристальный взгляд приковал объект followIndexMetadata. Метод start принимает несколько аргументов на вход, среди которых и наш подозреваемый. После чего на основании проверки нашего объекта на null формируется новый объект, который участвует в дальнейшей логике метода. Проверка на null говорит нам, что followIndexMetadata все же может прийти извне нулевым объектом. Хорошо, смотрим дальше.

Дальше вызывается метод валидации с проталкиванием множества аргументов (опять же, среди которых есть рассматриваемый объект). И если посмотреть реализацию метода валидации, то все встает на свои места. Наш потенциальный нулевой объект третьим аргументом передается в метод validate, где безусловно разыменовывается. Как итог — потенциальный 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(....);
  }
  ....
}}

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

V6060 ^[5] The 'node' reference was utilized before it was verified against null. 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());
  ....
}

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

V6060 ^[5] The 'cause' reference was utilized 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();
        }
        ....
    }
    ....
}

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

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

V6007 ^[6] 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;
}

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

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

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 ^[6] 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;
    }
    ....
  }
  ....
}

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

V6007 ^[6] 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 ....;
        },
        ....);
  ....
}

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

Я привел только некоторые ошибочные примеры, но помимо этого нашлось предостаточно случаев срабатывания диагностики V6007 ^[6], которые нужно рассматривать отдельно и делать соответствующие выводы.

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

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)
2. V6025 Possibly index '(int) x' is out of bounds. BCrypt.java(431)

Проблема N1. Выражение (int)x < 0 всегда ложно (Да, да, опять V6007 ^[6]). Переменная x не может быть отрицательной, так как она типа char. Тип char представляет собой беззнаковое целое число. Это нельзя назвать настоящей ошибкой, но, тем не менее, проверка избыточна и её можно удалить.

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

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

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 ^[7] 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);
}

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

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

V6058 ^[8] The 'equals' function compares objects 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())) // <=
    && ....)
}

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

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

V6001 ^[9] There are identical sub-expressions 'tookInMillis' to the left and to the right 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 ^[10] Function 'equals' receives an odd argument. An object 'shardId.getIndexName()' is used as an argument to its own method. 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();
}

Разное

V6006 ^[11] The object was created but it is not being used. The 'throw' keyword could be missing. JdbcConnection.java(88)

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

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

V6003 ^[12] 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)

@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 ^[13] 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)

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 ^[10] 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)); // <=
  }
  ....
}

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

Заключение

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

Чтобы PVS-Studio для Java быстро адаптировался в новом для себя мире, нужны новые испытания, новые пользователи, активная обратная связь и клиенты :). Так что, предлагаю, не откладывая, скачать ^[14] и испытать на своем рабочем проекте наш анализатор!

Автор: stefanbuzz

Источник ^[15]