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.

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