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.

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