# Проверка WildFly \- сервера JavaEE приложений WildFly \(ранее назывался JBoss Application Server\) — это сервер JavaEE приложений с открытым исходным кодом, созданный компанией JBoss в феврале 2008 года\. Основная цель проекта WildFly \- предоставить набор инструментов, которые обычно необходимы корпоративным приложениям Java\. А поскольку сервер используется для разработки корпоративных приложений, особенно важно минимизировать количество ошибок и возможных уязвимостей в коде\. Сейчас WildFly разрабатывает крупная компания Red Hat, и качество кода проекта поддерживается на достаточно высоком уровне, однако анализатору всё равно удалось найти некоторое количество ошибок, допущенных в проекте\. ![0763_WildFly_Check_ru/image1.png](https://import.viva64.com/docx/blog/0763_WildFly_Check_ru/image1.png) Меня зовут Дмитрий, и недавно я присоединился к команде PVS\-Studio в качестве Java программиста\. Как известно, лучший способ познакомиться с анализатором кода – попробовать его в деле, поэтому было решено выбрать какой\-нибудь интересный проект, проверить его и по результатам написать об этом статью\. Именно ее вы сейчас и читаете\. :\) ## Анализ проекта Для анализа я использовал исходный код проекта WildFly, опубликованный на [GitHub](https://github.com/wildfly/wildfly/tree/303dcde5ff8e5f3787320e376fb6c5ff3c4d039f)\. [Cloc](https://github.com/AlDanial/cloc) насчитал в проекте 600 тысяч строк кода на Java, без учета пустых и комментариев\. Поиск ошибок в коде проводился [PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/download/)\. PVS\-Studio \- инструмент поиска ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C\+\+, C\# и Java\. Использовался плагин анализатора для IntelliJ IDEA версии 7\.09\. В результате проверки проекта было получено всего 491 срабатывание анализатора, что говорит о хорошем уровне качества кода WildFly\. Среди них 113 имеют уровень high и 146 – medium\. При этом приличная часть срабатываний приходится на диагностики: * V6002\. The switch statement does not cover all values of the enum\. * V6008\. Potential null dereference\. * V6021\. The value is assigned to the 'x' variable but is not used\. Срабатывание этих диагностик я не рассматривал в статье, так как сложно понять, являются ли они на самом деле ошибками\. Такие предупреждения лучше разбирать авторам кода\. Далее мы рассмотрим 10 срабатываний анализатора, которые показались мне самыми интересными\. Спросите – почему именно 10? Просто, потому что число нравится\. :\) Итак, поехали\. ## Предупреждение N1 – бесполезный условный оператор [V6004](https://pvs-studio.ru/ru/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. WeldPortableExtensionProcessor\.java\(61\), WeldPortableExtensionProcessor\.java\(65\)\. ```cpp @Override public void deploy(DeploymentPhaseContext phaseContext) throws DeploymentUnitProcessingException { final DeploymentUnit deploymentUnit = phaseContext.getDeploymentUnit(); // for war modules we require a beans.xml to load portable extensions if (PrivateSubDeploymentMarker.isPrivate(deploymentUnit)) { if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) { return; } } else { // if any deployments have a beans.xml we need // to load portable extensions // even if this one does not. if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) { return; } } } ``` Код в ветках _if_ и _else_ одинаковый, и условный оператор не имеет смысла в текущем виде\. Сложно придумать причину, по которой разработчик написал данный метод таким образом\. Скорее всего, ошибка возникла в результате копипаста или рефакторинга\. ## Предупреждение N2 – дублирование условий [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'poolStatsSize \> 0' is always true\. PooledConnectionFactoryStatisticsService\.java\(85\) ```cpp @Override public void start(StartContext context) throws StartException { .... if (poolStatsSize > 0) { if (registration != null) { if (poolStatsSize > 0) { .... } } } } ``` В данном случае обнаружилось дублирование условий\. На результаты работы программы это не повлияет, но ухудшает читаемость кода\. Однако, возможно, вторая проверка должна была содержать в себе какое\-то другое, более сильное условие\. Другие примеры срабатывания этой диагностики в WildFly: * V6007 Expression 'referralMode \=\= null' is always false\. [DirContext\.java](https://github.com/wildfly/wildfly/blob/bf1c7695de3f9bd16426f83623cdc6341af36480/testsuite/shared/src/main/java/org/wildfly/test/security/common/elytron/DirContext.java)\(93\) * V6007 Expression 'mBeanServer \=\= null' is always true\. [WildFlyServerPlatform\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/jpa/eclipselink/src/main/java/org/jipijapa/eclipselink/WildFlyServerPlatform.java)\(82\) * V6007 Expression 'result \!\= null' is always true\. New returns not\-null reference\. [JarCheck\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/jdr/jboss-as-jdr/src/main/java/org/jboss/as/jdr/commands/JarCheck.java)\(84\) * V6007 Expression 'result' is always true\. [MultipleAdminObject2Impl\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/testsuite/integration/basic/src/test/java/org/jboss/as/test/integration/jca/rar/MultipleAdminObject2Impl.java)\(147\) ## Предупреждение N3 – обращение по нулевой ссылке [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Null dereference of 'tc'\. ExternalPooledConnectionFactoryService\.java\(382\) ```cpp private void createService(ServiceTarget serviceTarget, ServiceContainer container) throws Exception { .... for (TransportConfiguration tc : connectors) { if (tc == null) { throw MessagingLogger.ROOT_LOGGER.connectorNotDefined(tc.getName()); } } .... } ``` Здесь явно накосячили\. Сначала убеждаемся, что ссылка нулевая, а затем вызываем метод _getName_ на этой самой нулевой ссылке\. В результате получим _NullPointerException_ вместо ожидаемого исключения из _connectorNotDefined\(\.\.\.\.\)\._ ## Предупреждение N4 – очень странный код [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. EJB3Subsystem12Parser\.java\(79\) [V6037](https://pvs-studio.ru/ru/docs/warnings/v6037/) An unconditional 'throw' within a loop\. EJB3Subsystem12Parser\.java\(81\) ```cpp protected void readAttributes(final XMLExtendedStreamReader reader) throws XMLStreamException { for (int i = 0; i < reader.getAttributeCount(); i++) { ParseUtils.requireNoNamespaceAttribute(reader, i); throw ParseUtils.unexpectedAttribute(reader, i); } } ``` Крайне странная конструкция, на которую отреагировали сразу две диагностики: [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) и [V6037](https://pvs-studio.ru/ru/docs/warnings/v6037/)\. Тут выполняется только одна итерация цикла, после чего происходит выход из него по безусловному _throw_\. В таком виде метод _readAttributes_ выбрасывает исключение, если _reader_ содержит хотя бы один атрибут\. Данный цикл можно заменить на эквивалентное условие: ```cpp if(reader.getAttributeCount() > 0) { throw ParseUtils.unexpectedAttribute(reader, 0); } ``` Однако, можно копнуть немного глубже и посмотреть на метод _requireNoNamespaceAttribute\(\.\.\.\.\)_ : ```cpp public static void requireNoNamespaceAttribute (XMLExtendedStreamReader reader, int index) throws XMLStreamException { if (!isNoNamespaceAttribute(reader, index)) { throw unexpectedAttribute(reader, index); } } ``` Обнаруживается, что этот метод внутри себя выбрасывает то же исключение\. Скорее всего, метод _readAttributes_ должен проверять, что ни один указанный атрибут не принадлежит какому\-либо namespace, а не то, что атрибуты отсутствуют\. Хотелось бы сказать, что такая конструкция возникла в результате рефакторинга кода и выноса исключения в метод _requireNoNamespaceAttribute_\. Вот только просмотр истории коммитов говорит о том, что весь этот код был [добавлен](https://github.com/wildfly/wildfly/blob/2598dfcff01751afc7069949f322e8ae08688c38/ejb3/src/main/java/org/jboss/as/ejb3/subsystem/EJB3Subsystem12Parser.java) одновременно\. ## Предупреждение N5 – передача параметров в конструктор [V6022](https://pvs-studio.ru/ru/docs/warnings/v6022/) Parameter 'mechanismName' is not used inside constructor body\. DigestAuthenticationMechanism\.java\(144\) ```cpp public DigestAuthenticationMechanism(final String realmName, final String domain, final String mechanismName, final IdentityManager identityManager, boolean validateUri) { this(Collections.singletonList(DigestAlgorithm.MD5), Collections.singletonList(DigestQop.AUTH), realmName, domain, new SimpleNonceManager(), DEFAULT_NAME, identityManager, validateUri); } ``` Обычно неиспользуемые переменные и параметры функций не являются чем\-то критичным: в основном, они остаются после рефакторинга или добавлены для реализации нового функционала в будущем\. Однако данное срабатывание показалось мне достаточно подозрительным: ```cpp public DigestAuthenticationMechanism (final List supportedAlgorithms, final List supportedQops, final String realmName, final String domain, final NonceManager nonceManager, final String mechanismName, final IdentityManager identityManager, boolean validateUri) {....} ``` Если посмотреть на второй конструктор класса, видно, что в качестве шестого параметра предполагается строка _mechanizmName_\. Первый конструктор в качестве третьего параметра получает строку с таким же именем и вызывает второй конструктор\. Однако эта строка не используется, и во второй конструктор вместо нее передается константа\. Возможно, автор кода тут планировал передавать _mechanismName_ в конструктор вместо константы _DEFAULT\_NAME_\. ## Предупреждение N6 – дублирование строк [V6033](https://pvs-studio.ru/ru/docs/warnings/v6033/) An item with the same key 'org\.apache\.activemq\.artemis\.core\.remoting\.impl\.netty\.TransportConstants\.NIO\_REMOTING\_THREADS\_PROPNAME' has already been added\. LegacyConnectionFactoryService\.java\(145\), LegacyConnectionFactoryService\.java\(139\) ```cpp private static final Map PARAM_KEY_MAPPING = new HashMap<>(); .... static { PARAM_KEY_MAPPING.put( org.apache.activemq.artemis.core.remoting.impl.netty .TransportConstants.NIO_REMOTING_THREADS_PROPNAME, TransportConstants.NIO_REMOTING_THREADS_PROPNAME); .... PARAM_KEY_MAPPING.put( org.apache.activemq.artemis.core.remoting.impl.netty .TransportConstants.NIO_REMOTING_THREADS_PROPNAME, TransportConstants.NIO_REMOTING_THREADS_PROPNAME); .... } ``` Анализатор сообщает о добавлении в словарь двух значений с одинаковым ключом\. В данном случае добавляемые соответствия ключ\-значения полностью дублируются\. Записываемые значения являются константами в классе _TransportConstants_, и тут может быть один из двух вариантов: или автор случайно продублировал код, или при копипасте забыл поменять значения\. Во время беглого осмотра мне не удалось обнаружить пропущенных ключей и значений, поэтому предположу, что первый вариант развития событий является более вероятным\. ## Предупреждение N7 – потерялись переменные [V6046](https://pvs-studio.ru/ru/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Missing arguments: 2\. TxTestUtil\.java\(80\) ```cpp public static void addSynchronization(TransactionManager tm, TransactionCheckerSingletonRemote checker) { try { addSynchronization(tm.getTransaction(), checker); } catch (SystemException se) { throw new RuntimeException(String .format("Can't obtain transaction for transaction manager '%s' " + "to enlist add test synchronization '%s'"), se); } } ``` Потерялись \(разыскиваются\) переменные\. Предполагалось, что в форматированную строку будут подставлены две других строки, однако автор кода, видимо, забыл их добавить\. Форматирование строки без соответствующих аргументов выбросит исключение _IllegalFormatException_ вместо _RuntimeException_, предполагающегося разработчиками\. В принципе, _IllegalFormatException_ наследуется от _RuntimeException_, но в выдаче потеряется сообщение, передаваемое в исключение, и понять, что именно пошло не так, при отладке будет сложнее\. ## Предупреждение N8 – сравнение строки с объектом [V6058](https://pvs-studio.ru/ru/docs/warnings/v6058/) The 'equals' function compares objects of incompatible types: String, ModelNode\. JaxrsIntegrationProcessor\.java\(563\) ```cpp // Send value to RESTEasy only if it's not null, empty string, or the // default value. private boolean isTransmittable(AttributeDefinition attribute, ModelNode modelNode) { if (modelNode == null || ModelType .UNDEFINED.equals(modelNode.getType())) { return false; } String value = modelNode.asString(); if ("".equals(value.trim())) { return false; } return !value.equals(attribute.getDefaultValue()); // <= } ``` В данном случае строка сравнивается с объектом, и такое сравнение всегда ложно\. То есть, если в метод будет передано значение _modelNode_, равное _attribute\.getDefaultValue\(\)_, то поведение метода окажется неверным, и значение будет признано допустимым к отправке вопреки комментарию\. Скорее всего, тут забыли вызвать метод _asString\(\)_, чтобы представить _attribute\.getDefaultValue\(\)_ в виде строки\. Исправленный вариант мог бы выглядеть так: ```cpp return !value.equals(attribute.getDefaultValue().asString()); ``` В WildFly присутствует еще одно аналогичное срабатывание диагностики [V6058](https://pvs-studio.ru/ru/docs/warnings/v6058/): * V6058 The 'equals' function compares objects of incompatible types: String, ObjectTypeAttributeDefinition\. [DataSourceDefinition\.java](https://github.com/wildfly/wildfly/blob/12da512624970c0e6e28aab85bdbef9fdac31108/connector/src/main/java/org/jboss/as/connector/subsystems/datasources/DataSourceDefinition.java)\(141\) ## Предупреждение N9 – запоздалая проверка [V6060](https://pvs-studio.ru/ru/docs/warnings/v6060/) The 'dataSourceController' reference was utilized before it was verified against null\. AbstractDataSourceAdd\.java\(399\), AbstractDataSourceAdd\.java\(297\) ```cpp static void secondRuntimeStep(OperationContext context, ModelNode operation, ManagementResourceRegistration datasourceRegistration, ModelNode model, boolean isXa) throws OperationFailedException { final ServiceController dataSourceController = registry.getService(dataSourceServiceName); .... dataSourceController.getService() .... if (dataSourceController != null) {....} .... } ``` Анализатор обнаружил, что объект используется в коде задолго до его проверки на _null_, причем расстояние между ними аж в 102 строки кода\! При ручном анализе кода такое крайне сложно заметить\. ## Предупреждение N10 \- double\-checked locking [V6082](https://pvs-studio.ru/ru/docs/warnings/v6082/) Unsafe double\-checked locking\. A previously assigned object may be replaced by another object\. JspApplicationContextWrapper\.java\(74\), JspApplicationContextWrapper\.java\(72\) ```cpp private volatile ExpressionFactory factory; .... @Override public ExpressionFactory getExpressionFactory() { if (factory == null) { synchronized (this) { if (factory == null) { factory = delegate.getExpressionFactory(); for (ExpressionFactoryWrapper wrapper : wrapperList) { factory = wrapper.wrap(factory, servletContext); } } } } return factory; } ``` Тут используется паттерн "блокировка с двойной проверкой", и может произойти ситуация, при которой метод вернет не до конца инициализированную переменную\. Поток A замечает, что значение не инициализировано, поэтому он получает блокировку и начинает инициализировать значение\. При этом поток успевает записать объект в поле ещё до того, как он был проинициализирован до конца\. Поток B обнаруживает, что объект создан, и возвращает его, хотя поток А еще не успел выполнить все действия с _factory_\. В итоге из метода может быть возвращён объект, над которым выполнились не все запланированные действия\. ## Выводы Несмотря на то, что проект разрабатывается крупной компанией Red Hat и качество кода в проекте на высоком уровне, статический анализ, проведенный с помощью PVS\-Studio, смог выявить определенное количество ошибок, которые тем или иным образом могут влиять на работу сервера\. А поскольку WildFly предназначен для создания корпоративных приложений, эти ошибки могут привести к крайне печальным последствиям\. Предлагаю всем желающим скачать PVS\-Studio и проверить с помощью него свой проект\. Для этого можно запросить [пробную лицензию](https://pvs-studio.ru/ru/pvs-studio/download/) или воспользоваться одним из [бесплатных](https://pvs-studio.ru/ru/blog/posts/0614/) вариантов использования\.