Bei der Weiterentwicklung des statischen Codeanalysators PVS-Studio müssen wir oft große Open-Source-Projekte von renommierten Entwicklern überprüfen. Die Tatsache, dass auch solche Projekte eine gewisse Menge an Fehlern enthalten, verleiht unserer Arbeit noch mehr Sinn und Gewicht. Leider macht jeder Fehler. Egal wie sorgfältig Sie die Qualität Ihres Codes kontrollieren, es gibt einfach keine Möglichkeit, „menschliches Versagen“ zu vermeiden. Solange Software von Menschen entwickelt wird, werden Analysetools wie PVS-Studio relevant und benötigt bleiben. Heute werden wir Fehler besprechen, die im Quellcode von MSBuild gefunden wurden, der leider auch nicht perfekt ist.
Einführung
Microsoft Build Engine (MSBuild) ist eine Plattform von Microsoft zum Erstellen von Anwendungen. Es wird normalerweise zusammen mit Microsoft Visual Studio verwendet, ist aber nicht davon abhängig. MSBuild stellt ein XML-Schema für Projektdateien (*.csproj, *.vbproj, *.vcxproj) bereit, das steuert, wie die Build-Plattform Software verarbeitet und erstellt. Das Tool wird als Teil der .NET-Plattform ausgeliefert und ist in C# geschrieben.
Microsoft stellt MSBuild-Quelldateien zum kostenlosen Hochladen auf GitHub bereit. Unter Berücksichtigung der hohen Entwicklungsstandards von Microsoft kann das Auffinden von Fehlern in MSBuild selbst für erstklassige statische Analyseprogramme eine schwierige Aufgabe sein. Aber Erfolg kommt mit Hartnäckigkeit. Mit Hilfe von PVS-Studio, Version 6.07, haben wir den Quellcode des MSBuild-Projekts überprüft und Folgendes gefunden.
Analysedaten und Statistiken
MSBuild besteht aus 14 Projekten, die insgesamt 1256 Quelldateien in C# enthalten. Das macht etwa 440.000 LOC.
PVS-Studio hat 262 Warnungen für dieses Projekt ausgegeben. Die allgemeine Analysestatistik mit Differenzierung der Warnungen nach Schweregraden ist in der folgenden Grafik dargestellt:
Wie Sie dem Diagramm entnehmen können, hat das Tool 73 Warnungen hoher Stufe, 107 Warnungen mittlerer Stufe und 82 Warnungen niedriger Stufe ausgegeben. Wir werden uns auf die hohen und mittleren Ebenen konzentrieren, da sie potenziell gefährliche Konstrukte und echte Fehler enthalten, während sich Warnungen auf niedriger Ebene, obwohl sie sich ebenfalls mit Fehlern befassen, oft als Fehlalarme herausstellen, sodass wir sie normalerweise nicht diskutieren unsere Artikel.
Die Analyse von MSBuild hat ergeben, dass etwa 45 % der Warnungen auf hoher und mittlerer Ebene auf falschen Code (81 Fehler) hinweisen, während sich die restlichen Warnungen einfach auf Konstrukte beziehen, die PVS-Studio für verdächtig hält, und eher auf Fehlalarme als auf echte Fehler . Einige der Warnungen wurden durch Komponententests oder Code mit Kommentaren zu potenziell gefährlichen Konstrukten ausgelöst, die zum Prüfen auf Ausnahmen verwendet wurden. In jedem Fall müssen die verbleibenden Warnungen von den Entwicklern geprüft werden, da sie die einzigen Personen sind, die den Code vollständig kennen und zuverlässig einschätzen können, ob die Warnungen korrekt sind oder nicht.
Basierend auf diesen Daten beträgt das PVS-Studio-Verhältnis von Fehlern auf hohem und mittlerem Niveau zu 1 KLOC (d. h. Fehlerdichte) nur 0,184 (Fehler pro 1 KLOC). Diese Zahl ist bei Microsoft-Projekten nicht verwunderlich und ein Zeichen für die hohe Qualität des MSBuild-Codes.
Lassen Sie uns nun die Analyseergebnisse im Detail besprechen. Beachten Sie auch, dass die Aufgabe, alle für dieses Projekt ausgegebenen Warnungen zu untersuchen, den Rahmen unseres Artikels sprengen würde, daher werden wir nur über die interessantesten Fehler sprechen und sie in Gruppen einteilen.
Analyseergebnisse
Falsche Nullprüfung
Diagnosemeldung von PVS-Studio :V3019 Möglicherweise wird eine falsche Variable nach der Typkonvertierung mit dem Schlüsselwort 'as' mit null verglichen. Überprüfen Sie die Variablen 'obj', 'name'. AssemblyRemapping.cs 64
Dies ist wahrscheinlich ein klassischer Fehler:Wir sehen ihn in fast jedem Projekt, das wir überprüfen. Es hat mit dem Umwandeln einer Variablen in einen anderen Typ unter Verwendung von as zu tun -Operator und testen dieselbe Variable anstelle der resultierenden auf null :
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (obj == null) // <=
{
return false;
}Stattdessen ist es der Name Variable, die geprüft werden soll:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (name == null)
{
return false;
}
Späte Nullprüfung
Diagnosemeldung von PVS-Studio :V3095 Das Objekt „diskRoots“ wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen:2656, 2659. ToolLocationHelper.cs 2656
Beachten Sie die diskRoots Parameter. Es ist eine Instanz der Liste Klasse und kann den Wert null haben . Die entsprechende Prüfung erfolgt jedoch erst im zweiten if Block, nach diskRoots Variable wurde bereits zum Einfügen von Werten in eine Liste verwendet:
private static void ExtractSdkDiskRootsFromEnvironment
(List<string> diskRoots, string directoryRoots)
{
if (!String.IsNullOrEmpty(directoryRoots))
{
....
diskRoots.AddRange(splitRoots); // <=
}
if (diskRoots != null) // <=
....
}Es gibt 8 weitere potenziell gefährliche Konstrukte wie dieses in MSBuild:
- V3095 Das Objekt 'propertyValue' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen:2760, 2799. Expander.cs 2760
- V3095 Das Objekt 'publicKeyToken' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen:232, 236. GenerateBindingRedirects.cs 232
- V3095 Das Objekt 'searchLocation' wurde verwendet, bevor es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen:170, 178. Resolver.cs 170
- V3095 Das Objekt 'assemblyName' wurde verwendet, bevor es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen:176, 194. Resolver.cs 176
- V3095 Das Objekt 'searchLocation' wurde verwendet, bevor es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen:249, 264. Resolver.cs 249
- V3095 Das 'ReferenceInfo'-Objekt wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen:87, 97. AxReference.cs 87
- V3095 Das Objekt 'packageFileName' wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen:1448, 1457. BootstrapperBuilder.cs 1448
- V3095 Das Objekt 'metadataNames' wurde verwendet, bevor es gegen Null verifiziert wurde. Überprüfen Sie die Zeilen:243, 253. CommandLineBuilderExtension.cs 243
Falsche Annahme über Stringlänge
Diagnosemeldung von PVS-Studio :V3057 Die Funktion 'Substring' könnte den Wert '-1' erhalten, obwohl ein nicht negativer Wert erwartet wird. Überprüfen Sie das zweite Argument. Dienstprogramme.cs 328
Für das wenn Block ausgeführt werden soll, muss es eine Zeichenfolge geben, die aus einem oder mehreren Zeichen besteht, während der Programmierer innerhalb dieses Blocks versucht, eine Teilzeichenfolge aus der ursprünglichen Zeichenfolge zu erhalten. Offensichtlich der zweite Parameter des Substring -Methode ist für eine Zeichenfolge mit einem Zeichen negativ, sodass die Methode eine ArgumentOutOfRangeException auslöst :
if (toolsVersionList.Length > 0)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}So könnte eine sichere Version dieses Codes aussehen:
if (toolsVersionList.Length > 1)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}Andere ähnliche Fehler:
- V3057 Die Funktion 'Substring' könnte den Wert '-1' erhalten, obwohl ein nicht negativer Wert erwartet wird. Überprüfen Sie das zweite Argument. Lösungsdatei.cs 1217
- V3057 Die Funktion 'Substring' könnte den Wert '-1' erhalten, obwohl ein nicht negativer Wert erwartet wird. Überprüfen Sie das zweite Argument. XMake.cs 2929
- V3057 Die Funktion 'Entfernen' könnte den Wert '-1' erhalten, obwohl ein nicht negativer Wert erwartet wird. Überprüfen Sie das erste Argument. BootstrapperBuilder.cs 767
Typumwandlung mit Bedeutungsverlust
Diagnosemeldung von PVS-Studio :V3041 Der Ausdruck wurde implizit vom Typ „long“ in den Typ „float“ umgewandelt. Erwägen Sie die Verwendung einer expliziten Typumwandlung, um den Verlust eines Bruchteils zu vermeiden. Ein Beispiel:double A =(double)(X) / Y;. CommunicationsUtilities.cs 593
Die Variablen jetzt und s_lastLoggedTicks sind lang Typ. Sie nehmen an einigen Berechnungen teil, die einen Wert von float ergeben sollten Typ. Da die Divisionsoperation jedoch über Werte vom Typ long erfolgt und erst dann wird der resultierende Wert in den Typ float umgewandelt , führt dies zu einem Genauigkeitsverlust:
float millisecondsSinceLastLog =
(float)((now - s_lastLoggedTicks)/10000L);Fester Code:
float millisecondsSinceLastLog =
(float)(now - s_lastLoggedTicks)/10000;Seien Sie immer vorsichtig bei Berechnungen, bei denen Integer- und Fließkommawerte zusammen verwendet werden.
Methode, die immer true zurückgibt
Diagnosemeldung von PVS-Studio :V3009 Es ist seltsam, dass diese Methode immer ein und denselben Wert von 'true' zurückgibt. ComReference.cs 304
Die GetTypeLibNameForITypeLib Methode gibt true zurück unabhängig von den Bedingungen:
internal static bool GetTypeLibNameForITypeLib(....)
{
....
if (typeLib2 == null)
{
....
return true; // <=
}
....
try
{
if (data == null || ...)
{
....
return true; // <=
}
....
}
catch (COMException ex)
{
....
return true; // <=
}
return true; // <=
}Gleichzeitig ist der Wert vom Typ bool zurückgegeben von GetTypeLibNameForITypeLib Methode wird im Aufrufer überprüft. Die Auswirkungen eines solchen Verhaltens sind nicht vorhersehbar. Dieser Code muss überarbeitet und korrigiert werden.
Sinnloser Vergleich
Diagnosemeldung von PVS-Studio :V3022 Ausdruck 'itemsAndMetadataFound.Metadata.Values.Count> 0' ist immer wahr. Evaluator.cs 1752
Nach itemsAndMetadataFound.Metadata.Values.Count> 0 Ausdruck wird im externen if ausgewertet Block wird die gleiche Prüfung, diesmal sinnlos, innerhalb durchgeführt dieser Block:
if (itemsAndMetadataFound.Metadata != null &&
itemsAndMetadataFound.Metadata.Values.Count > 0)
{
....
if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <=
{
needToProcessItemsIndividually = true;
}
....
}Darüber hinaus enthält MSBuild 7 weitere Probleme dieser Art:
- V3022 Ausdruck 'fixedPathInfo !=null' ist immer wahr. FrameworkLocationHelper.cs 794
- V3022 Ausdruck '_shutdownException !=null' ist immer falsch. InProcNode.cs 527
- V3022 Ausdruck 'proj !=null' ist immer wahr. Lösungsdatei.cs 817
- V3022 Ausdruck '_directMetadata ==null' ist immer falsch. ProjectItem.cs 755
- V3022 Ausdruck 'Constants.defaultToolsVersion =="2.0"' ist immer wahr. ToolsetReader.cs 194
- V3022 Ausdruck '!isQuotedTransform &&functionCapture ==null' ist immer wahr. ExpressionShredder.cs 281
- V3022 Ausdruck '!isQuotedTransform &&functionCapture ==null' ist immer wahr. ExpressionShredder.cs 414
Sich gegenseitig ausschließende Vergleiche
Diagnosemeldung von PVS-Studio :V3011 Es wurden zwei gegensätzliche Bedingungen festgestellt. Die zweite Bedingung ist immer falsch. Überprüfen Sie die Zeilen:2840, 2838. XMake.cs 2840
Für das wenn Ausführungsblock, der Logger Variable muss Null haben Wert. Diese Variable wird jedoch erneut auf null getestet innerhalb dieses Blocks, im VerifyThrow Methode. Diese zweite Prüfung wird also immer falsch sein:
if (logger == null)
{
InitializationException.VerifyThrow(logger != null, // <=
"LoggerNotFoundError", unquotedParameter);
}Ich bin mir nicht sicher, wie dieser Code tatsächlich aussehen sollte, aber sicherlich nicht so. Vielleicht das wenn Operator ist überhaupt nicht erforderlich.
Nicht verwendete Argumente in String-Formatierungsmethoden
Diagnosemeldung von PVS-Studio :V3025 Falsches Format. Beim Aufruf der Funktion „WriteLine“ wird eine andere Anzahl von Formatelementen erwartet. Nicht verwendete Argumente:1. Scheduler.cs 2216
Der Fehler lauert in der zweiten Zeile. Der Programmierer muss es geschrieben haben, indem er die erste Zeile (das berüchtigte Kopieren und Einfügen) kopiert und den ersten Parameter im kopierten Code geändert hat, aber er hat vergessen, den zweiten Parameter _schedulingData.EventTime.Ticks zu entfernen , was nicht mehr nötig war:
file.WriteLine("Scheduler state at timestamp {0}:",
_schedulingData.EventTime.Ticks);
file.WriteLine("------------------------------------------------",
_schedulingData.EventTime.Ticks); // <=Also die Methode WriteLine(string format, object arg0) in der zweiten Zeile falsch überschrieben.
Andere ähnliche Defekte:
- V3025 Falsches Format. Beim Aufruf der Funktion „Format“ wird eine andere Anzahl von Formatelementen erwartet. Nicht verwendete Argumente:Ressource. XmlUtil.cs 75
- V3025 Falsches Format. Beim Aufruf der Funktion „Format“ wird eine andere Anzahl von Formatelementen erwartet. Nicht verwendete Argumente:Ressource. XmlUtil.cs 82
- V3025 Falsches Format. Beim Aufruf der Funktion „Format“ wird eine andere Anzahl von Formatelementen erwartet. Nicht verwendete Argumente:Ressource. XmlUtil.cs 91
- V3025 Falsches Format. Beim Aufruf der Funktion „Format“ wird eine andere Anzahl von Formatelementen erwartet. Nicht verwendete Argumente:Ressource. XmlUtil.cs 112
Unbenutzter Parameter
Diagnosemeldung von PVS-Studio :V3061 Parameter 'numericValue' wird vor der Verwendung immer neu in den Methodenkörper geschrieben. NodePacketTranslator.cs 320
Die Liste der formalen Parameter der Methode enthält die Variable numericValue dessen Wert nie verwendet wird, da er sofort durch einen neuen Wert ersetzt wird:
public void TranslateEnum<T>(ref T value, int numericValue)
{
numericValue = _reader.ReadInt32(); // <=
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Vielleicht hat der Programmierer etwas umgestaltet, aber es war nicht möglich, die Signatur der Methode (im Gegensatz zu ihrem Hauptteil) zu ändern. Andernfalls ist es besser, die Methode zu korrigieren:
public void TranslateEnum<T>(ref T value)
{
int numericValue = _reader.ReadInt32();
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Eine weitere ähnliche Warnung:
- V3061 Der Parameter 'defaultToolsVersion' wird immer neu in den Methodenkörper geschrieben, bevor er verwendet wird. ToolsetProvider.cs 118
Redundante Zuweisung
Diagnosemeldung von PVS-Studio :V3005 Die Variable '_nextProjectId' wird sich selbst zugewiesen. LoggingService.cs 325
Der Analysator hat ein Konstrukt mit einer zusätzlichen Zuweisung zum Feld _nextProjectId erkannt . Das Ergebnis von MaxCPUCount + 2 Ausdruck wird zum Wert von _nextProjectId hinzugefügt , und dann wird der resultierende Wert demselben Feld mit += zugewiesen Operator. Danach wird dieser Wert wieder der _nextProjectId zugewiesen Feld:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId = _nextProjectId += MaxCPUCount + 2; // <=
return _nextProjectId;
}
}
}Dieser Code enthält keinen Fehler, sieht aber seltsam aus. Das Konstrukt sollte vereinfacht werden:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId += MaxCPUCount + 2;
return _nextProjectId;
}
}
}Schlussfolgerung
Abschließend möchte ich sagen, dass selbst so hochwertige Projekte wie MSBuild von regelmäßigen Überprüfungen ihres Quellcodes auf potenzielle und tatsächliche Fehler durch statische Analyseprogramme wie PVS-Studio sehr profitieren könnten.
Fühlen Sie sich frei, die Demoversion des PVS-Studio-Analyzers zu verwenden, um dieses Projekt zu überprüfen und einen Blick auf die von uns besprochenen Warnungen zu werfen, sowie um Ihre eigenen Projekte zu überprüfen.