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

    image1.png

    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:


    Предупреждение 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.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

    Комментарии 0

    Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

    Самое читаемое