Проверка WildFly — сервера JavaEE приложений

WildFly (ранее назывался JBoss Application Server) — это сервер JavaEE приложений с открытым исходным кодом, созданный компанией JBoss в феврале 2008 года. Основная цель проекта WildFly — предоставить набор инструментов, которые обычно необходимы корпоративным приложениям Java. А поскольку сервер используется для разработки корпоративных приложений, особенно важно минимизировать количество ошибок и возможных уязвимостей в коде. Сейчас WildFly разрабатывает крупная компания Red Hat, и качество кода проекта поддерживается на достаточно высоком уровне, однако анализатору всё равно удалось найти некоторое количество ошибок, допущенных в проекте.

Меня зовут Дмитрий, и недавно я присоединился к команде PVS-Studio в качестве Java программиста. Как известно, лучший способ познакомиться с анализатором кода – попробовать его в деле, поэтому было решено выбрать какой-нибудь интересный проект, проверить его и по результатам написать об этом статью. Именно ее вы сейчас и читаете. 🙂

Анализ проекта

Для анализа я использовал исходный код проекта WildFly, опубликованный на GitHub. Cloc насчитал в проекте 600 тысяч строк кода на Java, без учета пустых и комментариев. Поиск ошибок в коде проводился PVS-Studio. 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 The 'then' statement is equivalent to the 'else' statement. WeldPortableExtensionProcessor.java(61), WeldPortableExtensionProcessor.java(65).

@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 Expression 'poolStatsSize > 0' is always true. PooledConnectionFactoryStatisticsService.java(85)

@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(93)
• V6007 Expression 'mBeanServer == null' is always true. WildFlyServerPlatform.java(82)
• V6007 Expression 'result != null' is always true. New returns not-null reference. JarCheck.java(84)
• V6007 Expression 'result' is always true. MultipleAdminObject2Impl.java(147)

Предупреждение N3 – обращение по нулевой ссылке

V6008 Null dereference of 'tc'. ExternalPooledConnectionFactoryService.java(382)

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 Unreachable code detected. It is possible that an error is present. EJB3Subsystem12Parser.java(79)

V6037 An unconditional 'throw' within a loop. EJB3Subsystem12Parser.java(81)

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 и V6037. Тут выполняется только одна итерация цикла, после чего происходит выход из него по безусловному throw. В таком виде метод readAttributes выбрасывает исключение, если reader содержит хотя бы один атрибут. Данный цикл можно заменить на эквивалентное условие:

if(reader.getAttributeCount() > 0) {   throw ParseUtils.unexpectedAttribute(reader, 0); }

Однако, можно копнуть немного глубже и посмотреть на метод requireNoNamespaceAttribute(….):

public static void requireNoNamespaceAttribute  (XMLExtendedStreamReader reader, int index)    throws XMLStreamException {    if (!isNoNamespaceAttribute(reader, index)) {         throw unexpectedAttribute(reader, index);    } }

Обнаруживается, что этот метод внутри себя выбрасывает то же исключение. Скорее всего, метод readAttributes должен проверять, что ни один указанный атрибут не принадлежит какому-либо namespace, а не то, что атрибуты отсутствуют. Хотелось бы сказать, что такая конструкция возникла в результате рефакторинга кода и выноса исключения в метод requireNoNamespaceAttribute. Вот только просмотр истории коммитов говорит о том, что весь этот код был добавлен одновременно.

Предупреждение N5 – передача параметров в конструктор

V6022 Parameter 'mechanismName' is not used inside constructor body. DigestAuthenticationMechanism.java(144)

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); }

Обычно неиспользуемые переменные и параметры функций не являются чем-то критичным: в основном, они остаются после рефакторинга или добавлены для реализации нового функционала в будущем. Однако данное срабатывание показалось мне достаточно подозрительным:

public DigestAuthenticationMechanism   (final List<DigestAlgorithm> supportedAlgorithms,     final List<DigestQop> supportedQops,    final String realmName,     final String domain,     final NonceManager nonceManager,     final String mechanismName,     final IdentityManager identityManager,    boolean validateUri) {....}

Если посмотреть на второй конструктор класса, видно, что в качестве шестого параметра предполагается строка mechanizmName. Первый конструктор в качестве третьего параметра получает строку с таким же именем и вызывает второй конструктор. Однако эта строка не используется, и во второй конструктор вместо нее передается константа. Возможно, автор кода тут планировал передавать mechanismName в конструктор вместо константы DEFAULT_NAME.

Предупреждение N6 – дублирование строк

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)

private static final Map<String, String>  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 Incorrect format. A different number of format items is expected. Missing arguments: 2. TxTestUtil.java(80)

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 The 'equals' function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java(563)

 // 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() в виде строки. Исправленный вариант мог бы выглядеть так:

return !value.equals(attribute.getDefaultValue().asString());

В WildFly присутствует еще одно аналогичное срабатывание диагностики V6058:

• V6058 The 'equals' function compares objects of incompatible types: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java(141)

Предупреждение N9 – запоздалая проверка

V6060 The 'dataSourceController' reference was utilized before it was verified against null. AbstractDataSourceAdd.java(399), AbstractDataSourceAdd.java(297)

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 Unsafe double-checked locking. A previously assigned object may be replaced by another object. JspApplicationContextWrapper.java(74), JspApplicationContextWrapper.java(72)

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 и проверить с помощью него свой проект. Для этого можно запросить пробную лицензию или воспользоваться одним из бесплатных вариантов использования.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Dmitry Scherbakov. Checking WildFly, a JavaEE Application Server.