Анализ исходного кода RPC фреймворка Apache Dubbo статическим анализатором PVS-Studio

Apache Dubbo — один из самых популярных Java проектов на GitHub. И это неудивительно. Он был создан 8 лет назад и широко применяется как высокопроизводительная RPC среда. Конечно, большинство ошибок в его коде давно исправлены и качество кода поддерживается на высоком уровне. Однако, нет причины отказаться от проверки такого интересного проекта с помощью статического анализатора кода PVS-Studio. Давайте посмотрим, что же нам удалось найти.

О PVS-Studio

Статический анализатор кода PVS-Studio существует более 10 лет на IT-рынке и представляет собой многофункциональное и легко внедряемое программное решение. На данный момент анализатор поддерживает языки C, C++, C#, Java и работает на платформах Windows, Linux и macOS.

PVS-Studio является платным B2B-решением и используется большим количеством команд в различных компаниях. Если хотите оценить возможности анализатора, то скачать дистрибутив и запросить триальный ключ можно здесь.

Также есть варианты использовать PVS-Studio бесплатно в open source проектах или если вы студент.

Apache Dubbo: что это и с чем это едят?

В настоящее время практически все большие программные системы являются распределенными. Если в распределенной системе существует интерактивная связь между удаленными компонентами с небольшим временем ответов и относительно малым количеством передаваемых данных, то это веский повод использовать среду RPC (remote procedure call).

Apache Dubbo — это высокопроизводительная среда RPC (remote procedure call) с открытым исходным кодом на основе Java. Как и во многих системах RPC, в основе dubbo лежит идея создания службы определения методов, которые можно вызывать удаленно с их параметрами и типами возвращаемых данных. На стороне сервера реализуется интерфейс и запускается сервер dubbo для обработки клиентских вызовов. На стороне клиента имеется заглушка, которая предоставляет те же методы, что и сервер. Dubbo предлагает три ключевые функции, которые включают интерфейсный удаленный вызов, отказоустойчивость и балансировку нагрузки, а также автоматическую регистрацию и обнаружение услуг.

Об анализе

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

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

Результаты анализа: 73 предупреждения уровня достоверности High и Medium (46 и 27, соответственно) были выданы для 4000+ файлов, что является хорошим показателем качества кода.

Не все предупреждения являются ошибками. Это нормальная ситуация и перед регулярным использованием анализатора требуется его настройка. После чего можно ожидать достаточно низкий процент ложных срабатываний (пример).

Среди предупреждений не рассматривались 9 предупреждений (7 High и 2 Medium), приходящихся на файлы с тестами.

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

Знаковый byte в Java

V6007 Expression 'endKey[i] < 0xff' is always true. OptionUtil.java(32)

public static final ByteSequence prefixEndOf(ByteSequence prefix) {
  byte[] endKey = prefix.getBytes().clone();
  for (int i = endKey.length - 1; i >= 0; i--) {
    if (endKey[i] < 0xff) {                                           // <=
      endKey[i] = (byte) (endKey[i] + 1);
      return ByteSequence.from(Arrays.copyOf(endKey, i + 1));
    }
  }
  return ByteSequence.from(NO_PREFIX_END);
}

Значение типа byte (значение от -128 до 127) сравнивается с значением 0xff (255). В этом условии не учтено что тип byte в Java знаковый, поэтому условие всегда будет выполнено, а, значит, оно не имеет смысла.

Возврат одинаковых значений

V6007 Expression 'isPreferIPV6Address()' is always false. NetUtils.java(236)

private static Optional<InetAddress> toValidAddress(InetAddress address) {
  if (address instanceof Inet6Address) {
    Inet6Address v6Address = (Inet6Address) address;
    if (isPreferIPV6Address()) {                               // <= 
      return Optional.ofNullable(normalizeV6Address(v6Address));
    }
  }
  if (isValidV4Address(address)) {
    return Optional.of(address);
  }
  return Optional.empty();
}

Метод isPreferIPV6Address.

static boolean isPreferIPV6Address() {
  boolean preferIpv6 = Boolean.getBoolean("java.net.preferIPv6Addresses");
  if (!preferIpv6) {
    return false;                                                     // <=
  }
  return false;                                                         // <=
}

Метод isPreferIPV6Address возвращает false в обоих случаях, скорее всего, что один из случаев должен был вернуть true по задумке программиста, иначе метод не имеет смысла.

Бессмысленные проверки

V6007 Expression '!mask[i].equals(ipAddress[i])' is always true. NetUtils.java(476)

public static boolean matchIpRange(....) throws UnknownHostException {
  ....
  for (int i = 0; i < mask.length; i++) {
    if ("*".equals(mask[i]) || mask[i].equals(ipAddress[i])) {
      continue;
    } else if (mask[i].contains("-")) {
       ....
    } else if (....) {
      continue;
    } else if (!mask[i].equals(ipAddress[i])) {                 // <=
      return false;
    }
  }
  return true;
}

