Сбор, обработка и перемещение данных — ключевые процессы в IT. Но что, если они нарушатся из-за коварных багов в коде? Рассказываем об ошибках, найденных статическим анализатором в проекте Apache NiFi.

Apache NiFi — это бесплатный инструмент с открытым кодом для сбора, обработки и перемещения данных. Сначала он разрабатывался в американском агентстве безопасности, а в 2014 году стал открытым и вошёл в экосистему Apache. Сегодня NiFi часто используют в проектах с большими данными, особенно вместе с Hadoop.

Примечание. Ранее мы уже проверяли и проект Apache Hadoop. Прочитать эту статью можно в нашем блоге.

Главное преимущество NiFi — простой визуальный интерфейс, где данные можно перемещать, как на схеме: перетаскиваешь блоки (процессоры) и соединяешь их стрелками. Это позволяет не только администраторам, но и аналитикам с разработчиками настраивать потоки данных без глубоких знаний в системе. NiFi умеет работать с разными источниками: файлы по SFTP, логи по syslog, базы данных (через JDBC), Kafka, HDFS, Elasticsearch и многие другие.

Сегодня мы посмотрим на то, какие интересные баги смог найти статический анализатор PVS-Studio в этом проекте. Проверялся проект на коммите d924680.

Кстати, в проекте уже используется инструмент контроля качества кода — PMD. Посмотрим, сможет ли PVS-Studio найти ошибки в коде, который уже проверяется :)

Как это делалось?

Проект использует сборочную систему Maven, поэтому для его анализа использовался плагин PVS-Studio для этой сборочной системы.

Чтобы проанализировать исходный код, нужно было добавить несколько строк в файл pom.xml. Для начала необходимо подключить репозиторий плагинов PVS-Studio:

....
<pluginRepositories>
  <pluginRepository>
    <id>pvsstudio-maven-repo</id>
    <url>https://wcdn.pvs-studio.com/java/pvsstudio-maven-repository/</url>
  </pluginRepository>
</pluginRepositories>
....

А затем применить плагин и выставить нужные настройки для анализа:

....
<pluginManagement>
  <plugins>
    <plugin>
      <groupId>com.pvsstudio</groupId>
      <artifactId>pvsstudio-maven-plugin</artifactId>
      <version>7.38.96564</version>
      <configuration>
        <analyzer>
          <outputType>json</outputType>
          <outputFile>PVS-Studio.json</outputFile>
          <analysisMode>GA,OWASP</analysisMode>
        </analyzer>
      </configuration>
    </plugin>
    ....
  </plugins>
</pluginManagement>
....

Здесь в конфигурации анализатора используются следующие настройки:

<outputType> — формат, в котором анализатор сохранит результаты анализа (в нашем случае — формат отчёта json);
<outputFile> — путь, по которому будет расположен отчёт анализатора (в нашем случае — корневая директория проекта);
<analysisMode> — включённые группы диагностик для анализа проекта (в нашем случае — диагностические правила из групп General Analysis и OWASP).

После произведённых изменений в pom.xml и сборки проекта осталось лишь запустить команду:

mvn pvsstudio:pvsAnalyze

Примечание. Подробнее о плагине PVS-Studio для сборочной системы Maven можно прочитать в нашей документации.

Таковы наши условия

Срабатывания анализатора на логически неверные выражения — частые гости статей с проверками open source проектов. Так почему эта статья должна быть исключением? :)

Фрагмент 1

