WildFly (anciennement JBoss Application Server) est un serveur d'applications JavaEE open source créé par JBoss en février 2008. L'objectif principal du projet WildFly est de fournir un ensemble d'outils qui sont généralement nécessaires aux applications Java d'entreprise. Et comme le serveur est utilisé pour développer des applications d'entreprise, il est particulièrement important de minimiser le nombre d'erreurs et d'éventuelles vulnérabilités dans le code. Maintenant, WildFly est développé par une grande entreprise Red Hat, et la qualité du code du projet est maintenue à un niveau assez élevé, mais l'analyseur a quand même réussi à trouver un certain nombre d'erreurs dans le projet.
Je m'appelle Dmitry et j'ai récemment rejoint l'équipe PVS-Studio en tant que programmeur Java. Comme vous le savez, la meilleure façon de se familiariser avec l'analyseur de code est de l'essayer en pratique, il a donc été décidé de choisir un projet intéressant, de le vérifier et d'écrire un article à ce sujet en fonction des résultats. C'est ce que vous lisez maintenant. :)
Analyse de projet
Pour l'analyse, j'ai utilisé le code source du projet WildFly publié sur GitHub . Cloc a compté 600 000 lignes de code Java dans le projet, à l'exclusion des blancs et des commentaires. La recherche d'erreurs dans le code a été effectuée par PVS-Studio . PVS-Studio est un outil de recherche d'erreurs et de vulnérabilités potentielles dans le code source des programmes écrits en C, C ++, C # et Java. Le plug-in d'analyseur pour IntelliJ IDEA version 7.09 a été utilisé.
Suite à la vérification du projet, seuls 491 déclencheurs d'analyseurs ont été reçus, ce qui indique un bon niveau de qualité du code WildFly. Parmi ceux-ci, 113 sont élevés et 146 sont moyens. Dans le même temps, une bonne partie des points positifs tombe sur le diagnostic:
- V6002. L'instruction switch ne couvre pas toutes les valeurs de l'énumération.
- V6008. Déréférence nulle potentielle.
- V6021. La valeur est affectée à la variable «x» mais n'est pas utilisée.
Je n'ai pas envisagé le déclenchement de ces diagnostics dans l'article, car il est difficile de comprendre s'il s'agit en fait d'erreurs. Ces avertissements sont mieux compris par les auteurs du code.
Ensuite, nous examinerons 10 déclencheurs d'analyseurs que j'ai trouvés les plus intéressants. Demandez - pourquoi 10? Juste parce que le nombre est comme ça. :)
Alors, allons-y.
Avertissement N1 est une instruction conditionnelle inutile
V6004 L' instruction «then» équivaut à l'instruction «else». 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;
}
}
}Le code dans les branches if et else est le même, et l'opérateur conditionnel n'a pas de sens dans sa forme actuelle. Il est difficile de penser à une raison pour laquelle un développeur a écrit cette méthode de cette façon. Très probablement, l'erreur s'est produite à la suite d'un copier-coller ou d'une refactorisation.
Avertissement N2 - Conditions en double
L' expression V6007 'poolStatsSize> 0' est toujours vraie. PooledConnectionFactoryStatisticsService.java (85)
@Override
public void start(StartContext context) throws StartException {
....
if (poolStatsSize > 0) {
if (registration != null) {
if (poolStatsSize > 0) {
....
}
}
}
}Dans ce cas, il y avait une duplication des conditions. Cela n'affectera pas les résultats du programme, mais cela aggravera la lisibilité du code. Cependant, il est possible que la deuxième vérification contienne une autre condition plus forte.
Autres exemples de ce diagnostic déclenché dans WildFly:
- L'expression V6007 'referralMode == null' est toujours fausse. DirContext.java (93)
- L'expression V6007 'mBeanServer == null' est toujours vraie. WildFlyServerPlatform.java (82)
- V6007 Expression 'result! = Null' est toujours vrai. New renvoie une référence non nulle. JarCheck.java (84)
- V6007 L'expression «résultat» est toujours vraie. MultipleAdminObject2Impl.java (147)
Avertissement N3 - référence par référence nulle
V6008 Déréférence nulle de «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());
}
}
....
}Ils ont manifestement foiré ici. Tout d'abord, nous nous assurons que la référence est nulle, puis nous appelons la méthode getName sur cette référence très nulle. Cela entraînera une NullPointerException au lieu de l'exception attendue de connectorNotDefined (....).
Avertissement N4 - code très étrange
V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. EJB3Subsystem12Parser.java (79)
V6037 Un «lancer» inconditionnel dans une boucle. 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);
}
}Une conception extrêmement étrange, à laquelle deux diagnostics ont réagi à la fois: V6019 et V6037 . Ici, une seule itération de la boucle est effectuée, après quoi elle se termine par un jet inconditionnel . En tant que telle, la méthode readAttributes lève une exception si le lecteur contient au moins un attribut. Cette boucle peut être remplacée par une condition équivalente:
if(reader.getAttributeCount() > 0) {
throw ParseUtils.unexpectedAttribute(reader, 0);
}Cependant, vous pouvez creuser un peu plus et regarder la méthode requireNoNamespaceAttribute (....) :
public static void requireNoNamespaceAttribute
(XMLExtendedStreamReader reader, int index)
throws XMLStreamException {
if (!isNoNamespaceAttribute(reader, index)) {
throw unexpectedAttribute(reader, index);
}
}On constate que cette méthode lève en interne la même exception. Très probablement, la méthode readAttributes doit vérifier qu'aucun des attributs spécifiés n'appartient à un espace de noms, et non que les attributs sont manquants. Je voudrais dire qu'une telle construction est le résultat de la refactorisation du code et du lancement d'une exception dans la méthode requireNoNamespaceAttribute . Le simple fait de regarder l'historique des validations montre que tout ce code a été ajouté en même temps.
Avertissement N5 - passer des paramètres au constructeur
V6022 Le paramètre ' nomMécanisme ' n'est pas utilisé dans le corps du constructeur. 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);
}Habituellement, les variables et paramètres de fonction inutilisés ne sont pas critiques: en général, ils restent après la refactorisation ou sont ajoutés pour implémenter de nouvelles fonctionnalités à l'avenir. Cependant, cette opération m'a paru assez suspecte:
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) {....}Si vous regardez le deuxième constructeur de la classe, vous pouvez voir que MechanizmName est considéré comme le sixième paramètre . Le premier constructeur prend une chaîne du même nom que le troisième paramètre et appelle le deuxième constructeur. Cependant, cette chaîne n'est pas utilisée et une constante est transmise au deuxième constructeur à la place. Peut-être que l'auteur du code a prévu de passer le nom du mécanisme au constructeur au lieu de la constante DEFAULT_NAME .
Avertissement N6 - lignes en double
V6033 Un élément avec la même clé 'org.apache.activemq.artemis.core.remoting.impl.netty.
TransportConstants.NIO_REMOTING_THREADS_PROPNAME 'a déjà été ajouté. 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);
....
}L'analyseur signale que deux valeurs avec la même clé ont été ajoutées au dictionnaire. Dans ce cas, les correspondances clé-valeur ajoutées sont complètement dupliquées. Les valeurs enregistrées sont des constantes dans la classe TransportConstants , et il peut y avoir l'une des deux options suivantes: soit l'auteur a accidentellement dupliqué le code, soit oublié de modifier les valeurs lors du copier-coller. Lors d'une inspection superficielle, je n'ai pas pu trouver de clés et de valeurs manquantes, je suppose donc que le premier scénario est plus probable.
Avertissement N7 - Variables perdues
V6046 Format incorrect. Un nombre différent d'éléments de format est attendu. Arguments manquants: 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);
}
}Les variables sont perdues (voulues). On supposait que deux autres lignes seraient remplacées dans la chaîne formatée, mais l'auteur du code, apparemment, a oublié de les ajouter. Le formatage d'une chaîne sans arguments appropriés lèvera une IllegalFormatException au lieu de l' Exception RuntimeException voulue par les développeurs. En principe, IllegalFormatException hérite de RuntimeException , mais le message passé à l'exception sera perdu dans la sortie, et il sera plus difficile de comprendre ce qui s'est passé exactement lors du débogage.
Avertissement N8 - comparaison chaîne à objet
V6058 La fonction 'equals' compare des objets de types incompatibles: 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()); // <=
}Dans ce cas, la chaîne est comparée à un objet et une telle comparaison est toujours fausse. Autrement dit, si une valeur modelNode égale à attribute.getDefaultValue () est transmise à la méthode , alors le comportement de la méthode s'avérera incorrect et la valeur sera reconnue comme valide pour l'envoi malgré le commentaire.
Très probablement, ils ont oublié d'appeler la méthode asString () pour représenter attribute.getDefaultValue () sous forme de chaîne. La version corrigée pourrait ressembler à ceci:
return !value.equals(attribute.getDefaultValue().asString());WildFly a un autre déclenchement similaire du diagnostic V6058 :
- V6058 La fonction 'equals' compare des objets de types incompatibles: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java (141)
Avertissement N9 - Contrôle tardif
V6060 La référence 'dataSourceController' a été utilisée avant d'être vérifiée par rapport à 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) {....}
....
}L'analyseur a constaté que l'objet était utilisé dans le code bien avant qu'il ne soit vérifié pour null , et la distance entre eux est jusqu'à 102 lignes de code! Ceci est extrêmement difficile à remarquer lors de l'analyse manuelle du code.
Avertissement N10 - verrouillage à double contrôle
V6082 Verrouillage non sécurisé à double contrôle. Un objet précédemment attribué peut être remplacé par un autre objet. 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;
}Cela utilise le modèle de «verrouillage double vérifié» et une situation peut se produire lorsque la méthode renvoie une variable incomplètement initialisée.
Le thread A remarque que la valeur n'a pas été initialisée, il acquiert donc le verrou et commence l'initialisation de la valeur. Dans ce cas, le thread parvient à écrire l'objet dans le champ avant même qu'il n'ait été initialisé jusqu'à la fin. Le thread B détecte que l'objet a été créé et le renvoie, bien que le thread A n'ait pas encore eu le temps de faire tout le travail avec l' usine .
En conséquence, un objet peut être renvoyé par la méthode, sur laquelle toutes les actions planifiées n'ont pas été effectuées.
conclusions
Malgré le fait que le projet soit développé par une grande entreprise Red Hat et que la qualité du code du projet soit à un niveau élevé, l'analyse statique réalisée à l'aide de PVS-Studio a pu révéler un certain nombre d'erreurs qui d'une manière ou d'une autre peuvent affecter le fonctionnement du serveur. Et comme WildFly est conçu pour créer des applications d'entreprise, ces erreurs peuvent entraîner des conséquences extrêmement tristes.
J'invite tout le monde à télécharger PVS-Studio et à vérifier son projet avec. Pour ce faire, vous pouvez demander une licence d'essai ou utiliser l'un des cas d'utilisation gratuits .
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Dmitry Scherbakov. Vérification de WildFly, un serveur d'applications JavaEE .