И вновь мы проверяем продукт Apache. На этот раз выбор пал на Solr — платформу поискового сервера с открытым исходным кодом. Благодаря Solr можно эффективно и быстро искать информацию в базах данных и на интернет-ресурсах. При решении такой сложной задачи очень легко допустить разнообразные ошибки, даже несмотря на огромный опыт разработчиков Apache. Именно такие ошибки мы рассмотрим в этой статье.

Кто ты такой, Solr?

Совсем недавно мы проверяли один из известнейших проектов Apache — NetBeans IDE, и в ходе проверки нашли много интересных срабатываний нашего анализатора, на которые, кстати, оперативно обратили внимание разработчики и сделали pull request раньше меня :) В этот раз мы решили посмотреть ещё один их крупный продукт — платформу для полнотекстовых запросов Solr.

Apache Solr, впервые появившийся в 2006 году, предлагает широкий спектр возможностей: от динамической кластеризации и интеграции с базами данных до обработки документов со сложным форматом. Это поисковая система, позволяющая с большой скоростью искать и анализировать информацию на сайте, а также предлагающая функционал по размещению поисковых серверов на оборудовании под управлением Linux. Solr обладает...

Короче говоря, сильно нагружать деталями не буду. Достаточно знать, что это полезная программная платформа, оптимизирующая работу с большими данными. Почему бы нам не посмотреть на её исходный код и не поискать в нем что-то интересное или необычное? Именно этим мы сейчас и займёмся.

Здесь что-то перепутали

От опечаток с перепутанными операторами программист никогда не застрахован. И вот как раз нашлась одна из таких:

public Map<String, List<String>> getIndexFilesPathForSnapshot(
    String collectionName, String snapshotName, String pathPrefix)
    throws SolrServerException, IOException {
  ....

  if (meta != null) {                  // <=
    throw new IllegalArgumentException(
                  "The snapshot named " + snapshotName +
                  " is not found for collection " + collectionName);
  }

  DocCollection collectionState = solrClient.getClusterState() 
                                            .getCollection(collectionName);
  for (Slice s : collectionState.getSlices()) {
    List<CoreSnapshotMetaData> replicaSnaps = 
                     meta.getReplicaSnapshotsForShard(s.getName());  // <=
    ....
  }
  return result;
}

В этом методе используют переменную meta для хранения информации о снимках системы. Также в приведённом фрагменте есть проверка на то, что meta != null. Если да, тогда выбрасываем IllegalArgumentException. Уже этот момент кажется странным. Хорошо, а теперь посмотрим на тело цикла. Тут происходит вызов getReplicaSnapshotsForShard. Учитывая, что meta тут будет всегда null, мы получим NullPointerException. Кажется, что разработчик просто опечатался и перепутал оператор, а исключение должно выбрасываться, если meta равна null.

Анализатор PVS-Studio поработал корректором и сообщил о найденной опечатке:

V6008 Null dereference of 'meta'. SolrSnapshotsTool.java 262

Хочу добавить, что такие ошибки с перепутанными операторами встречаются чаще, чем может показаться. Вот, например, в моей предыдущей статье про NetBeans были сразу две похожие:

private SourcesModel getModel() {
  SourcesModel tm = model.get();
  if (tm == null) {
    tm.sourcePath.removePropertyChangeListener (this);
    tm.debugger.getSmartSteppingFilter ().
    removePropertyChangeListener (this);
  }
  return tm;
}

V6008 Null dereference of 'tm'. SourcesModel.java 713

public void propertyChange(PropertyChangeEvent evt) {
  ....
  synchronized (this) {
    artifacts = null;
    if (listeners == null && listeners.isEmpty()) {
      return;
    }
    ....
  }
}

V6008 Null dereference of 'listeners'. MavenArtifactsImplementation.java 613

Возможно, что мы нашли новый паттерн опечаток. Будем наблюдать дальше.

Переходим к следующему фрагменту. В этом классе разработчик перепутал, что нужно возвращать в одном из get-методов.

public class FunctionQParser extends QParser {
  ....
  boolean parseMultipleSources = true;
  boolean parseToEnd = true;
  ....
  public void setParseMultipleSources(boolean parseMultipleSources) {
    this.parseMultipleSources = parseMultipleSources;
  }
 
  /** parse multiple comma separated value sources */
  public boolean getParseMultipleSources() {
    return parseMultipleSources;
  }
  
  public void setParseToEnd(boolean parseToEnd) {
    this.parseToEnd = parseToEnd;
  }
 
