WildFly (ehemals JBoss Application Server) ist ein Open-Source-JavaEE-Anwendungsserver, der im Februar 2008 von JBoss erstellt wurde. Das Hauptziel des WildFly-Projekts besteht darin, eine Reihe von Tools bereitzustellen, die normalerweise von Java-Unternehmensanwendungen benötigt werden. Da der Server für die Entwicklung von Unternehmensanwendungen verwendet wird, ist es besonders wichtig, die Anzahl der Fehler und möglichen Schwachstellen im Code zu minimieren. WildFly wird derzeit von einem großen Unternehmen, Red Hat, entwickelt, und die Qualität des Projektcodes wird auf einem relativ hohen Niveau gehalten, aber der Analysator konnte dennoch eine Reihe von Fehlern im Projekt finden.
Mein Name ist Dmitry und ich bin kürzlich als Java-Programmierer zum PVS-Studio-Team gekommen. Wie Sie wissen, ist der beste Weg, sich mit dem Code-Analysator vertraut zu machen, ihn in der Praxis auszuprobieren. Daher wurde beschlossen, ein interessantes Projekt auszuwählen, es zu überprüfen und basierend auf den Ergebnissen einen Artikel darüber zu schreiben. Das lesen Sie gerade. :) :)
Projektanalyse
Für die Analyse habe ich den Quellcode des auf GitHub veröffentlichten WildFly-Projekts verwendet . Cloc zählte 600.000 Zeilen Java-Code im Projekt, ohne Leerzeichen und Kommentare. Die Suche nach Fehlern im Code wurde von PVS-Studio durchgeführt . PVS-Studio ist ein Tool zur Suche nach Fehlern und potenziellen Schwachstellen im Quellcode von Programmen, die in C, C ++, C # und Java geschrieben wurden. Das Analysator-Plugin für IntelliJ IDEA Version 7.09 wurde verwendet.
Als Ergebnis der Überprüfung des Projekts wurden nur 491 Analysator-Trigger empfangen, was auf eine gute WildFly-Codequalität hinweist. Von diesen sind 113 hoch und 146 mittel. Gleichzeitig fällt ein anständiger Teil der Erkennungen auf die Diagnose:
- V6002. Die switch-Anweisung deckt nicht alle Werte der Aufzählung ab.
- V6008. Mögliche Null-Dereferenzierung.
- V6021. Der Wert wird der Variablen 'x' zugewiesen, aber nicht verwendet.
Ich habe die Auslösung dieser Diagnosen in dem Artikel nicht berücksichtigt, da es schwierig ist zu verstehen, ob es sich tatsächlich um Fehler handelt. Solche Warnungen werden von den Autoren des Codes besser verstanden.
Als nächstes werden wir uns 10 Analysator-Trigger ansehen, die ich am interessantesten fand. Fragen Sie - warum 10? Nur weil die Nummer so ist. :)
Also, lass uns gehen.
Warnung N1 ist eine nutzlose bedingte Anweisung
V6004 Die Anweisung 'then' entspricht der Anweisung '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;
}
}
}Der Code in den Zweigen if und else ist derselbe, und der bedingte Operator ist in seiner aktuellen Form nicht sinnvoll. Es ist schwer, sich einen Grund vorzustellen, warum ein Entwickler diese Methode so geschrieben hat. Der Fehler trat höchstwahrscheinlich durch Kopieren, Einfügen oder Umgestalten auf.
N2 Warnung - Doppelte Bedingungen
V6007 Der Ausdruck 'poolStatsSize> 0' ist immer wahr. PooledConnectionFactoryStatisticsService.java (85)
@Override
public void start(StartContext context) throws StartException {
....
if (poolStatsSize > 0) {
if (registration != null) {
if (poolStatsSize > 0) {
....
}
}
}
}In diesem Fall gab es eine Verdoppelung der Bedingungen. Dies hat keine Auswirkungen auf die Ergebnisse des Programms, verschlechtert jedoch die Lesbarkeit des Codes. Es ist jedoch möglich, dass die zweite Prüfung eine andere, stärkere Bedingung enthalten sollte.
Weitere Beispiele für diese Diagnose, die in WildFly ausgelöst wird:
- V6007 Der Ausdruck 'referralMode == null' ist immer falsch. DirContext.java (93)
- V6007 Ausdruck 'mBeanServer == null' ist immer wahr. WildFlyServerPlatform.java (82)
- V6007 Ausdruck 'Ergebnis! = Null' ist immer wahr. New gibt eine Referenz ungleich Null zurück. JarCheck.java (84)
- V6007 Der Ausdruck 'Ergebnis' ist immer wahr. MultipleAdminObject2Impl.java (147)
Warnung N3 - Referenz durch Nullreferenz
V6008 Null-Dereferenzierung von '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());
}
}
....
}Sie haben hier offensichtlich versaut. Zuerst stellen wir sicher, dass die Referenz null ist, und rufen dann die getName- Methode für diese Nullreferenz auf. Dies führt zu einer NullPointerException anstelle der erwarteten Ausnahme von connectorNotDefined (....).
Warnung N4 - sehr seltsamer Code
V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. EJB3Subsystem12Parser.java (79)
V6037 Ein bedingungsloser 'Wurf' innerhalb einer Schleife. 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);
}
}Ein äußerst seltsames Design, auf das zwei Diagnosen gleichzeitig reagierten: V6019 und V6037 . Hier wird nur eine Iteration der Schleife durchgeführt, wonach sie durch bedingungslosen Wurf beendet wird . Daher löst die Methode readAttributes eine Ausnahme aus, wenn der Reader mindestens ein Attribut enthält. Diese Schleife kann durch eine äquivalente Bedingung ersetzt werden:
if(reader.getAttributeCount() > 0) {
throw ParseUtils.unexpectedAttribute(reader, 0);
}Sie können jedoch etwas tiefer gehen und sich die Methode requireNoNamespaceAttribute (....) ansehen :
public static void requireNoNamespaceAttribute
(XMLExtendedStreamReader reader, int index)
throws XMLStreamException {
if (!isNoNamespaceAttribute(reader, index)) {
throw unexpectedAttribute(reader, index);
}
}Es wurde festgestellt, dass diese Methode intern dieselbe Ausnahme auslöst. Höchstwahrscheinlich sollte die Methode readAttributes überprüfen, ob keines der angegebenen Attribute zu einem Namespace gehört und nicht, dass die Attribute fehlen. Ich möchte sagen, dass eine solche Konstruktion als Ergebnis des Code-Refactorings und des Auslösens einer Ausnahme in die requireNoNamespaceAttribute- Methode entstanden ist . Ein Blick auf den Commit-Verlauf zeigt, dass der gesamte Code gleichzeitig hinzugefügt wurde.
Warnung N5 - Übergabe von Parametern an den Konstruktor
V6022 Der Parameter 'Mechanismusname' wird im Konstruktorkörper nicht verwendet. 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);
}Normalerweise sind nicht verwendete Variablen und Funktionsparameter nicht kritisch: Im Allgemeinen bleiben sie nach dem Refactoring erhalten oder werden hinzugefügt, um in Zukunft neue Funktionen zu implementieren. Diese Operation schien mir jedoch ziemlich verdächtig:
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) {....}Wenn Sie sich den zweiten Konstruktor der Klasse ansehen, sehen Sie, dass die Zeichenfolge mechanizmName als sechster Parameter angenommen wird . Der erste Konstruktor verwendet eine Zeichenfolge mit demselben Namen wie der dritte Parameter und ruft den zweiten Konstruktor auf. Diese Zeichenfolge wird jedoch nicht verwendet, und stattdessen wird eine Konstante an den zweiten Konstruktor übergeben. Vielleicht wollte der Autor des Codes hier den Mechanismusnamen anstelle der Konstante DEFAULT_NAME an den Konstruktor übergeben .
Warnung N6 - doppelte Zeilen
V6033 Ein Element mit demselben Schlüssel 'org.apache.activemq.artemis.core.remoting.impl.netty.
TransportConstants.NIO_REMOTING_THREADS_PROPNAME 'wurde bereits hinzugefügt. 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);
....
}Der Analysator meldet, dass dem Wörterbuch zwei Werte mit demselben Schlüssel hinzugefügt wurden. In diesem Fall werden die hinzugefügten Schlüsselwertübereinstimmungen vollständig dupliziert. Die aufgezeichneten Werte sind Konstanten in der TransportConstants- Klasse . Es gibt zwei Möglichkeiten: Entweder hat der Autor den Code versehentlich dupliziert oder er hat vergessen, die Werte beim Kopieren und Einfügen zu ändern. Während einer flüchtigen Prüfung konnte ich keine fehlenden Schlüssel und Werte finden, daher gehe ich davon aus, dass das erste Szenario wahrscheinlicher ist.
Warnung N7 - Variablen verloren
V6046 Falsches Format. Eine andere Anzahl von Formatelementen wird erwartet. Fehlende Argumente: 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);
}
}Variablen gehen verloren (gesucht). Es wurde angenommen, dass zwei andere Zeilen in die formatierte Zeichenfolge eingesetzt werden, aber der Autor des Codes hat anscheinend vergessen, sie hinzuzufügen. Durch das Formatieren einer Zeichenfolge ohne entsprechende Argumente wird anstelle der von den Entwicklern beabsichtigten RuntimeException eine IllegalFormatException ausgelöst . Im Prinzip erbt IllegalFormatException von RuntimeException , aber die an die Ausnahme übergebene Nachricht geht in der Ausgabe verloren und es ist schwieriger zu verstehen, was genau beim Debuggen schief gelaufen ist.
Warnung N8 - Vergleich von Zeichenfolge zu Objekt
V6058 Die Funktion 'equals' vergleicht Objekte inkompatibler Typen: 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()); // <=
}In diesem Fall wird die Zeichenfolge mit einem Objekt verglichen, und ein solcher Vergleich ist immer falsch. Das heißt, wenn ein modelNode- Wert gleich attribute.getDefaultValue () an die Methode übergeben wird , stellt sich das Verhalten der Methode als falsch heraus und der Wert wird trotz des Kommentars als gültig für das Senden erkannt.
Höchstwahrscheinlich haben sie vergessen, die asString () -Methode aufzurufen , um attribute.getDefaultValue () als Zeichenfolge darzustellen . Die korrigierte Version könnte folgendermaßen aussehen:
return !value.equals(attribute.getDefaultValue().asString());WildFly hat eine weitere ähnliche Auslösung der V6058- Diagnose :
- V6058 Die Funktion 'equals' vergleicht Objekte inkompatibler Typen: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java (141)
Warnung N9 - Späte Überprüfung
V6060 Die Referenz 'dataSourceController' wurde verwendet, bevor sie gegen null verifiziert wurde. 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) {....}
....
}Der Analysator stellte fest, dass das Objekt lange vor der Überprüfung auf Null im Code verwendet wurde und der Abstand zwischen ihnen bis zu 102 Codezeilen beträgt! Dies ist bei der manuellen Analyse des Codes äußerst schwer zu bemerken.
Warnung N10 - doppelt überprüfte Verriegelung
V6082 Unsichere doppelt überprüfte Verriegelung. Ein zuvor zugewiesenes Objekt kann durch ein anderes Objekt ersetzt werden. 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;
}Dies verwendet das Muster "doppelt geprüftes Sperren", und es kann vorkommen, dass die Methode eine unvollständig initialisierte Variable zurückgibt.
Thread A stellt fest, dass der Wert nicht initialisiert wurde, erhält also die Sperre und beginnt mit der Initialisierung des Werts. In diesem Fall schafft es der Thread, das Objekt in das Feld zu schreiben, noch bevor es bis zum Ende initialisiert wurde. Thread B erkennt, dass das Objekt erstellt wurde, und gibt es zurück, obwohl Thread A noch keine Zeit hatte, die gesamte Arbeit mit der Fabrik zu erledigen .
Infolgedessen kann ein Objekt von der Methode zurückgegeben werden, für die nicht alle geplanten Aktionen ausgeführt wurden.
Schlussfolgerungen
Trotz der Tatsache, dass das Projekt von einem großen Unternehmen, Red Hat, entwickelt wird und die Qualität des Codes im Projekt auf einem hohen Niveau liegt, konnte die mit PVS-Studio durchgeführte statische Analyse eine bestimmte Anzahl von Fehlern aufdecken, die auf die eine oder andere Weise den Betrieb des Servers beeinträchtigen können. Und da WildFly für die Erstellung von Unternehmensanwendungen entwickelt wurde, können diese Fehler zu äußerst traurigen Konsequenzen führen.
Ich lade alle ein, PVS-Studio herunterzuladen und ihr Projekt damit zu überprüfen. Dazu können Sie eine Testlizenz anfordern oder einen der kostenlosen Anwendungsfälle verwenden.
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Dmitry Scherbakov. Überprüfen von WildFly, einem JavaEE-Anwendungsserver .