Als wir uns entschieden, nach Fehlern im Azure SDK für .NET-Projekt zu suchen, waren wir angenehm überrascht von seiner Größe. "Dreieinhalb Millionen Codezeilen", sagten wir und studierten die Statistiken des Projekts. Möglicherweise gibt es so viele Erkenntnisse. Ach und ach! Das Projekt erwies sich als schlau. Was war der Reiz des Projekts und wie wurde es überprüft - lesen Sie in diesem Artikel.
Über das Projekt
Ich schreibe diesen Artikel im Anschluss an meinen vorherigen, der sich auch mit einem Projekt im Zusammenhang mit Microsoft Azure befasste:
Azure PowerShell: Mostly Harmless . Dieses Mal habe ich also auf eine solide Anzahl verschiedener und interessanter Fehler gesetzt. Schließlich ist die Projektgröße ein sehr wichtiger Faktor für die statische Analyse, insbesondere bei der erstmaligen Überprüfung eines Projekts. In der Praxis ist die Anwendung von Einmalprüfungen nicht der richtige Ansatz. Wenn sich Entwickler dafür entscheiden, erfolgt dies jedoch erst in der Phase der Einführung des Analysegeräts. Gleichzeitig macht sich niemand daran, die enorme Anzahl von Warnungen auszusortieren und sie einfach als technische Schulden zu verwerfen, indem Massenwarnmechanismen zur Unterdrückung von Warnungen eingesetzt und in speziellen Basen gespeichert werden. Apropos: Wenn Sie das Analysegerät zum ersten Mal verwenden, ist es in Ordnung, eine große Anzahl von Warnungen zu erhalten. Für uns werden einmalige Überprüfungen zu Forschungszwecken durchgeführt. Aus diesem Grund sind große Projekte für die folgende Analyse im Vergleich zu kleinen immer vorzuziehen.
Das Azure SDK für .NET-Projekt erwies sich jedoch sofort als unrentable Testumgebung. Sogar die beeindruckende Größe hat nicht geholfen, sondern die Arbeit ist kompliziert. Der Grund ist in der folgenden Projektstatistik angegeben:
- CS-Quelldateien (ohne Tests): 16.500
- Visual Studio-Lösungen (.sln): 163
- Nicht leere Codezeilen: 3 462 000
- Davon automatisch generiert: ca. 3.300.000
- Das Projekt-Repository ist auf GitHub verfügbar.
Ungefähr 95% des Codes werden automatisch generiert, und ein Großteil dieses Codes wird mehrmals wiederholt. Das Überprüfen solcher Projekte mit einem statischen Analysegerät ist in der Regel zeitaufwändig und nutzlos, da viele funktionsfähige, aber unlogische (zumindest auf den ersten Blick) und redundante Codes vorhanden sind. Dies führt zu einer Vielzahl von Fehlalarmen.
Die gesamte Menge an Code, die über 163 Visual Studio-Lösungen verteilt war, wurde zur "Kirsche an der Spitze". Es waren einige Anstrengungen erforderlich, um den verbleibenden Code zu überprüfen (nicht automatisch generiert). Was wirklich geholfen hat, war die Tatsache, dass der gesamte automatisch generierte Code in Lösungsunterverzeichnissen unter dem relativen Pfad "<Lösungsverzeichnis> \ src \ Generated" gespeichert wurde. Außerdem enthält jede .cs-Datei dieses Typs einen speziellen Kommentar im Tag
<automatisch generiert> :
Für die Reinheit des Experiments überprüfte ich lückenhaft etwa zehn zufällig ausgewählte automatisch generierte Lösungen. Ich werde später über das Ergebnis erzählen.
Trotz der geringen Menge des verbleibenden „ehrlichen“ Codes konnte ich dennoch eine Reihe von Fehlern aus dem verbleibenden Code finden. Dieses Mal werde ich keine Warnungen in der Reihenfolge der PVS-Studio-Diagnosecodes anführen. Stattdessen gruppiere ich die Nachrichten nach den Lösungen, in denen sie gefunden wurden.
Mal sehen, was ich im Azure SDK für .NET-Code gefunden habe.
Microsoft.Azure.Management.Advisor
Dies ist eine von vielen Lösungen, die automatisch generierten Code enthalten. Wie ich bereits sagte, überprüfte ich zufällig etwa ein Dutzend solcher Lösungen. In jedem Fall waren die Warnungen dieselben und erwartungsgemäß nutzlos. Hier sind einige Beispiele.
V3022 Der Ausdruck 'Credentials! = Null' ist immer wahr. AdvisorManagementClient.cs 204
Offensichtlich ist dieser Code redundant und die Prüfung der
Berechtigungsnachweise! = Null ist sinnlos. Trotzdem funktioniert der Code. Und wird automatisch generiert. Aus diesem Grund hier keine Beschwerden.
V3022 Der Ausdruck '_queryParameters.Count> 0' ist immer falsch. ConfigurationsOperations.cs 871
Wiederum scheint es eine unlogische Konstruktion zu sein. Aus irgendeinem Grund überprüfen Codeautoren die Größe der neu erstellten
leeren Liste. In der Tat ist alles richtig. An dieser Stelle macht die Prüfung keinen Sinn. Wenn Entwickler jedoch eine Listengenerierung hinzufügen, die beispielsweise auf einer anderen Sammlung basiert, lohnt sich die Prüfung auf jeden Fall. Auch hier - natürlich keine Ansprüche an den Code in Bezug auf seine Herkunft.
Für jede automatisch generierte Lösung wurden Hunderte ähnlicher Warnungen ausgegeben. Angesichts ihrer Sinnlosigkeit halte ich es nicht für sinnvoll, solche Fälle weiter zu erörtern. Als nächstes werden nur echte Fehler im "normalen" Code berücksichtigt.
Azure.Core
V3001 Es gibt identische Unterausdrücke 'buffer.Length' links und rechts vom Operator '<'. AzureBaseBuffersExtensions.cs 30
public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....) { byte[]? array = null; .... if (array == null || buffer.Length < buffer.Length)
Der Fehler in der Bedingung war wahrscheinlich das Ergebnis von Copy-Paste. Entsprechend der Tatsache, dass der
Puffer in ein
Array kopiert
wird , sollte die Prüfung folgendermaßen aussehen:
if (array == null || array.Length < buffer.Length)
Wie ich immer sage, sollte sich der Autor des Codes auf jeden Fall mit der Behebung solcher Fehler befassen.
V3083 Unsicherer Aufruf des Ereignisses '_onChange', NullReferenceException ist möglich. Überlegen Sie, ob Sie einer lokalen Variablen ein Ereignis zuweisen möchten, bevor Sie sie aufrufen. ClientOptionsMonitor.cs 44
private event Action<TOptions, string> _onChange; .... private void InvokeChanged(....) { .... if (_onChange != null) { _onChange.Invoke(options, name); } }
Nicht kritisch, aber ein Fehler ist hier. Der Konsument kann das Ereignis zwischen dem Überprüfen des Ereignisses auf
Null und dessen Aufruf abbestellen. Dann ist die Variable
_onChange null und es wird eine Ausnahme ausgelöst. Dieser Code muss sicherer umgeschrieben werden. Zum Beispiel wie folgt:
private void InvokeChanged(....) { .... _onChange?.Invoke(options, name); }
Azure.Messaging.EventHubs
V3080 Mögliche Null-Dereferenzierung. Sehen Sie sich 'eventPropertyValue' an. AmqpMessageConverter.cs 650
private static bool TryCreateEventPropertyForAmqpProperty( object amqpPropertyValue, out object eventPropertyValue) { eventPropertyValue = null; .... switch (GetTypeIdentifier(amqpPropertyValue)) { case AmqpProperty.Type.Byte: .... case AmqpProperty.Type.String: eventPropertyValue = amqpPropertyValue; return true; .... } .... switch (amqpPropertyValue) { case AmqpSymbol symbol: eventPropertyValue = ....; break; case byte[] array: eventPropertyValue = ....; break; case ArraySegment<byte> segment when segment.Count == segment.Array.Length: eventPropertyValue = ....; break; case ArraySegment<byte> segment: .... eventPropertyValue = ....; break; case DescribedType described when (described.Descriptor is AmqpSymbol): eventPropertyValue = ....; break; default: var exception = new SerializationException( string.Format(...., eventPropertyValue.GetType().FullName));
Mal sehen, was mit dem Variablenwert
eventPropertyValue im angegebenen Codefragment passiert. Die Variable erhält zu Beginn der Methode den Wert
null . Ferner wird in einer der ersten
Schaltbedingungen die Variable initialisiert, wonach das Verfahren beendet wird. Der zweite
Schalterblock enthält viele Bedingungen, in denen die Variable auch einen neuen Wert erhält. Während im
Standardblock die Variable
eventPropertyValue ohne Prüfung verwendet wird, ist dies ein Fehler, da die Variable derzeit
null ist.
V3066 Mögliche falsche Reihenfolge der Argumente, die an den Konstruktor 'EventHubConsumer' übergeben wurden: 'partitionId' und 'consumerGroup'. TrackOneEventHubClient.cs 394
public override EventHubConsumer CreateConsumer(....) { return new EventHubConsumer ( new TrackOneEventHubConsumer(....), TrackOneClient.EventHubName, partitionId,
Der Analysator vermutet beim Aufrufen des
EventHubConsumer- Klassenkonstruktors eine verwirrte Reihenfolge des dritten und vierten Arguments. Schauen wir uns also diese Konstruktordeklaration an:
internal EventHubConsumer(TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, // <= 3 string partitionId, // <= 4 EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... }
In der Tat sind die Argumente verwechselt. Ich würde es wagen vorzuschlagen, wie der Fehler gemacht wurde. Möglicherweise ist hier eine falsche Code-Formatierung schuld. Schauen Sie sich die
EventHubConsumer- Konstruktordeklaration noch einmal an. Aufgrund der Tatsache, dass sich der erste
transportConsumer- Parameter in derselben Zeile wie der Klassenname befindet, scheint der
partitionId- Parameter an dritter und nicht an vierter Stelle zu stehen (meine Kommentare mit den Parameternummern sind im Originalcode nicht verfügbar). . Das ist nur eine Vermutung, aber ich würde die Formatierung des Konstruktorcodes folgendermaßen ändern:
internal EventHubConsumer ( TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, string partitionId, EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... }
Azure.Storage
V3112 Eine Abnormalität in ähnlichen Vergleichen. Es ist möglich, dass der Ausdruck 'ContentLanguage == other.ContentEncoding' einen Tippfehler enthält. BlobSasBuilder.cs 410
public struct BlobSasBuilder : IEquatable<BlobSasBuilder> { .... public bool Equals(BlobSasBuilder other) => BlobName == other.BlobName && CacheControl == other.CacheControl && BlobContainerName == other.BlobContainerName && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding &&
Ein Fehler, der durch Unaufmerksamkeit begangen wurde. Einen solchen Fehler bei der Codeüberprüfung zu finden, ist ziemlich schwierig. Hier ist die richtige Version des Codes:
.... ContentEncoding == other.ContentEncoding && ContentLanguage == other.ContentLanguage && ....
V3112 Eine Abnormalität in ähnlichen Vergleichen. Es ist möglich, dass der Ausdruck 'ContentLanguage == other.ContentEncoding' einen Tippfehler enthält. FileSasBuilder.cs 265
public struct FileSasBuilder : IEquatable<FileSasBuilder> { .... public bool Equals(FileSasBuilder other) => CacheControl == other.CacheControl && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding
Es gibt genau den gleichen Fehler in einem sehr ähnlichen Code. Der Code wurde möglicherweise kopiert und teilweise geändert. Der Fehler blieb jedoch bestehen.
Microsoft.Azure.Batch
V3053 Ein übermäßiger Ausdruck. Untersuchen Sie die Teilzeichenfolgen 'IList' und 'List'. PropertyData.cs 157
V3053 Ein übermäßiger Ausdruck. Untersuchen Sie die Teilzeichenfolgen 'List' und 'IReadOnlyList'. PropertyData.cs 158
public class PropertyData { .... public bool IsTypeCollection => this.Type.Contains("IList") || this.Type.Contains("IEnumerable") || this.Type.Contains("List") ||
Der Analysator gab zwei Warnungen über sinnlose oder fehlerhafte Überprüfungen aus. Im ersten Fall erscheint die Suche nach der Unterzeichenfolge "List" nach der Suche nach "IList" redundant. Es ist wahr, diese Bedingung:
this.Type.Contains("IList") || this.Type.Contains("List")
kann für das folgende gut geändert werden:
this.Type.Contains("List")
Im zweiten Fall ist die Suche nach der Teilzeichenfolge "IReadOnlyList" sinnlos, da zuvor nach einer kürzeren Teilzeichenfolge "List" gesucht wird.
Es besteht auch die Möglichkeit, dass Such-Teilzeichenfolgen selbst Fehler aufweisen und etwas anderes vorhanden sein sollte. Auf jeden Fall muss nur der Autor des Codes die richtige Codeversion vorschlagen, wobei beide Kommentare berücksichtigt werden.
V3095 Das Objekt 'httpRequest.Content.Headers' wurde verwendet, bevor es gegen null verifiziert wurde. Zeilen überprüfen: 76, 79. BatchSharedKeyCredential.cs 76
public override Task ProcessHttpRequestAsync( HttpRequestMessage httpRequest, ....) { .... signature.Append(httpRequest.Content != null && httpRequest.Content.Headers.Contains("Content-Language") ? .... : ....; long? contentLength = httpRequest.Content?.Headers?.ContentLength; .... }
Die Variable
httpRequest.Content.Headers wird zunächst ohne Prüfung verwendet, später jedoch mit dem Operator für den bedingten Zugriff angesprochen.
V3125 Das Objekt 'omPropertyData' wurde verwendet, nachdem es gegen null verifiziert wurde. Überprüfen Sie die Zeilen: 156, 148. CodeGenerationUtilities.cs 156
private static string GetProtocolCollectionToObjectModelCollectionString( ...., PropertyData omPropertyData, ....) { if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....)) { .... } if (IsTypeComplex(omPropertyData.GenericTypeParameter)) .... }
Und hier ist eine umgekehrte Situation. Ein Codeblock enthält eine sichere Zugriffsvariante auf die Referenz
omPropertyData, die möglicherweise null ist. Weiter im Code wird diese Referenz ohne Prüfung behandelt.
V3146 Mögliche Null-Dereferenzierung von 'Wert'. Der 'FirstOrDefault' kann einen Standard-Nullwert zurückgeben. BatchSharedKeyCredential.cs 127
public override Task ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....) { .... foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } .... }
Aufgrund der
FirstOrDefault- Methode wird der Standardwert zurückgegeben, der für den
Zeichenfolgentyp null ist, wenn die Suche fehlschlägt. Der Wert wird der Wertevariablen zugewiesen, die dann im Code mit der
Replace- Methode ohne Prüfung verwendet wird. Der Code sollte sicherer gemacht werden. Zum Beispiel wie folgt:
foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... }
Microsoft.Azure.ServiceBus
V3121 Eine Aufzählung 'BlocksUsing' wurde mit dem Attribut 'Flags' deklariert, setzt jedoch keine Initialisierer, um Standardwerte zu überschreiben. Fx.cs 69
static class Fx { .... public static class Tag { .... [Flags] public enum BlocksUsing { MonitorEnter, MonitorWait, ManualResetEvent, AutoResetEvent, AsyncResult, IAsyncResult, PInvoke, InputQueue, ThreadNeutralSemaphore, PrivatePrimitive, OtherInternalPrimitive, OtherFrameworkPrimitive, OtherInterop, Other, NonBlocking, } .... } .... }
Die Aufzählung wird mit dem Attribut
Flags deklariert. Gleichzeitig bleiben die konstanten Werte standardmäßig erhalten (
MonitorEnter = 0 ,
MonitorWait = 1 ,
ManualResetEvent = 2 usw.). Dies kann in folgendem Fall auftreten: Wenn Sie versuchen, eine Flags-Kombination zu verwenden, werden beispielsweise die zweite und die dritte Konstante
MonitorWait (= 1) | verwendet
ManualResetEvent (= 2) , es wird kein eindeutiger Wert empfangen, sondern standardmäßig die Konstante mit dem Wert 3
(AutoResetEvent ). Dies kann für den Anrufercode eine Überraschung sein. Wenn die
BlocksUsing- Enumeration wirklich zum Setzen von
Merkerkombinationen (Bitfeld) verwendet werden soll, sollten Konstanten Werte erhalten, die der Zahl mit Zweierpotenzen entsprechen.
[Flags] public enum BlocksUsing { MonitorEnter = 1, MonitorWait = 2, ManualResetEvent = 4, AutoResetEvent = 8, AsyncResult = 16, IAsyncResult = 32, PInvoke = 64, InputQueue = 128, ThreadNeutralSemaphore = 256, PrivatePrimitive = 512, OtherInternalPrimitive = 1024, OtherFrameworkPrimitive = 2048, OtherInterop = 4096, Other = 8192, NonBlocking = 16384, }
V3125 Das Objekt 'session' wurde verwendet, nachdem es gegen null geprüft wurde. Überprüfen Sie die Zeilen: 69, 68. AmqpLinkCreator.cs 69
public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try {
Beachten Sie die Behandlung der
Sitzungsvariablen im
catch- Block. Die
Abort- Methode wird vom Operator für bedingte Zugriffe sicher aufgerufen. Aber nach der
GetInnerException- Methode wird unsicher aufgerufen. Dabei wird möglicherweise
NullReferenceException anstelle einer Ausnahme des erwarteten Typs ausgelöst. Dieser Code muss behoben werden. Die
AmqpExceptionHelper.GetClientException- Methode unterstützt die Übergabe des
Nullwerts für den
innerException- Parameter:
public static Exception GetClientException( Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false) { .... }
Daher kann beim Aufrufen von
session.GetInnerException () nur der Operator für bedingte Zugriffe verwendet werden:
public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try {
Fazit
Wie Sie sehen, garantiert eine große Projektgröße nicht immer viele Fehler. Wir bleiben jedoch wachsam, da wir immer etwas finden können. Auch in einem Projekt, das strukturell so komplex ist wie das Azure SDK für .NET. Das Auffinden einiger entscheidender Mängel erfordert zusätzlichen Aufwand. Aber je schwieriger, desto angenehmer das Ergebnis. Um unnötigen Aufwand zu vermeiden, empfehlen wir, beim Schreiben von neuem Code statische Analysen direkt auf den Computern der Entwickler durchzuführen. Dies ist der effektivste Ansatz.
Laden Sie PVS-Studio herunter und testen Sie es in Aktion. Viel Glück bei der Bekämpfung von Bugs!