  /** throw exception if there is extra 
      stuff at the end of the parsed valuesource(s). */
  public boolean getParseToEnd() {
    return parseMultipleSources;
  }
  ....
}

Данный класс предназначен для парсинга определённой записанной математической функции. Для изменения поведения парсера есть два свойства. parseMultipleSources отвечает за то, чтобы анализировать все источники числовых значений, parseToEnd проверяет, нужно ли парсить до конца строку с функцией.

Теперь посмотрим на методы get и set для данных полей. В getParseToEnd возвращается поле parseMultipleSources. Здесь перепутали, какое именно поле нужно возвращать.

Поиск несоответствующих возвращаемых полей — не проблема для анализатора:

V6091 Suspicious getter implementation. The 'parseToEnd' field should probably be returned instead. FunctionQParser.java 87, FunctionQParser.java 57

В следующем фрагменте опечатка может привести к NullPointerException.

public void stringField(FieldInfo fieldInfo, String value) throws IOException {
  // trim the value if needed
  int len = value != null ? 
             UnicodeUtil.calcUTF16toUTF8Length(value, 0, value.length()) : 0;
  if (value.length() > maxLength) {               // <=    
    value = value.substring(0, maxLength);
  }
  countItem(fieldInfo.name, value, len);
}

Посмотрим внимательнее: сначала value сравнивают с null, а затем в следующей строке у value вызывают метод length(). Но ведь переменная может быть равна null! Скорее всего разработчику нужно было использовать переменную len вместо вызова length().

Такую опечатку удалось найти благодаря сообщению анализатора:

V6008 Potential null dereference of 'value'. IndexSizeEstimator.java 735, IndexSizeEstimator.java 736

Давайте рассмотрим ещё один фрагмент с опечаткой:

public Object doWork(Object value) throws IOException {
  ....
  List<?> list = (List<?>) value;
  // Validate all of same type and are comparable
  Object checkingObject = list.get(0);
  for (int idx = 0; idx < list.size(); ++idx) {
    Object item = list.get(0);                         // <=
    
    if (null == item) {
      throw new IOException(....);
    } else if (!(item instanceof Comparable<?>)) {
      throw new IOException(....);
    } else if (!item.getClass()
                    .getCanonicalName()
                    .equals(checkingObject.getClass()
                                          .getCanonicalName())) {
       throw new IOException(....);
    }
  }
  ....
}

Здесь стоит обратить внимание на цикл for. Программист как обычно объявляет сам цикл, переменную счётчика idx, получает количество элементов в списке. Но есть проблема — каждую итерацию цикла берут только элемент по индексу 0: list.get(0).

Такая ошибка была найдена с помощью PVS-Studio:

V6016 Suspicious access to element of 'list' object by a constant index inside a loop. AscEvaluator.java 56

В следующем примере представлены два метода с разными названиями. Но делают они одно и тоже.

private static List<Feature> makeFeatures(int[] featureIds) {
  final List<Feature> features = new ArrayList<>();
  for (final int i : featureIds) {
    Map<String, Object> params = new HashMap<String, Object>();
    params.put("value", i);
    final Feature f = Feature.getInstance(solrResourceLoader, 
                       ValueFeature.class.getName(), "f" + i, params);
    f.setIndex(i);
    features.add(f);
  }
  return features;
}

private static List<Feature> makeFilterFeatures(int[] featureIds) {
  final List<Feature> features = new ArrayList<>();
  for (final int i : featureIds) {
    Map<String, Object> params = new HashMap<String, Object>();
    params.put("value", i);
    final Feature f = Feature.getInstance(solrResourceLoader, 
                       ValueFeature.class.getName(), "f" + i, params);
    f.setIndex(i);
    features.add(f);
  }
  return features;
}

Первый выполняет создание списка объектов класса Feature. Второй, исходя из названия, должен возвращать другой тип или же выполнять фильтрацию этих Feature. Eсли бы тип FilterFeature в исходном коде присутствовал, мы могли бы предположить, что разработчики просто опечатались. Однако такого типа нет. Возможно, это просто копипаста, и разработчики забыли про этот метод после копирования.

В любом случае, это место выглядят очень подозрительно, о чем также сигнализирует анализатор:

V6032 It is odd that the body of method 'makeFeatures' is fully equivalent to the body of another method 'makeFilterFeatures'. TestLTRScoringQuery.java 66, TestLTRScoringQuery.java 79

Проверку забыли – ошибку добыли

Если вам кажется, что проверка на null бывает лишней — вам только кажется. В представленном ниже коде "лишняя" проверка могла спасти от NullPointerException.

