In diesem Artikel werden die Ergebnisse einer zweiten Überprüfung des Orchard-Projekts mit dem statischen Analysegerät PVS-Studio überprüft. Orchard ist ein Open-Source-Content-Manager-System, das als Teil der ASP.NET Open Source-Galerie unter der gemeinnützigen Outercurve Foundation bereitgestellt wird. Die heutige Überprüfung ist besonders interessant, da sowohl das Projekt als auch der Analysator seit der ersten Überprüfung einen langen Weg zurückgelegt haben. Dieses Mal werden wir uns neue Diagnosemeldungen und einige nette Fehler ansehen.
Über Orchard CMSWir haben Orchard vor drei Jahren
überprüft . Der C # -Analysator von PVS-Studio hat sich seitdem stark weiterentwickelt: Wir haben die Datenflussanalyse verbessert, eine interprozedurale Analyse und neue Diagnosen hinzugefügt und eine Reihe von Fehlalarmen behoben. Darüber hinaus ergab die zweite Überprüfung, dass die Entwickler von Orchard alle im ersten Artikel gemeldeten Fehler behoben hatten, was bedeutet, dass wir unser Ziel erreicht hatten, d. H., Dass sie ihren Code verbessert hatten.
Ich hoffe, dass sie auch auf diesen Artikel achten und die notwendigen Korrekturen vornehmen oder, noch besser, PVS-Studio für die regelmäßige Verwendung übernehmen. Zur Erinnerung, wir stellen
Open-Source- Entwicklern eine kostenlose Lizenz zur Verfügung. Übrigens gibt es auch andere
Optionen , die proprietäre Projekte genießen können.
Der Quellcode von Orchard steht hier zum Download zur Verfügung. Die vollständige Projektbeschreibung finden Sie
hier . Wenn Sie noch keine PVS-Studio-Kopie haben, können Sie die Testversion
herunterladen . Ich habe PVS-Studio 7.05 Beta verwendet und werde einige seiner Warnungen in diesen Artikel aufnehmen. Ich hoffe, diese Bewertung wird Sie davon überzeugen, dass PVS-Studio ein nützliches Werkzeug ist. Denken Sie daran, dass es
regelmäßig verwendet
werden soll .
AnalyseergebnisseHier sind einige der Zahlen aus der ersten Überprüfung von Orchard, damit Sie nicht zum Vergleich zwischen den beiden Artikeln wechseln müssen.
Bei der vorherigen Überprüfung "haben wir alle Quellcodedateien (3739 Elemente) mit der Erweiterung .cs analysiert. Insgesamt gab es 214.564 Codezeilen. Das Ergebnis der Überprüfung waren 137 Warnungen. Genauer gesagt gab es 39 erste (hohe) Warnungen. Es gab auch 60 Sekunden (mittlere) Warnungen. “
Die aktuelle Version von Orchard besteht aus 2.767 CS-Dateien, d. H. Es sind ungefähr tausend Dateien kleiner. Die Verkleinerung und Umbenennung des Repositorys lässt darauf schließen, dass die Entwickler den Kern des Projekts (
Commit 966 ) isoliert haben, der 108.287 LOC lang ist. Der Analysator gab 153 Warnungen aus: 33 Warnungen der ersten und 70 Warnungen der zweiten Ebene. Wir enthalten normalerweise keine Warnungen der dritten Ebene, und ich werde mich an die Tradition halten.
PVS-Studio-Diagnosemeldung: V3110 Mögliche unendliche Rekursion innerhalb der Methode 'TryValidateModel'. PrefixedModuleUpdater.cs 48
public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }
Beginnen wir mit einem unendlichen Rekursionsfehler, wie wir es im ersten Artikel getan haben. Diesmal sind die genauen Absichten des Entwicklers nicht klar, aber ich habe festgestellt, dass die
TryValidateModel- Methode eine überladene Version mit einem Parameter hatte:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Ich denke, dass der Entwickler genau wie bei der überladenen Version beabsichtigte, die Methode über
_updateModel aufzurufen. Der Compiler hat den Fehler nicht bemerkt.
_updateModel ist vom Typ
IUpdateModel , und die aktuelle Klasse implementiert auch diese Schnittstelle. Da die Methode keine Prüfung gegen
StackOverflowException enthält , wurde sie wahrscheinlich nie aufgerufen, obwohl ich nicht damit rechnen würde. Wenn meine Annahme richtig ist, sollte die feste Version folgendermaßen aussehen:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
PVS-Studio-Diagnosemeldung: 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 hat zwei Zuordnungen zum
Inhalt der lokalen Variablen festgestellt
. GetChildContentAsync ist eine Bibliotheksmethode, die zu selten verwendet wird, als dass wir uns die Mühe machen
könnten , sie zu untersuchen und mit Anmerkungen zu versehen. Ich fürchte, weder wir noch der Analysator wissen etwas über das Rückgabeobjekt und die Nebenwirkungen der Methode. Wir wissen jedoch mit Sicherheit, dass die Zuweisung des Rückgabewerts zum
Inhalt keinen Sinn ergibt, wenn er nicht weiter im Code verwendet wird. Vielleicht ist es eher eine redundante Operation als ein Fehler. Ich kann nicht sagen, wie genau dies behoben werden soll, also überlasse ich es den Entwicklern.
PVS-Studio-Diagnosemeldung: 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; }
Der Analysator hat eine unsichere Dereferenzierung von
itemTag festgestellt . Dieses Snippet ist ein gutes Beispiel dafür, wie sich ein statisches Analysetool von einem menschlichen Entwickler unterscheidet, der Codeüberprüfungen durchführt. Die Methode hat einen Parameter namens
ItemTag und eine lokale Variable namens
itemTag . Sie müssen Ihnen nicht sagen, dass dies einen großen Unterschied für den Compiler darstellt! Dies sind zwei verschiedene, obwohl verwandte Variablen. Die Art und Weise, wie sie zusammenhängen, erfolgt über eine dritte Variable,
itemTagName. Hier ist die Reihenfolge der Schritte, die zur möglichen Ausnahme führen: Wenn das
ItemTag- Argument gleich "-" ist, wird
itemTagName kein Wert zugewiesen, sodass es eine
Nullreferenz bleibt. Wenn es sich um eine
Nullreferenz handelt , wird die lokale Variable
itemTag verwendet wird auch zu einer Nullreferenz. Meiner Meinung nach ist es besser, nach der Zeichenfolgenprüfung eine Ausnahme auszulösen.
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-Diagnosemeldung: V3095 Das Objekt 'remoteClient' 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 hat eine Dereferenzierung von
remoteClient festgestellt, gefolgt von einer
Nullprüfung einige Zeilen später. Dies ist in der Tat eine potenzielle
NullReferenceException, da die
FirstOrDefault- Methode möglicherweise einen Standardwert
zurückgibt (der für Referenztypen
null ist). Ich denke, dieses Snippet kann repariert werden, indem einfach die Überprüfung so verschoben wird, dass sie der Dereferenzierungsoperation vorausgeht:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Oder vielleicht sollte es behoben werden, indem
FirstOrDefault durch
First ersetzt und die Prüfung insgesamt entfernt wird.
Warnungen von PVS-Studio 7.05 Beta:Inzwischen haben wir alle
orDefault- Methoden von
LINQ mit Anmerkungen versehen. Diese Informationen werden von der neuen Diagnose verwendet, an der wir arbeiten: Sie erkennt Fälle, in denen die von diesen Methoden zurückgegebenen Werte ohne vorherige Überprüfung dereferenziert werden. Jede
orDefault- Methode verfügt über ein Gegenstück, das eine Ausnahme
auslöst , wenn kein übereinstimmendes Element gefunden wurde. Diese Ausnahme ist beim Aufspüren des Problems hilfreicher als die abstrakte
NullReferenceException .
Ich kann nur die Ergebnisse teilen, die ich aus dieser Diagnose für das Orchard-Projekt erhalten habe. Es gibt 27 potenziell gefährliche Stellen. 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-Diagnosemeldung: 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(....); } .... }
Der Analysator erwähnte eine Dereferenzierung des von der
CreateScope- Methode zurückgegebenen
Werts .
CreateScope ist eine winzige Methode. Hier ist die vollständige Implementierung:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Wie Sie sehen können, gibt es zwei Fälle, in denen
null zurückgegeben werden kann . Der Analysator weiß nicht, welchem Zweig der Ausführungsfluss folgen wird, geht also auf Nummer sicher und meldet den Code als verdächtig. Wenn ich so einen Code schreiben würde, würde ich sofort einen Null-Check schreiben.
Vielleicht ist meine Meinung voreingenommen, aber ich glaube, dass jede asynchrone Methode so weit wie möglich vor
NullReferenceException geschützt werden sollte, da das Debuggen
solcher Dinge alles andere als unterhaltsam ist.
In diesem speziellen Fall wird die
CreateScope- Methode viermal aufgerufen: Zwei dieser Aufrufe werden von Überprüfungen begleitet, die anderen beiden nicht. Die beiden letztgenannten Aufrufe (ohne Prüfung) scheinen Copy-Paste-Klone zu sein (dieselbe Klasse, dieselbe Methode, dieselbe Methode zum Dereferenzieren des Ergebnisses für den Aufruf von UsingAsync). Der erste dieser beiden Anrufe wurde oben angezeigt, und Sie können sicher sein, dass der zweite dieselbe Warnung ausgelöst hat:
V3080 Mögliche Null-Dereferenzierung des Methodenrückgabewerts. Betrachten Sie Folgendes: CreateScope (). SetupService.cs 192
PVS-Studio-Diagnosemeldung: 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); .... }
Das ist ein klassischer Fehler beim Kopieren und Einfügen.
ConsumerSecret wurde zweimal überprüft, während
AccessTokenSecret überhaupt nicht überprüft wurde. Offensichtlich ist dies wie folgt behoben:
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-Diagnosemeldung: V3139 Zwei oder mehr
Fallzweige führen dieselben Aktionen aus. SerialDocumentExecuter.cs 23
Ein weiterer Fehler beim Kopieren und Einfügen. Zur Verdeutlichung finden Sie hier die vollständige Klassenimplementierung (klein).
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 ....; } } }
Dem Analysator gefielen die beiden identischen Fallzweige nicht. In der Tat hat die Klasse drei Entitäten, während die switch-Anweisung nur zwei davon zurückgibt. Wenn dieses Verhalten beabsichtigt ist und die dritte Entität eigentlich nicht verwendet werden soll, kann der Code verbessert werden, indem der dritte Zweig entfernt wird, der die beiden wie folgt zusammengeführt hat:
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, sollte das erste der doppelten Rückgabefelder wie folgt behoben werden:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Oder es sollte der zweite Fallzweig sein. Ich kenne die Projektdetails nicht und kann daher die Korrelation zwischen den Namen der Operationstypen und Strategien nicht bestimmen.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
PVS-Studio-Diagnosemeldung: 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; .... }
Der
Anforderungsvariablen wird im ersten
if- Block mehrmals ein anderer Wert als
null zugewiesen, jedoch jedes Mal mit verschachtelten Bedingungen. Das Einbeziehen all dieser Bedingungen würde das Beispiel zu lang machen, daher werden wir nur mit den ersten
fortfahren , die den Typ der http-Methode
IsGet oder
IsPost überprüfen . Die
Microsoft.AspNetCore.Http.HttpMethods- Klasse verfügt über neun statische Methoden zum Überprüfen des Abfragetyps. Wenn Sie beispielsweise eine
Delete- oder
Set- Abfrage an die
ExecuteAsync- Methode übergeben, wird eine
NullReferenceException ausgelöst . Selbst wenn solche Methoden derzeit überhaupt nicht unterstützt werden, ist es dennoch ratsam, eine Ausnahmeprüfung hinzuzufügen. Schließlich können sich die Systemanforderungen ändern. Hier ist ein Beispiel für eine solche Prüfung:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
PVS-Studio-Diagnosemeldung: V3080 Mögliche Null-Dereferenzierung des Methodenrückgabewerts. Betrachten Sie Folgendes: Holen Sie sich <ContentPart> (...). ContentPartHandlerCoordinator.cs 190
Die meisten
V3080- Warnungen lassen sich bequemer in der Entwicklungsumgebung anzeigen, da Sie die Methodennavigation, die Hervorhebung von Typen und die freundliche Atmosphäre der IDE benötigen. Ich versuche, den Text von Beispielen so weit wie möglich zu reduzieren, um sie lesbar zu halten. Aber wenn ich es nicht richtig mache oder wenn Sie Ihre Programmierkenntnisse testen und alles selbst herausfinden möchten, empfehle ich, das Ergebnis dieser Diagnose in einem Open-Source-Projekt oder nur in Ihrem eigenen Code zu überprüfen.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
Der Analysator meldet diese Zeile. Werfen wir einen Blick auf die
Get- Methode:
public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Es ruft seine überladene Version auf. Lassen Sie es uns auch überprüfen:
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 wir mit dem
Namensindexer eine Entität eines mit
JObject nicht kompatiblen
Typs von
Data erhalten . Ich weiß nicht genau, wie wahrscheinlich das ist, da diese Typen aus der
Newtonsoft.Json- Bibliothek stammen, mit der ich nicht viel gearbeitet habe. Der Autor des Codes vermutete jedoch, dass das gesuchte Element möglicherweise nicht vorhanden ist. Daher sollten wir dies berücksichtigen, wenn wir auch auf das Ergebnis der Leseoperation zugreifen. Persönlich würde ich beim allerersten
Get eine Ausnahme
auslösen, wenn wir glauben, dass der Knoten vorhanden sein muss, oder vor der Dereferenzierung eine Prüfung hinzufügen, wenn die Nichtexistenz des Knotens die Gesamtlogik nicht ändert (zum Beispiel erhalten wir eine Standardwert).
Lösung 1:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Lösung 2:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
PVS-Studio-Diagnosemeldung: 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) { .... } .... }
Dies ist ein recht einfaches Beispiel im Vergleich zum vorherigen. Der Analysator vermutet, dass die
QueryAsync- Methode möglicherweise eine
Nullreferenz zurückgibt . Hier ist die Implementierung der Methode:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Da
GetQueryAsync eine Schnittstellenmethode ist, können Sie sich bei jeder Implementierung nicht sicher sein, insbesondere wenn wir bedenken, dass das Projekt auch die folgende Version enthält:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
Die mehrfachen Aufrufe externer Funktionen und Cache-Zugriffe erschweren die Analyse von
GetDocumentAsync. Nehmen wir also an, dass die Prüfung erforderlich ist - umso mehr, als die Methode asynchron ist.
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 nur die hohe Qualität des Orchard-Codes erwähnen! Es gab zwar einige andere Mängel, die ich hier nicht besprochen habe, aber ich habe Ihnen die schwerwiegendsten Fehler gezeigt. Dies bedeutet natürlich nicht, dass es ausreicht, Ihren Quellcode alle drei Jahre einmal zu überprüfen. Wenn Sie sie
regelmäßig verwenden, können Sie die statische Analyse optimal nutzen, da Sie auf diese Weise garantiert Fehler in den frühesten Entwicklungsphasen erkennen und beheben können, in denen die Fehlerbehebung am billigsten und einfachsten ist.
Auch wenn einmalige Überprüfungen nicht viel helfen, empfehle ich Ihnen dennoch,
PVS-Studio herunterzuladen und es in Ihrem Projekt auszuprobieren: Wer weiß, vielleicht finden Sie auch einige interessante Fehler.