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

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

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

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

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

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

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

Как всё было

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

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

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

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

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

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

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

Еще одно предупреждение от V6008 диагностики. Теперь пристальный взгляд приковал объект 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 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 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 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 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 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, которые нужно рассматривать отдельно и делать соответствующие выводы.

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

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). Переменная 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 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 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 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 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 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 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 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 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 быстро адаптировался в новом для себя мире, нужны новые испытания, новые пользователи, активная обратная связь и клиенты :). Так что, предлагаю, не откладывая, скачать и испытать на своем рабочем проекте наш анализатор!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Maxim Stefanov. PVS-Studio for Java hits the road. Next stop is Elasticsearch

Автор: stefanbuzz

Источник