В первом из условий if-else-if происходит проверка "*".equals(mask[i]) || mask[i].equals(ipAddress[i]).Если это условие не выполняется, то код переходит к следующей проверке в if-else-if, а нам становится известно, что mask[i] и ipAddress[i] не эквивалентны. Но в одной из следующих проверок в if-else-if как раз проверяется, что mask[i] и ipAddress[i] не эквивалентны. Так как mask[i] и ipAddress[i] в коде метода не присваивается никаких значений, то вторая проверка не имеет смысла.

V6007 Expression 'message.length > 0' is always true. DeprecatedTelnetCodec.java(302)

V6007 Expression 'message != null' is always true. DeprecatedTelnetCodec.java(302)

protected Object decode(.... , byte[] message) throws IOException {
  ....
  if (message == null || message.length == 0) {
    return NEED_MORE_INPUT;
  }
  ....
  // Здесь переменная message не изменяется!
  ....
  if (....) {
    String value = history.get(index);
    if (value != null) {
      byte[] b1 = value.getBytes();
      if (message != null && message.length > 0) {                         // <=
        byte[] b2 = new byte[b1.length + message.length];
        System.arraycopy(b1, 0, b2, 0, b1.length);
        System.arraycopy(message, 0, b2, b1.length, message.length);
        message = b2;
      } else {
        message = b1;
      }
    }
  }
  ....
}

Проверка message != null && message.length > 0 в строке 302 не имеет смысла. До проверки в строке 302 производится проверка:

if (message == null || message.length == 0) {
  return NEED_MORE_INPUT;
}

Если условие проверки не выполнится, то мы будем знать, что message не равно null и ее длинна не равна 0. Из этой информации следует вывод, что ее длинна больше нуля (т.к. length строки не может быть отрицательным числом). Локальной переменной message до строки 302 не присваивается никаких значений, а, значит, в строке 302 используется значение переменной message, что и в коде выше. Из всего этого можно сделать вывод, что выражение message != null && message.length > 0 всегда будет равно true, а значит код в блоке else никогда не будет выполнен.

Установка значения неинициализированного ссылочного поля

V6007 Expression '!shouldExport()' is always false. ServiceConfig.java(371)

