Dieser Artikel ist das Ergebnis einer erneuten Überprüfung des Orchard-Projekts mit dem statischen Analysator PVS-Studio. Orchard ist ein Open Source-Content-Management-System, das Teil der Outercurve Foundation ist, einer gemeinnützigen ASP.NET-Projektgalerie. Der Test ist insofern interessant, als das Projekt und der Analysator seit der ersten Analyse erheblich gewachsen sind. Neue positive und interessante Bugs warten auf Sie.
Über Orchard CMSWir haben Orchard vor drei Jahren mit PVS-Studio
getestet . Seitdem hat sich der C # -Analysator stark weiterentwickelt: Wir haben die Datenflussanalyse verbessert, die interprozedurale Analyse entwickelt, neue Diagnosen hinzugefügt und eine Reihe von Fehlalarmen behoben. Darüber hinaus ergab die Analyse, dass alle Kommentare aus dem vorherigen Artikel korrigiert wurden. Damit ist das Ziel erreicht - der Code ist besser geworden.
Ich möchte glauben, dass die Entwickler diesem Artikel Aufmerksamkeit schenken und die notwendigen Änderungen vornehmen werden. Noch besser, wenn Sie die Praxis der regelmäßigen Verwendung von PVS-Studio einführen. Ich möchte Sie daran erinnern, dass wir für
Open-Source- Projekte eine kostenlose Version der Lizenz anbieten. Übrigens gibt es andere
Optionen , die für geschlossene Projekte geeignet sind.
Der Orchard-Projektcode kann wie ich
von hier heruntergeladen
werden . Eine vollständige Beschreibung des Projekts finden Sie
hier . Wenn Sie unseren PVS-Studio-Analysator noch nicht haben, können Sie
hier eine Testversion herunterladen. Ich habe die Version von PVS-Studio Version 7.05 Beta verwendet. Ich werde die Ergebnisse ihrer Arbeit teilen. Ich hoffe, Sie stimmen der Nützlichkeit der Verwendung von PVS-Studio zu. Die Hauptsache ist, den Analysator
regelmäßig zu benutzen.
ValidierungsergebnisseIch gebe Ihnen einige Zahlen aus dem letzten Artikel, damit Sie nicht zum Vergleichen wechseln müssen.
„Alle Dateien (3739 Teile) mit der Erweiterung .cs haben an der letzten Prüfung teilgenommen. Insgesamt enthielten sie 214.564 Codezeilen. Basierend auf den Prüfungsergebnissen gingen 137 Warnungen ein. Beim ersten (hohen) Konfidenzniveau gingen 39 Warnungen ein. Auf der zweiten Ebene (Durchschnitt) gingen 60 Warnungen ein. “
Im Moment hat das Projekt 2767 .cs-Dateien, das heißt, das Projekt ist weniger als tausend Dateien geworden. Gemessen an dieser Verringerung der Anzahl der Dateien und der Änderung des Repository-Namens wurde ein Kernel aus dem Projekt zugewiesen (
Commit 966 ). Es gibt 108.287 Codezeilen im Kernel und der Analysator gab 153 Warnungen aus, 33 auf hohem Niveau, 70 im Durchschnitt. Wir berücksichtigen normalerweise keine Warnungen der dritten Ebene, ich habe die Tradition nicht gebrochen.
PVS-Studio Warnung :
V3110 Mögliche unendliche Rekursion innerhalb der 'TryValidateModel'-Methode. PrefixedModuleUpdater.cs 48
public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }
Wie im vorherigen Artikel öffnen wir die Fehlerliste mit unendlicher Rekursion. Es ist schwer zu sagen, was genau der Entwickler in diesem Fall tun wollte. Ich habe jedoch festgestellt, dass die
TryValidateModel- Methode eine einzige Argumentüberladung aufweist, die folgendermaßen aussieht:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Ich
gehe davon aus, dass der Entwickler wie bei der überladenen Methode Methoden über
_updateModel aufrufen
wollte. Der Compiler hat keinen Fehler gesehen,
_updateModel ist vom Typ
IUpdateModel und die aktuelle Klasse implementiert auch diese Schnittstelle. Da die Methode keine einzige Bedingung zum Schutz vor einer
StackOverflowException enthält , wurde die Methode möglicherweise nicht einmal aufgerufen, aber ich würde nicht darauf zählen. Wenn die Annahme richtig ist, sieht die korrigierte Version folgendermaßen aus:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
PVS-Studio Warnung: V3008 Der Variablen 'content' werden zweimal hintereinander Werte zugewiesen. Vielleicht ist das ein Fehler. Überprüfen Sie die Zeilen: 197, 190. DynamicCacheTagHelper.cs 197
public override async Task ProcessAsync(....) { .... IHtmlContent content; .... try { content = await output.GetChildContentAsync(); } finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); .... }
Der Analysator sah zwei Zuordnungen zum
Inhalt der lokalen Variablen
. GetChildContentAsync ist eine Methode aus der Bibliothek, die nicht häufig genug ist, um eine Entscheidung zu treffen und sie zu kommentieren. Leider wissen weder wir noch der Analysator etwas über das zurückgegebene Objekt der Methode und die Nebenwirkungen. Aber wir können definitiv sagen, dass das Zuweisen eines Ergebnisses in
Inhalten ohne Verwendung keinen Sinn ergibt. Vielleicht ist dies überhaupt kein Fehler, sondern nur eine zusätzliche Operation. Ich konnte nicht zu einem eindeutigen Schluss kommen, wie ich das Problem beheben könnte. Ich werde diese Entscheidung den Entwicklern überlassen.
PVS-Studio Warnung :
V3080 Mögliche Null-Dereferenzierung. Überprüfen Sie 'itemTag'. CoreShapes.cs 92
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....; .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
In diesem Code hat der Analysator einen gefährlichen Dereferenzierungsgegenstand
festgestellt . Ein gutes Beispiel für den Unterschied zwischen einem statischen Analysegerät und einer Testperson. Die Methode hat einen Parameter namens
ItemTag und eine lokale Variable namens
itemTag . Wie Sie wissen, ist der Unterschied für den Compiler enorm! Dies sind zwei verschiedene, wenn auch verwandte Variablen. Die Verbindung erfolgt über die dritte Variable,
itemTagName. Das Szenario zum
Auslösen einer Ausnahme lautet wie folgt: Das
ItemTag- Argument lautet "-", dem
itemTagName wird kein Wert zugewiesen, es bleibt eine
Nullreferenz , und wenn es sich um eine
Nullreferenz handelt, wird das lokale
itemTag auch zu einer
Nullreferenz . Ich denke, es ist besser, hier eine Ausnahme auszulösen, nachdem Sie die Zeichenfolge überprüft haben.
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = ....; if(String.IsNullOrEmpty(itemTag)) throw .... .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
PVS-Studio Warnung: V3095 Das 'remoteClient'-Objekt wurde verwendet, bevor es gegen null verifiziert wurde. Überprüfen Sie die Zeilen: 49, 51. ImportRemoteInstanceController.cs 49
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey)); if (remoteClient == null || ....) { .... } .... }
Der Analysator sah den Dereferenzierungs-
RemoteClient und prüfte unten auf
Null . Dies ist wirklich eine potenzielle
NullReferenceException , da die
FirstOrDefault- Methode den Standardwert zurückgeben kann (
null für Referenztypen). Ich nehme an, um den Code zu korrigieren, übertragen Sie einfach den obigen Scheck:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Es kann sich jedoch lohnen,
FirstOrDefault durch
First zu ersetzen und die Prüfung vollständig zu entfernen.
Von PVS-Studio 7.05 Beta:Im Moment haben wir alle
orDefault- Methoden von
LINQ kommentiert. Diese Informationen werden von neuen Diagnosen verwendet, wobei die Dereferenzierung des Ergebnisses des Aufrufs dieser Methoden ohne Überprüfung festgestellt wird. Für jede
orDefault- Methode gibt es ein Analogon, das eine Ausnahme
auslöst , wenn kein geeignetes Element gefunden wurde. Diese Ausnahme hilft mehr beim Verständnis des Problems als die abstrakte
NullReferenceException .
Ich kann nur das Ergebnis der entwickelten Diagnose für dieses Projekt teilen. 27 potenziell gefährliche Orte. Hier sind einige davon:
ContentTypesAdminNodeNavigationBuilder.cs 71:
var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault(); await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders);
ListPartDisplayDriver.cs 217:
var contentTypePartDefinition = ....Parts.FirstOrDefault(....); return contentTypePartDefinition.Settings....;
ContentTypesAdminNodeNavigationBuilder.cs 113:
var typeEntry = node.ContentTypes.Where(....).FirstOrDefault(); return AddPrefixToClasses(typeEntry.IconClass);
PVS-Studio Warnung :
V3080 Mögliche Null-Dereferenzierung des Methodenrückgabewerts. Betrachten Sie Folgendes: CreateScope (). SetupService.cs 136
public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... }
Daher hat der Analysator die Dereferenzierung des Ergebnisses des Aufrufs der
CreateScope- Methode
festgestellt . Die
CreateScope- Methode
ist sehr klein, ich werde sie in ihrer Gesamtheit geben:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Wie Sie sehen können, kann es in zwei Fällen
null zurückgeben . Der Analysator kann nicht sagen, welchen Zweig das Programm durchlaufen wird, daher markiert er den Code als verdächtig. In meinem Code würde ich sofort eine
Nullprüfung hinzufügen.
Vielleicht
urteile ich voreingenommen, aber alle asynchronen Methoden sollten so gut wie möglich gegen
NullReferenceException versichert sein. Das Debuggen solcher Dinge ist ein sehr zweifelhaftes Vergnügen.
In diesem Fall hat die
CreateScope- Methode vier Aufrufe, und in zwei gibt es eine Prüfung, in den anderen beiden fehlt sie. Darüber hinaus ähnelt ein Paar ohne Überprüfung dem Kopieren und Einfügen (eine Klasse, eine Methode, die für den Aufruf von UsingAsync dereferenziert wurde). Ich habe bereits den ersten Anruf von diesem Paar getätigt, und natürlich hat der zweite Anruf auch eine ähnliche Analysatorwarnung:
V3080 Mögliche Null-Dereferenzierung des Methodenrückgabewerts. Betrachten Sie Folgendes: CreateScope (). SetupService.cs 192
PVS-Studio Warnung: V3127 Es wurden zwei ähnliche Codefragmente gefunden. Möglicherweise ist dies ein Tippfehler, und die Variable 'AccessTokenSecret' sollte anstelle von 'ConsumerSecret' TwitterClientMessageHandler.cs 52 verwendet werden
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
Klassisches Kopieren und Einfügen.
ConsumerSecret wurde zweimal und
AccessTokenSecret zweimal überprüft. Offensichtlich müssen Sie Folgendes beheben:
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
PVS-Studio Warnung: V3139 Zwei oder mehr
Fallzweige führen dieselben Aktionen aus. SerialDocumentExecuter.cs 23
Ein weiterer Fehler beim Kopieren und Einfügen. Um es klarer zu machen, werde ich die ganze Klasse geben, da sie klein ist.
public class SerialDocumentExecuter : DocumentExecuter { private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy(); private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy(); protected override IExecutionStrategy SelectExecutionStrategy(....) { switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } } }
Der Analysator hielt zwei identische Fallzweige für verdächtig. In der Tat gibt es drei Entitäten in der Klasse, zwei kehren zurück, eine nicht. Wenn dies geplant ist und die dritte Option aufgegeben wird, können Sie sie löschen, indem Sie die beiden Zweige wie folgt kombinieren:
switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Wenn dies ein Fehler beim Kopieren und Einfügen ist, müssen Sie das zurückgegebene Feld wie folgt korrigieren:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Oder umgekehrt. Ich bin mit dem Projekt nicht vertraut und kann die Namen der Arten von Operationen und Strategien nicht korrelieren.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
PVS-Studio Warnung :
V3080 Mögliche Null-Dereferenzierung. Betrachten Sie die Überprüfung "Anfrage". GraphQLMiddleware.cs 148
private async Task ExecuteAsync(HttpContext context....) { .... GraphQLRequest request = null; .... if (HttpMethods.IsPost(context.Request.Method)) { .... } else if (HttpMethods.IsGet(context.Request.Method)) { .... request = new GraphQLRequest(); .... } var queryToExecute = request.Query; .... }
Im ersten
if- Block erhält die
Anforderungsvariable an mehreren Stellen einen anderen Wert als
null , jedoch überall mit verschachtelten Bedingungen. Ich habe nicht alle diese Bedingungen angeführt, da das Beispiel zu umständlich gewesen wäre. Die ersten Bedingungen, die den http-Typ der
IsGet- oder
IsPost- Methode
überprüfen ,
sind ausreichend . Die
Microsoft.AspNetCore.Http.HttpMethods- Klasse verfügt über neun statische Methoden zum Überprüfen des Anforderungstyps. Dies bedeutet, dass eine
NullReferenceException ausgelöst wird, wenn beispielsweise eine
Delete- oder
Set- Anforderung in die
ExecuteAsync- Methode
eingeht . Auch wenn solche Methoden derzeit überhaupt nicht unterstützt werden, ist es mit einer Ausnahme besser, eine Prüfung durchzuführen. Schließlich können sich die Anforderungen an das System ändern. Überprüfungsbeispiel unten:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
PVS-Studio Warnung :
V3080 Mögliche Null-Dereferenzierung des Methodenrückgabewerts. Betrachten Sie Folgendes: Holen Sie sich <ContentPart> (...). ContentPartHandlerCoordinator.cs 190
Die meisten
V3080- Diagnosetrigger
sind in der Entwicklungsumgebung leichter zu erkennen. Benötigen Sie Methodennavigation, Typhervorhebung, eine ermutigende IDE-Atmosphäre. Ich versuche, die Beispiele so kurz wie möglich zu halten, damit Sie besser lesen können. Aber wenn es für mich nicht funktioniert oder wenn Sie Ihre Programmierebene überprüfen und selbst herausfinden möchten, empfehle ich Ihnen zu sehen, wie diese Diagnose bei einem offenen oder Ihrem eigenen Projekt funktioniert.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
Der Analysator schwört auf diese Linie. Schauen wir uns die
Get- Methode an
: public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Es verursacht seine Überlastung. Schauen wir sie uns an:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
Es stellt sich heraus, dass die
Get- Methode
null zurückgibt , wenn beim Empfang eines Elements von
Data durch den
Namensindexer eine Entität eines mit
JObject nicht kompatiblen Typs erhalten wird. Ich werde es nicht wagen, die Wahrscheinlichkeit dafür zu beurteilen, da diese Typen aus der
Newtonsoft.Json- Bibliothek stammen und ich ein wenig Erfahrung damit habe. Der Entwickler vermutete jedoch, dass der gewünschte Artikel möglicherweise nicht vorhanden ist. Vergessen Sie diese Möglichkeit also nicht, wenn Sie sich auf die Leseergebnisse beziehen. Ich würde dem allerersten
Get einen Ausnahmefall hinzufügen, wenn wir der Meinung sind, dass der Knoten sein sollte, oder vor der Dereferenzierung prüfen, ob das Fehlen des Knotens die allgemeine Logik nicht ändert (der Standardwert wird beispielsweise verwendet).
Option eins:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Option zwei:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
PVS-Studio Warnung :
V3080 Mögliche Null-Dereferenzierung. Betrachten Sie die Überprüfung der "Ergebnisse". ContentQueryOrchardRazorHelperExtensions.cs 19
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... }
Ein ziemlich einfaches Beispiel im Vergleich zum vorherigen. Der Analysator ist der Ansicht, dass das Ergebnis des Aufrufs von
QueryAsync eine
Nullreferenz sein kann. Hier ist die Methode:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Hier ist
GetQueryAsync eine Schnittstellenmethode, bei der Sie nicht bei jeder Implementierung sicher sein können. Darüber hinaus gibt es im selben Projekt eine solche Option:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
Die Analyse der
GetDocumentAsync- Methode wird durch die Vielzahl von Aufrufen externer Funktionen und Cache-Zugriffen erschwert. Lassen Sie uns auf die Tatsache eingehen, dass es sich lohnt, einen Scheck hinzuzufügen. Darüber hinaus ist das Verfahren asynchron.
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... }
FazitIch kann nicht anders, als die hohe Qualität des Codes zu beachten! Ja, es gab andere Mängel, die ich in diesem Artikel nicht angesprochen habe, aber die schwerwiegendsten wurden dennoch berücksichtigt. Dies bedeutet natürlich nicht, dass eine Überprüfung alle drei Jahre ausreicht. Der maximale Nutzen wird durch die
regelmäßige Verwendung eines statischen Analysators erzielt, da Sie mit diesem Ansatz Probleme frühestens erkennen und beheben können, wenn die Kosten und die Komplexität der Bearbeitung minimal sind.
Obwohl einmalige Überprüfungen nicht so effektiv wie möglich sind, fordere ich Sie dringend auf,
PVS-Studio für Ihr Projekt herunterzuladen und auszuprobieren - was ist, wenn Sie etwas Interessantes finden?
Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Alexander Senichkin.
Scannen des Codes von Orchard CMS auf Fehler .