# Анализ исходного кода RPC фреймворка Apache Dubbo статическим анализатором PVS\-Studio Apache Dubbo — один из самых популярных Java проектов на GitHub\. И это неудивительно\. Он был создан 8 лет назад и широко применяется как высокопроизводительная RPC среда\. Конечно, большинство ошибок в его коде давно исправлены и качество кода поддерживается на высоком уровне\. Однако, нет причины отказаться от проверки такого интересного проекта с помощью статического анализатора кода PVS\-Studio\. Давайте посмотрим, что же нам удалось найти\. ![0663_ApacheDubbo_ru/image1.png](https://import.viva64.com/docx/blog/0663_ApacheDubbo_ru/image1.png) ## О PVS\-Studio Статический анализатор кода [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/) существует более 10 лет на IT\-рынке и представляет собой многофункциональное и легко внедряемое программное решение\. На данный момент анализатор поддерживает языки C, C\+\+, C\#, Java и работает на платформах Windows, Linux и macOS\. PVS\-Studio является платным B2B\-решением и используется большим количеством команд в различных компаниях\. Если хотите оценить возможности анализатора, то скачать дистрибутив и запросить триальный ключ можно [здесь](https://pvs-studio.ru/ru/pvs-studio/download/)\. Также есть [варианты](https://pvs-studio.ru/ru/blog/posts/0614/) использовать PVS\-Studio бесплатно в open source проектах или если вы студент\. ## Apache Dubbo: что это и с чем это едят? В настоящее время практически все большие программные системы являются [распределёнными](https://ru.wikipedia.org/wiki/%D0%A0%D0%B0%D1%81%D0%BF%D1%80%D0%B5%D0%B4%D0%B5%D0%BB%D1%91%D0%BD%D0%BD%D0%B0%D1%8F_%D1%81%D0%B8%D1%81%D1%82%D0%B5%D0%BC%D0%B0)\. Если в распределенной системе существует интерактивная связь между удаленными компонентами с небольшим временем ответов и относительно малым количеством передаваемых данных, то это веский повод использовать среду [RPC](https://ru.wikipedia.org/wiki/Удалённый_вызов_процедур) \(remote procedure call\)\. Apache Dubbo — это высокопроизводительная среда [RPC](https://ru.wikipedia.org/wiki/%D0%A3%D0%B4%D0%B0%D0%BB%D1%91%D0%BD%D0%BD%D1%8B%D0%B9_%D0%B2%D1%8B%D0%B7%D0%BE%D0%B2_%D0%BF%D1%80%D0%BE%D1%86%D0%B5%D0%B4%D1%83%D1%80) \(remote procedure call\) с открытым исходным кодом на основе Java\. Как и во многих системах RPC, в основе dubbo лежит идея создания службы определения методов, которые можно вызывать удаленно с их параметрами и типами возвращаемых данных\. На стороне сервера реализуется интерфейс и запускается сервер dubbo для обработки клиентских вызовов\. На стороне клиента имеется заглушка, которая предоставляет те же методы, что и сервер\. Dubbo предлагает три ключевые функции, которые включают интерфейсный удаленный вызов, отказоустойчивость и балансировку нагрузки, а также автоматическую регистрацию и обнаружение услуг\. ## Об анализе Последовательность действий для анализа достаточно проста и не потребовала много времени: * Получил Apache Dubbo с [GitHub](https://github.com/apache/dubbo); * Воспользовался [инструкцией](https://pvs-studio.ru/ru/docs/manual/6703/) по запуску Java\-анализатора и запустил анализ; * Получил отчет анализатора, проанализировал его и выделил интересные случаи\. Результаты анализа: 73 предупреждения уровня достоверности High и Medium \(46 и 27, соответственно\) были выданы для 4000\+ файлов, что является хорошим показателем качества кода\. Не все предупреждения являются ошибками\. Это нормальная ситуация и перед регулярным использованием анализатора требуется его настройка\. После чего можно ожидать достаточно низкий процент ложных срабатываний \([пример](https://pvs-studio.ru/ru/blog/posts/cpp/0523/)\)\. Среди предупреждений не рассматривались 9 предупреждений \(7 High и 2 Medium\), приходящихся на файлы с тестами\. В итоге осталось небольшое количество предупреждений, но среди них также остались ложные срабатывания, так как я не настраивал анализатор\. Разбирать 73 предупреждения в статье \- это долго, глупо и нудно, поэтому были отобраны самые интересные\. ## Знаковый byte в Java [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'endKey\[i\] < 0xff' is always true\. OptionUtil\.java\(32\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'isPreferIPV6Address\(\)' is always false\. NetUtils\.java\(236\) ```cpp private static Optional 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_\. ```cpp static boolean isPreferIPV6Address() { boolean preferIpv6 = Boolean.getBoolean("java.net.preferIPv6Addresses"); if (!preferIpv6) { return false; // <= } return false; // <= } ``` Метод _isPreferIPV6Address_ возвращает _false_ в обоих случаях, скорее всего, что один из случаев должен был вернуть _true_ по задумке программиста, иначе метод не имеет смысла\. ## Бессмысленные проверки [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\!mask\[i\]\.equals\(ipAddress\[i\]\)' is always true\. NetUtils\.java\(476\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'message\.length \> 0' is always true\. DeprecatedTelnetCodec\.java\(302\) [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'message \!\= null' is always true\. DeprecatedTelnetCodec\.java\(302\) ```cpp 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 производится проверка: ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\!shouldExport\(\)' is always false\. ServiceConfig\.java\(371\) ```cpp public synchronized void export() { checkAndUpdateSubConfigs(); if (!shouldExport()) { // <= return; } if (shouldDelay()) { .... } else { doExport(); } ``` Метод _shouldExport_ класса _ServiceConfig_ вызывает метод _getExport_ определенный в этом же классе\. ```cpp 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_ для установки значения поля\. ```cpp protected Boolean export; .... public Boolean getExport() { return export; } .... public void setExport(Boolean export) { this.export = export; } ``` Устанавливается поле _export_ в коде только методом _setExport_\. Метод _setExport_ вызывается только в методе _build_ абстрактного класса _AbstractServiceBuilder_ \(расширяющего _AbstractServiceConfig_\) и только если поле export не равно null\. ```cpp @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_ до выполнения кода: ```cpp if (shouldDelay()) { DELAY_EXPORT_EXECUTOR.schedule(this::doExport, getDelay(), ....); } else { doExport(); } ``` ## Неотрицательное значение параметра [V6009](https://pvs-studio.ru/ru/docs/warnings/v6009/) The 'substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect argument: 2\. AbstractEtcdClient\.java\(169\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6016/) Suspicious access to element of 'types' object by a constant index inside a loop\. RpcUtils\.java\(153\) ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. GrizzlyCodecAdapter\.java\(136\) ```cpp @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](https://pvs-studio.ru/ru/docs/warnings/v6067/) Two or more case\-branches perform the same actions\. JVMUtil\.java\(67\), JVMUtil\.java\(71\) ```cpp 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](https://ru.wikipedia.org/wiki/Удалённый_вызов_процедур) в Java, тот наверняка слышал о Apache Dubbo\. Это популярный open source проект с долгой историей и кодом, написанным многими разработчиками\. Код этого проекта отличного качества, но статическому анализатору PVS\-Studio удалось найти некоторое количество ошибок\. Из этого можно сделать вывод, что статический анализ очень важен при разработке средних и больших проектов, как бы не был идеален ваш код\. Примечание\. Подобные разовые проверки демонстрируют возможности статического анализатора кода, но являются совершенно неправильным способом его использовать\. Более подробно эта мысль изложена [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0594/) и [здесь](https://pvs-studio.ru/ru/blog/posts/cpp/0639/)\. Используйте анализ регулярно\! Спасибо разработчикам Apache Dubbo за столь замечательный инструмент\. Надеюсь, эта статья поможет вам в улучшении кода\. В статье описаны не все подозрительные участки кода, так что разработчикам лучше проверить проект самостоятельно и оценить полученные результаты\.