public static Map<String, Object> postProcessCollectionJSON(
                                            Map<String, Object> collection) {
  final Map<String, Map<String, Object>> shards = collection != null   // <=
         ? (Map<String, Map<String, Object>>)
           collection.getOrDefault("shards", Collections.emptyMap())
         : Collections.emptyMap();
  final List<Health> healthStates = new ArrayList<>(shards.size());
  shards.forEach(
  ....
  );
  collection.put("health", Health.combine(healthStates).toString());   // <=
  return collection;
}

В начале метода проверяют, не пустая ли ссылка collection. Если это так, тогда из collection получают shards. Самое интересное, что в конце в collection добавляют healthStates независимо от того, является ссылка на collection пустой или нет.

Предупреждение анализатора на этот фрагмент кода:

V6008 Potential null dereference of 'collection'. ClusterStatus.java 303, ClusterStatus.java 335

А в следующем примере сделали явную ошибку в конструкторе класса для поддержки параллельного распределения работы потоков.

public class ParallelStream extends CloudSolrStream 
                            implements Expressible {
  ....
  private transient StreamFactory streamFactory;

  public ParallelStream(String zkHost, 
                        String collection, 
                        String expressionString, 
                        int workers, 
                        StreamComparator comp
) throws IOException {
    TupleStream tStream = this.streamFactory
                              .constructStream(expressionString);  // <=
    init(zkHost, collection, tStream, workers, comp);
  }  
  ....
}

Ошибка находится в первой строке тела конструктора. Здесь происходит обращение к полю streamFactory, но при этом нет самой инициализации данного поля. Возможно, разработчики забыли добавить часть логики в конструкторе или эта строка написана случайно.

Сообщение PVS-Studio:

V6090 Field 'streamFactory' is being used before it was initialized. ParallelStream.java 61

А вот в этом методе добавить проверку не забыли, но, кажется, поместили её не в том месте.

private void createNewCollection(final String collection)
 throws InterruptedException {
  ....
  pending.add(completionService.submit(call));
  while (pending != null && pending.size() > 0) {
    Future<Object> future = completionService.take();
    if (future == null) return;
    pending.remove(future);
  }
}

Давайте посмотрим на взаимодействие с полем pending: сначала вызвали add, а потом решили сделать цикл, в котором постепенно удаляют из него элементы. Но самое интересное то, что в условии цикла проверили, что pending — не null. Выглядит очень подозрительно, учитывая, что в теле цикла обнуления переменной нет. Кажется, стоило добавить проверку и перед вызовом метода add.

Предупреждение анализатора:

V6060 The 'pending' reference was utilized before it was verified against null. AbstractBasicDistributedZkTestBase.java 1664, AbstractBasicDistributedZkTestBase.java 1665

Потерянное исключение

Как и во многих современных языках, в Java есть возможность обработки исключений. Главное — их не потерять, как это случилось здесь.

private void doSplitShardWithRule(SolrIndexSplitter.SplitMethod splitMethod) 
 throws Exception {
  ....
  try {
    ZkStateReader.from(cloudClient)
                 .waitForState(collectionName, 30, 
                             TimeUnit.SECONDS,
                             SolrCloudTestCase.activeClusterShape(1, 2));
  } catch (TimeoutException e) {
    new RuntimeException("Timeout waiting for " +        // <=
                         "1shards and 2 replicas.", e);     
  }
  ....
}

Ошибка кроется в блоке catch — внутри создали объект RuntimeException, даже добавили связь с текущим перехваченным TimeoutException и дополнили сообщением. А вот написать ключевое слово throw забыли. И в итоге это исключение никогда не выбросится.

Бюро находок в лице анализатора удалось найти потерянное исключение и проинформировать об этом:

V6006 The object was created but it is not being used. The 'throw' keyword could be missing. ShardSplitTest.java 773

Как ошибки арифметики мешают тестированию

Благодаря тестированию программы содержат меньше ошибок? Но тесты тоже пишут люди, и ошибаться они не перестают. Так произошло и в следующем примере:

Public class SpellCheckCollatorTest extends SolrTestCaseJ4 {
  private static final int NUM_DOCS_WITH_TERM_EVERYOTHER = 8;
  private static final int NUM_DOCS = 17;
  ....
  @Test
  public void testEstimatedHitCounts() {
    ....
    for (int val = 5; val <= 20; val++) {
      String hitsXPath = xpathPrefix + "long[@name='hits']"; 

      if (val <= NUM_DOCS_WITH_TERM_EVERYOTHER) {
        int max = NUM_DOCS;
        int min = (/* min collected */ val) / 
                  (/* max docs possibly scanned */ NUM_DOCS);
        hitsXPath += "[" + min + " <= . and . <= " + max + "]";
      } 
    ....
    }
  }
  ....
}