public void communicate() throws IOException {
  final String line = reader.readLine();
  final String[] splits = line.split(" "); 
  if (splits.length < 0) {
    throw new IOException(....);
  ....
}

Предупреждение PVS-Studio: V6007 Expression 'splits.length < 0' is always false. BootstrapCodec.java 45

Всем приготовиться, мы переходим границы времени и пространства! В этом фрагменте кода мы бросаем исключение, если длина полученного массива... меньше нуля.

Вероятно, автор хотел проверить, равна ли длина массива нулю, но эта ситуация тоже никогда не произойдёт... Почему?

Изначально мы получаем строку от пользователя через reader, а после разбиваем с помощью line.split() по пробелам. Дело в том, что даже если в строке нет ни одного пробела, то String.split() вернёт нам массив, в котором единственным элементом будет исходная строка.

Фрагмент 2

protected Map<String, String> getAttributes(
  final TarArchiveInputStream stream
) throws IOException {
  ....
  for (final Entry<Object, Object> entry : props.entrySet()) {
    final Object keyObject = entry.getKey();
    final Object valueObject = entry.getValue();
    if (!(keyObject instanceof String)) {            // <=
      throw new IOException(
        "Flow file attributes object contains key of type "
        + keyObject.getClass().getCanonicalName()
        + " but expected java.lang.String");
    } else if (!(keyObject instanceof String)) {     // <=
      throw new IOException(
        "Flow file attributes object contains value of type "
        + keyObject.getClass().getCanonicalName()
        + " but expected java.lang.String");
  }
  final String key = (String) keyObject;
  final String value = (String) valueObject;
  result.put(key, value);
  }
}

Предупреждение PVS-Studio: V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. FlowFileUnpackagerV1.java 80

Вы когда-нибудь испытывали ощущение безысходности?

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

Что же произошло? Автор написал первый блок этого ветвления, потом скопировал его и вставил в else if, забыв поменять keyObject на valueObject. Причём забыл он его поменять везде, в том числе и в сообщении выдаваемого IOException.

Фрагмент 3

public void setOutputPorts(
  final Set<RemoteProcessGroupPortDescriptor> ports, 
  final boolean pruneUnusedPorts
) {
....
final Iterator<StandardRemoteGroupPort> itr = outputPorts.values().iterator();
int prunedCount = 0;
while (itr.hasNext()) {
  final StandardRemoteGroupPort port = itr.next();
  if (....) {
    port.setTargetExists(false);
    port.setTargetRunning(false);
      // If port has connections, 
      // it will be cleaned up 
      // when connections are removed
      if (port.getConnections().isEmpty()) {
        itr.remove();
        logger.info(
          "Pruning unused Output Port {} from {}", port, this
        );
       }
     }
   }
    if (prunedCount == 0) {  // <=
      logger.debug(
        "There were no Output Ports to prune from {}", 
        this
      );
    } else {....}
  }
  ....
}

Предупреждение PVS-Studio: V6007 Expression 'prunedCount == 0' is always true. StandardRemoteProcessGroup.java 640

Понять, почему анализатор ругается на этот участок кода, помогают комментарии разработчика.

Здесь идёт очистка портов, у которых нет активных соединений. Также есть счётчик prunedCount, который должен считать, сколько портов было удалено.

Анализатор замечает, что условие prunedCount == 0 всегда истинно. И он прав, потому что счётчик нигде не увеличивается: он просто не меняется на протяжении всего кода.

Но, посмотрев на логи и комментарии, видно, что после строки itr.remove() как раз происходит удаление порта. Об этом даже пишется в лог. Значит, именно в этом месте и должно быть увеличение счётчика prunedCount. Скорее всего, его просто забыли добавить.

Исключительные баги

Вы когда-нибудь ловили исключение при обработке исключения?

Фрагмент 4

