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.