Здесь в hitsXPath записывается строка, содержащая переменную min, которая, в свою очередь, является результатом деления val на NUM_DOCS. Если посмотреть внимательнее, то можно увидеть, что максимальное и минимальное значение val в данном фрагменте 8 и 5. При этом значение NUM_DOCS всегда равно 17. Во всех случаях при целочисленном делении min равен нулю. Скорее всего, здесь забыли добавить приведение аргументов деления к вещественным числам и изменить тип переменной min.

Такую ошибку удалось найти с помощью совсем новой диагностики в анализаторе PVS-Studio:

V6113 The '(val) / (NUM_DOCS)' expression evaluates to 0 because the absolute value of the left operand 'val' is less than the value of the right operand 'NUM_DOCS'. SpellCheckCollatorTest.java 683

Опасность проверок объектов по ссылке

В представленном дальше классе описан метод сравнения equalsTo.

private static class RandomQuery extends Query {
  private final long seed;
  private float density;
  private final List<BytesRef> docValues;
  ....
  private boolean equalsTo(RandomQuery other) {
    return seed == other.seed && 
           docValues == other.docValues && 
           density == other.density;
  }
}

Сравнения полей seed и density практически не вызывают вопросов (за исключением, пожалуй, поля density, которое является вещественным числом), ведь рассматриваются непосредственно записанные в них значения. А вот сравнение через '==' списка docValues выглядит очень сомнительно, ведь это поле имеет ссылочный тип, и такая проверка учтёт только адрес, а не внутреннее состояние объекта.

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

Сообщение анализатора PVS-Studio:

V6013 Objects 'docValues' and 'other.docValues' are compared by reference. Possibly an equality comparison was intended. TestFieldCacheSortRandom.java 341

Подозрительное отсутствие синхронизации

А вот в следующем фрагменте вы ошибку не найдёте, но потенциально она всё равно есть. Как так? Предлагаю ознакомиться с этим кодом, а после узнать причину.

public abstract class CachingDirectoryFactory extends DirectoryFactory {
  ....
  private static final Logger log = LoggerFactory.getLogger(....);
  protected Map<String, CacheValue> byPathCache = new HashMap<>();
  protected IdentityHashMap<Directory, CacheValue> byDirectoryCache = 
                                                  new IdentityHashMap<>();
  ....

  private void removeFromCache(CacheValue v) {
    log.debug("Removing from cache: {}", v);
    byDirectoryCache.remove(v.directory);
    byPathCache.remove(v.path);
  }
}

Сразу понять, что тут не так, не удастся, пока не посмотрим все использования переменной byDirectoryCache. Во всех остальных методах взаимодействие происходит в блоке synchronized. Но в методе removeFromCache программист удаляет элементы коллекции вне блоков synchronized.

Такое подозрительное место отметил анализатор:

V6102 Inconsistent synchronization of the 'byDirectoryCache' field. Consider synchronizing the field on all usages. CachingDirectoryFactory.java 92, CachingDirectoryFactory.java 228

На этом этапе можно было бы сказать, что здесь присутствует ошибка и может возникнуть состояние гонки. Но как оказалось, все вызовы removeFromCache также заключены в блоки synchronized. То есть, по большому счёту, это срабатывание является ложным и его можно было бы подавить.

Тем не менее, этот код всё ещё можно улучшить, ведь потенциальная проблема здесь есть. Например, при необходимости использовать этот метод снова, его можно банально забыть заключить в блок синхронизации. Хотя в остальных методах есть дополнительные проверки на то, что объект существует в коллекции byDirectoryCache, несинхронизированный вызов может удалить элемент уже после проверки. Из-за этого в другом потоке с уже несуществующим элементом коллекции будут выполняться лишние действия, которые могут привести к ошибкам в логике работы программы.

Чтобы обезопасить себя от такой ситуации, достаточно добавить ключевое слово synchronized к методу removeFromCache. Таким образом, хоть реальной ошибки здесь не было найдено, статический анализатор всё же подталкивает писать более чистый код.

Кстати, совсем недавно у нас вышла статья о том, какие подводные камни скрываются при использовании синхронизации.

Классы с одинаковыми именами – возможно ли такое?

А в этом фрагменте не учли, что классы можно переименовывать или объявлять с одним именем в разных пакетах.