public void revertReceivedTo(Relationship r, Throwable t) {
  ....
  String errorMessage = Throwables.getMessage(t, null, 950);
  String stackTrace = Throwables.stringStackTrace(t);
  for (FlowFile f : toFail) {
    if (t != null && r != null) {
      ....
    }
  ....
}

Предупреждение PVS-Studio: V6060 The 't' reference was utilized before it was verified against null. ProcessSessionWrap.java 210

Здесь анализатор указывает нам на то, что объект t был использован до того, как прошёл проверку на null. Дабы окончательно убедиться, что этот объект разыменовывается, заглянем в метод Throwables.stringStackTrace():

public static String stringStackTrace(Throwable e) {
  StringWriter sw = new StringWriter(500);
  PrintWriter pw = new PrintWriter(sw);
  e.printStackTrace(pw);
  pw.flush();
  sw.flush();
  return sw.toString();
}

Здесь объект, который мы передаём, действительно разыменовывается. То есть, пытаясь обработать объект исключения, мы можем поймать исключение! Или нет?

На самом деле проверка t != null в этом фрагменте кода просто лишняя. При единственном вызове метода, в котором анализатор выдал предупреждение, в него передаётся константа, созданная через new:

public class ExecuteGroovyScript extends AbstractProcessor {
  ....

  public static final Relationship REL_FAILURE = new Relationship
      .Builder()
      .name("failure")
      .description("FlowFiles that failed to be processed")
      .build();

  ....

  public void onTrigger(
    final ProcessContext context, 
    final ProcessSession _session
  ) throws ProcessException {
  ....
    if (toFailureOnError) {
      session.revertReceivedTo(
        REL_FAILURE, 
        StackTraceUtils.deepSanitize(t)
      );
    } else {....}
    ....
  }

Поэтому получается, что анализатор просто выявил нарушение в логике метода. И это условие действительно сбивает с толку. Не зря же мы столько в нём разбирались :)

Фрагмент 5

public void finishTransferFlowFiles(
  final CommunicationsSession commSession
) throws IOException {
  if (postResult == null) {
    new IllegalStateException(....);
  }
  ....
}

Предупреждение PVS-Studio: V5303 The object was created but it is not being used. The 'throw' keyword could be missing. SiteToSiteRestApiClient.java 919

Трёхочковый!

Мы создали объект исключения со всей нужной информацией. Казалось бы, всё готово, можно бросать, но... ой. Мы его создали и благополучно забыли выдать...

Так трудно смотреть на Exception, который не можешь бросить...
© Неизданный сборник программистских цитат

Классический NullPointerException

Не обойдём стороной ещё одну традицию — ошибки разыменования null.

Фрагмент 6

private boolean replaceNodeStatus(
  final NodeIdentifier nodeId, 
  final NodeConnectionStatus currentStatus, 
  final NodeConnectionStatus newStatus
) {
  if (newStatus == null) {               // <=
    logger.error("....", nodeId, currentStatus, newStatus);
    logger.error("", new NullPointerException());
  }
  
  ....

  if (newStatus.getState() ==            // <=
      NodeConnectionState.REMOVED) {     
    ....
  } else {....}
}

Предупреждение PVS-Studio: V6008 Potential null dereference of 'newStatus'. NodeClusterCoordinator.java 448

Итак, что у нас происходит? Если переменная newStatus равна null, мы записываем это в лог, мол, "всё под контролем, замечено" и спокойно двигаемся далее.

А что нас ждёт "далее"? Всего через несколько строк — разыменование этой самой newStatus вне зависимости от того, равна она нулю или нет. Вероятно, разработчики просто забыли return внутри условия.

Фрагмент 7

public void addFileStatus(
  final FileStatus parent, 
  final FileStatus child
) {
  Set<FileStatus> children = fileStatuses.computeIfAbsent(
    parent.getPath(), k -> new HashSet<>()
  );
  if (child != null) {                           // <=
    children.add(child);
    if (
     child.isDirectory() && 
     !fileStatuses.containsKey(child.getPath())
    ) {
      fileStatuses.put(child.getPath(), new HashSet<>());
    }
  }

  pathToStatus.put(parent.getPath(), parent);
  pathToStatus.put(child.getPath(), child);      // <=
}

Предупреждение PVS-Studio: V6008 Potential null dereference of 'child'. MockFileSystem.java 265

Ещё один поистине хрестоматийный пример. Сначала проверили, что child не равен null, а после взяли и разыменовали его вне условия.

Также, как и в прошлом примере, такие фокусы приведут к NullPointerException в том случае, если child будет равен null.

Все равны?

В Apache NiFi нашёлся целый пласт срабатываний анализатора PVS-Studio на случаи, где что-то пошло не по плану при сравнении объектов.

Фрагмент 8

public void onTrigger(
  final ProcessContext context, 
  final ProcessSession session
) {
  final String listingStrategy = context.getProperty(
    LISTING_STRATEGY
  ).getValue();

  if (BY_TIMESTAMPS.equals(listingStrategy)) {       // <=
    listByTrackingTimestamps(context, session);
  } else if (BY_ENTITIES.equals(listingStrategy)) {  // <=
    listByTrackingEntities(context, session);
  } else if (NO_TRACKING.equals(listingStrategy)) {  // <=
    listNoTracking(context, session);
  } else {
    throw new ProcessException(....);
  }
}

Предупреждения PVS-Studio:

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListGCSBucket.java 437
V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListGCSBucket.java 439
V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListGCSBucket.java 441

В этом фрагменте анализатор ругается на то, что мы сравниваем два несовместимых типа: AllowableValue и String. Без контекста звучит довольно страшно, но на самом деле здесь всё довольно просто: у типа AllowableValue есть метод getValue(), который возвращает строковое значение, подходящее для сравнения в этом контексте:

public class AllowableValue implements DescribedValue {
  private final String value;
  ....
  public String getValue() {
        return this.value;
  }
}

Итого нужно было просто добавить вызов этого метода перед вызовом equals():

....
if (BY_TIMESTAMPS.getValue().equals(listingStrategy)) {
  listByTrackingTimestamps(context, session);
} else if (BY_ENTITIES.getValue().equals(listingStrategy)) {
  listByTrackingEntities(context, session);
} else if (NO_TRACKING.getValue().equals(listingStrategy)) {
  listNoTracking(context, session);
} else {
  throw new ProcessException(....);
}
....

Подобные срабатывания

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. AbstractListProcessor.java 419

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. AbstractListProcessor.java 422

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. AbstractListProcessor.java 425

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. AbstractListProcessor.java 428

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListS3.java 484

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListS3.java 486

V6058 The 'equals' function compares objects of incompatible types: AllowableValue, String. ListS3.java 488

Фрагмент 9

public synchronized void addFlowSnapshot(
  final VersionedExternalFlow versionedExternalFlow
) {
  final String version;
  if (metadata == null) {
    bucketId = DEFAULT_BUCKET_ID;
    flowId = "flow-" + flowIdGenerator.getAndIncrement();
    version = "1";
  } else {
    bucketId = metadata.getBucketIdentifier();
    flowId = metadata.getFlowIdentifier();
    version = metadata.getVersion();
  }
  ....
  final Optional<VersionedExternalFlow> optionalSnapshot = 
    snapshots.stream().filter(
      snapshot -> 
        snapshot.getMetadata().getVersion() == version  // <=
    ).findAny();
  ....
}

Предупреждение PVS-Studio: V6013 Strings 'snapshot.getMetadata().getVersion()' and 'version' are compared by reference. Possibly an equality comparison was intended. InMemoryFlowRegistry.java 165

В этом фрагменте у нас имеются две строки snapshot.getMetadata().getVersion() и version, которые сравниваются с помощью оператора ==. Приводит это к тому, что мы сравниваем не содержимое этих объектов, а их ссылки. То есть это условие будет равно только в том случае, если обе ссылки будут вести на один и тот же объект.

Коварство этой ошибки также в том, что из-за String Pool она может воспроизводиться не всегда.

Чтобы поправить ситуацию и сравнивать строки по содержанию, необходимо вместо оператора == использовать метод equals().

Фрагмент 10

public boolean equals(final Object o) {
  if (this == o) {
    return true;
  }

  if (o == null
      || getClass() != o.getClass()
      || !Arrays.equals(
           o.getClass().getGenericInterfaces(),  // <=
           o.getClass().getGenericInterfaces()   // <=
       )                                        
     ) {
       return false;
     }
  ....
}

Предупреждение PVS-Studio: V6009 Function 'equals' receives an odd argument. The 'o.getClass().getGenericInterfaces()' argument was passed several times. EqualsWrapper.java 126

Ошибка с использованием equals() внутри override'а equals(). К-К-К-КОМБО!

Здесь мы вызываем equals() для сравнения двух массивов, но... злосчастный copy-paste испортил нам все планы. В этом переопределении equals мы сравниваем текущий объект с тем, что передан в метод. Но получается, что сравниваем результаты выполнения getClass().getGenericInterfaces() у одного и того же объекта.

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

public boolean equals(final Object o) {
  if (this == o) {
    return true;
  }

  if (o == null
      || getClass() != o.getClass()
      || !Arrays.equals(
           getGenericInterfaces(),  
           o.getClass().getGenericInterfaces()   
       )                                        
     ) {
       return false;
     }
  ....
}

Ваши аргументы неубедительны

Фрагмент 11

static Properties load(File file, String propertiesType) {
  ....
  try (
    InputStream inputStream = new BufferedInputStream(
      new FileInputStream(file)
    )
  ) {
    rawProperties.load(inputStream);
  } catch (Exception e) {
    throw new RuntimeException(
      String.format(
        "Loading {} Properties [%s] failed", 
        propertiesType, 
        file
      ), e);
  }
  ....
}

Предупреждение PVS-Studio: V6046 Incorrect format. A different number of format items is expected. Arguments not used: 2. PropertiesLoader.java 43

При вызове String.format() мы передали большее количество аргументов, чем можно подставить в строку. Но подождите! В строке же два места для подстановки!

Да, это правда, но в этом фрагменте кода разработчики интересным образом смешали синтаксис строк для String.format() и логгера, из-за чего один из аргументов в этом вызове String.format() всегда остаётся в пролёте.

Подобные срабатывания

V6046 Incorrect format. A different number of format items is expected. Arguments not used: 2. PropertiesLoader.java 34

V6046 Incorrect format. A different number of format items is expected. Arguments not used: 1. PropertiesPersister.java 110

Заключение

Вот мы и подобрались к завершению. Обо всех найденных анализатором ошибках мы сообщим разработчикам проекта, открыв Issue в репозитории на GitHub.

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

Поэтому, если вас заинтересовали примеры ошибок, найденных анализатором PVS-Studio в Apache NiFi, вы можете попробовать найти ошибки уже в своём проекте абсолютно бесплатно, получив лицензию здесь.

Чистого кода вам, друзья!

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valerii Filatov. Bugs wear data. Let's check Apache NiFi.

Баги носят данные. Проверка Apache NiFi