public synchronized void export() {
  checkAndUpdateSubConfigs();

  if (!shouldExport()) {                                 // <=
    return;
  }

  if (shouldDelay()) {
    ....
  } else {
    doExport();
}

Метод shouldExport класса ServiceConfig вызывает метод getExport определенный в этом же классе.

private boolean shouldExport() {
  Boolean export = getExport();
  // default value is true
  return export == null ? true : export; 
}
....
@Override
public Boolean getExport() {
  return (export == null && provider != null) ? provider.getExport() : export;
}

Метод getExport вызывает метод getExport абстрактного класса AbstractServiceConfig, который возвращает значение поля export, тип которого Boolean. Также есть метод setExport для установки значения поля.

protected Boolean export;
....
public Boolean getExport() {
  return export;
}
....
public void setExport(Boolean export) {
  this.export = export;
}

Устанавливается поле export в коде только методом setExport. Метод setExport вызывается только в методе build абстрактного класса AbstractServiceBuilder (расширяющего AbstractServiceConfig) и только если поле export не равно null.

@Override
public void build(T instance) {
....
  if (export != null) {
    instance.setExport(export);
  }
....
}

Из-за того, что все ссылочные поля по умолчанию инициализируются значением null и того, что полю export нигде не было присвоено какое-либо значение в коде, метод setExport никогда не будет вызван.

В результате метод getExport класса ServiceConfig расширяющего класс AbstractServiceConfig будет всегда возвращать null. Возвращенное значение используется в методе shouldExport класса ServiceConfig, поэтому метод shouldExport всегда будет возвращать true. Из-за возврата true значение выражения !shouldExport() всегда будет ложно. Получается, что никогда не произойдет возврат из метода export класса ServiceConfig до выполнения кода:

if (shouldDelay()) {
  DELAY_EXPORT_EXECUTOR.schedule(this::doExport, getDelay(), ....);
} else {
  doExport();
}

Неотрицательное значение параметра

V6009 The 'substring' function could receive the '-1' value while non-negative value is expected. Inspect argument: 2. AbstractEtcdClient.java(169)

protected void createParentIfAbsent(String fixedPath) {
  int i = fixedPath.lastIndexOf('/');
  if (i > 0) {
    String parentPath = fixedPath.substring(0, i);
    if (categories.stream().anyMatch(c -> fixedPath.endsWith(c))) {
      if (!checkExists(parentPath)) {
          this.doCreatePersistent(parentPath);
      }
      } else if (categories.stream().anyMatch(c -> parentPath.endsWith(c))) {
        String grandfather = parentPath
        .substring(0, parentPath.lastIndexOf('/'));            // <=
        if (!checkExists(grandfather)) {
            this.doCreatePersistent(grandfather);
        }
      }
  }
}

Результат функции lastIndexOf передается вторым параметром в функцию substring, второй параметр которой не должен быть отрицательным числом, хотя lastIndexOf может вернуть -1, если не найдет искомое значение в строке. Если второй параметр метода substring меньше первого (-1 < 0), то произойдет исключение StringIndexOutOfBoundsException. Для исправления ошибки нужно выполнить проверку результата, возвращаемого функцией lastIndexOf, и если он корректный (как минимум не отрицательный), то передать его в функцию substring.

Неиспользуемый счетчик цикла

V6016 Suspicious access to element of 'types' object by a constant index inside a loop. RpcUtils.java(153)

public static Class<?>[] getParameterTypes(Invocation invocation) {
  if ($INVOKE.equals(invocation.getMethodName())
            && invocation.getArguments() != null
            && invocation.getArguments().length > 1
            && invocation.getArguments()[1] instanceof String[]) {
    String[] types = (String[]) invocation.getArguments()[1];
    if (types == null) {
      return new Class<?>[0];
    }
    Class<?>[] parameterTypes = new Class<?>[types.length];
    for (int i = 0; i < types.length; i++) {
      parameterTypes[i] = ReflectUtils.forName(types[0]);   // <= 
    }
    return parameterTypes;
  }
  return invocation.getParameterTypes();
}

В цикле for используется константный индекс 0 для обращения к элементу массива types. Возможно, подразумевалось использование переменной i как индекса для обращения к элементам массива, но не доглядели, как говорится.

Бессмысленный do-while

V6019 Unreachable code detected. It is possible that an error is present. GrizzlyCodecAdapter.java(136)

@Override
public NextAction handleRead(FilterChainContext context) throws IOException {
  .... 
  do {
    savedReadIndex = frame.readerIndex();
    try {
      msg = codec.decode(channel, frame);
    } catch (Exception e) {
      previousData = ChannelBuffers.EMPTY_BUFFER;
      throw new IOException(e.getMessage(), e);
    }
    if (msg == Codec2.DecodeResult.NEED_MORE_INPUT) {
      frame.readerIndex(savedReadIndex);
        return context.getStopAction();
    } else {
      if (savedReadIndex == frame.readerIndex()) {
        previousData = ChannelBuffers.EMPTY_BUFFER;
        throw new IOException("Decode without read data.");
      }
      if (msg != null) {
        context.setMessage(msg);
        return context.getInvokeAction();
      } else {
        return context.getInvokeAction();
      }
    }
  } while (frame.readable());                                    // <=
  ....
}

Выражение в условии цикла do — while (frame.readable()) является недостижимым кодом, так как при первой итерации цикла всегда происходит выход из метода. В теле цикла выполняется несколько проверок переменной msg при помощи if-else, причем как в if, так и в else всегда происходит возврат значения из метода либо выброс исключения. Именно из-за этого тело цикла выполнится только один раз, в результате использование цикла do-while не имеет смысла.

Копипаст в switch

V6067 Two or more case-branches perform the same actions. JVMUtil.java(67), JVMUtil.java(71)

private static String getThreadDumpString(ThreadInfo threadInfo) {
  ....
  if (i == 0 && threadInfo.getLockInfo() != null) {
    Thread.State ts = threadInfo.getThreadState();
    switch (ts) {
      case BLOCKED:
        sb.append("t-  blocked on " + threadInfo.getLockInfo());
        sb.append('n');
        break;
      case WAITING:                                               // <=
        sb.append("t-  waiting on " + threadInfo.getLockInfo()); // <=
        sb.append('n');                                          // <=
        break;                                                    // <=
      case TIMED_WAITING:                                         // <=
        sb.append("t-  waiting on " + threadInfo.getLockInfo()); // <=
        sb.append('n');                                          // <=
        break;                                                    // <=
      default:
    }
  }
  ....
}

Код в switch для значений WAITING и TIMED_WAITING содержит копипастнутый код. Если действительно должны выполняться одни и те же действия, то код можно упростить, удалив содержимое в блоке case для WAITING. В результате для WAITING и TIMED_WAITING будет выполнятся один и тот же код, записанный в единственном экземпляре.

Заключение

Кто интересовался использованием RPC в Java, тот наверняка слышал о Apache Dubbo. Это популярный open source проект с долгой историей и кодом, написанным многими разработчиками. Код этого проекта отличного качества, но статическому анализатору PVS-Studio удалось найти некоторое количество ошибок. Из этого можно сделать вывод, что статический анализ очень важен при разработке средних и больших проектов, как бы не был идеален ваш код.

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

Спасибо разработчикам Apache Dubbo за столь замечательный инструмент. Надеюсь, эта статья поможет вам в улучшении кода. В статье описаны не все подозрительные участки кода, так что разработчикам лучше проверить проект самостоятельно и оценить полученные результаты.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Valery Komarov. Analysis of the Apache Dubbo RPC Framework by the PVS-Studio Static Code Analyzer.

Автор: Valery Komarov

Источник