private static String getFieldFlags(IndexableField f) {
  IndexOptions opts = (f == null) ? null : f.fieldType().indexOptions();

  StringBuilder flags = new StringBuilder();
  ....
  flags.append((f != null && f.getClass()
                              .getSimpleName()
                              .equals("LazyField"))  // <=
                                   ? FieldFlag.LAZY.getAbbreviation(): '-');
  ....
  return flags.toString();
}

Здесь выполняется явно лишняя операция, которая, к тому же, может привести к ошибке. У переменной f вызывают метод getClass, который вернёт тип объекта, а потом получают и проверяют название без уточнения пакетов. Как таковой ошибки сейчас тут нет. Но она может произойти по двум причинам.

Первая связана с тем, что в разных пакетах могут быть классы с одинаковым названием. В таком случае будет непонятно, какой именно LazyField требуется, и в дальнейшем программа будет выполняться не так, как задумывалось.

Вторая связана с изменением названия класса. При изменении названия этот код вообще не будет выполняться так, как задумывалось. А искать все подобные строки в огромной кодовой базе очень тяжело. Даже прибегая к поиску, это можно просто забыть сделать.

Было бы гораздо безопаснее использовать оператор instanceof:

flags.append(f instanceof LazyDocument.LazyField
             ? FieldFlag.LAZY.getAbbreviation(): '-');

В таком случае проверка на null не потребуется, код заметно сократится. Вероятность получить ошибку также уменьшится, если есть классы с одинаковым именем в разных пакетах, ну или же название этого класса изменится.

Потенциальную ошибку удалось обнаружить и проинформировать об этом сообщением:

V6054 Classes should not be compared by their name. LukeRequestHandler.java 247

А как же документация?

И в качестве заключительного фрагмента рассмотрим следующий код:

@Override
public UpdateCommand clone() {
  try {
    return (UpdateCommand) super.clone();
  } catch (CloneNotSupportedException e) {
    return null;                         // <=
  }
}

Давайте разбираться, что же тут не так, ведь на первый взгляд всё хорошо. Анализатор ругается на то, что в методе clone возвращать null является плохим решением:

V6073 It is not recommended to return null from 'clone' method. UpdateCommand.java 97

Почему не рекомендуется возвращать null из clone? Тут стоит обратиться к документации Java:

Returns:
a clone of this instance.
Throws:
CloneNotSupportedException - if the object's class does not support the Cloneable interface. Subclasses that override the clone method can also throw this exception to indicate that an instance cannot be cloned.

Исключение в данном случае сигнализирует о том, что объект нельзя клонировать. Метод должен возвращать копию текущего объекта и ничего больше. Но почему же анализатор говорит, что возвращать null из clone не рекомендуется? Всё дело в дальнейшем использовании кода — если постоянно отходить от рекомендаций из документации, отловить нестандартные ситуации окажется затруднительным.

Давайте представим ситуацию: мы захотели использовать класс UpdateCommand, а исходный код недоступен и декомпилировать мы не можем. Ну, или нам просто лень. Всё, что остаётся в таком случае, — использовать уже собранную библиотеку с этим классом и ориентироваться только на интерфейс. В нашей программе потребовалось использовать метод clone, и мы написали такой код:

try {
  UpdateCommand localCopy = field.clone(); 
  System.out.println(localCopy.toString();
} catch (CloneNotSupportedException e) {
  System.out.println("Could not clone the field"); 
}

В этом коде мы пытаемся перехватить CloneNotSupportedException, но у нас не выйдет, так как исключением будет NullPointerException, из-за чего программа упадёт при вызове localCopy.ToString(), что окажется полной неожиданностью для разработчика. Поэтому отход от официальных рекомендаций может стать неприятной проблемой, из-за чего лучше их соблюдать всегда 😊

Заключение

Давайте на этом остановимся и ещё раз оценим те ошибки, которые удалось найти. Большая часть из них — это результат невнимательности, но есть и такие, которые требуют дополнительных размышлений. Например, сравнение названий классов без учета пакетов или возврат null вместо проброса исключения в методе clone.

Без специальных инструментов разработки в виде статических анализаторов подобные баги найти достаточно трудно, особенно в таких крупных проектах, как Apache Solr. А если вам хочется поискать подобные неочевидные ошибки в своём проекте, попробовать наш статический анализатор можно тут.

Кстати, мы проверяли не только Solr, но и другие продукты Apache:

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Daniil Liakhov. Search query for bugs in Apache Solr.

Поисковый запрос на баги в Apache Solr