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. So viel kannst du dort finden. Aber leider, ah. Das Projekt erwies sich als geheim. Was ist die Besonderheit des Projekts und wie es getestet wurde - lesen Sie diesen Artikel.
Über das Projekt
Ich schreibe diesen Artikel in einer Fortsetzung meines vorherigen Artikels, in dem es auch um ein Projekt im Zusammenhang mit Microsoft Azure ging:
Azure PowerShell: "Im Grunde genommen harmlos". Diesmal rechnete ich also mit einer soliden Anzahl von abwechslungsreichen und interessanten Fehlern. In der Tat ist für eine statische Analyse in der Regel die Größe des Projekts wichtig, insbesondere bei einmaligen Überprüfungen des gesamten Projekts. Ja, in der Praxis machen sie das normalerweise nicht. Und wenn doch, dann erst in der Phase der Implementierung des Analysators. Gleichzeitig versteht niemand sofort die große Anzahl von Vorgängen (eine beträchtliche Anzahl von Warnungen ist die Norm, wenn der Analysator in diesem Modus startet), sondern verschuldet sie einfach mit Hilfe von Nachrichtenunterdrückungsmechanismen und deren Speicherung in speziellen Datenbanken (Massenunterdrückung). Wir führen einmalige Inspektionen zu Forschungszwecken durch. Daher sind große Studienprojekte immer kleinen vorzuziehen.
Das Azure SDK für .NET-Projekt zeigte jedoch sofort, dass es als Testumgebung fehlgeschlagen ist. Auch seine beeindruckende Größe hat nicht geholfen, sondern die Arbeit sogar kompliziert. Der Grund ist in der folgenden Projektstatistik angegeben:
- Quelldateien .cs (ohne Tests): 16.500
- Visual Studio-Lösungen (.sln): 163
- Nicht leere Codezeilen: 3.462.000
- Davon automatisch generiert: rund 3,3 Millionen
- Das Projekt-Repository ist auf GitHub verfügbar.
Etwa 95% des Codes werden automatisch generiert, ein wesentlicher Teil dieses Codes wird mehrmals wiederholt. Das Überprüfen solcher Projekte mit einem statischen Analysator ist normalerweise zeitaufwändig und nutzlos, da viel gearbeitet wird, aber unlogischer (auf den ersten Blick) und redundanter Code. Dies führt zu einer Vielzahl von Fehlalarmen.
Eine große Anzahl von Visual Studio-Lösungen (163) wurden als Kirschen auf dem Kuchen verwendet, wonach diese Codemasse "verschmiert" wurde. Um den verbleibenden Code zu überprüfen (nicht automatisch generiert), musste ich einige Anstrengungen unternehmen. Es hat geholfen, dass sich der gesamte automatisch generierte Code in den Lösungsunterordnern entlang des relativen Pfads "<Lösungsordner> \ src \ Generated" befindet. Außerdem enthält jede .cs-Datei eines solchen Codes einen speziellen Kommentar im
<auto-generated> -Tag :
Für die Reinheit des Experiments überprüfte ich zufällig ungefähr zehn zufällig ausgewählte Lösungen mit automatisch erzeugtem Code. Die Ergebnisse werden niedriger sein.
Trotz der geringen Menge des verbleibenden "ehrlichen" Codes konnte ich dort dennoch eine bestimmte Anzahl von Fehlern finden. Dieses Mal werde ich keine Auslösungen in der Reihenfolge der Nummern der PVS-Studio-Diagnose geben. Stattdessen werde ich die Antworten nach den Lösungen gruppieren, in denen sie erkannt wurden.
Mal sehen, was ich im Azure SDK für .NET-Code gefunden habe.
Microsoft.Azure.Management.Advisor
Dies ist nur eine von vielen Lösungen, die automatisch generierten Code enthalten. Wie ich oben sagte, wurden ungefähr ein Dutzend solcher Lösungen selektiv getestet. Und überall waren die Nachrichten gleich und erwartungsgemäß nutzlos. Lassen Sie mich einige Beispiele für solche Antworten nennen.
V3022 Der Ausdruck 'Credentials! = Null' ist immer wahr. AdvisorManagementClient.cs 204
Offensichtlich ist der Code redundant und die Überprüfung der
Berechtigungsnachweise! = Null ist nutzlos. Aber der Code funktioniert. Und automatisch generiert. Daher - keine Beschwerden.
V3022 Der Ausdruck '_queryParameters.Count> 0' ist immer falsch. ConfigurationsOperations.cs 871
Und wieder ein scheinbar logikloses Design. Aus irgendeinem Grund überprüfen sie die Größe der gerade erstellten
leeren Liste. In der Tat - alles ist in Ordnung. Diese Prüfung ist jetzt sinnlos, aber wenn der Generator die Erstellung einer Liste z. B. auf der Grundlage einer anderen Sammlung aufbaut, ist die Prüfung bereits sinnvoll. Auch hier gibt es natürlich keine Beschwerden gegen den Code, da er seinen Ursprung hat.
Für jede Lösung mit automatisch generiertem Code wurden Hunderte ähnlicher Warnungen empfangen. In Anbetracht ihrer Sinnlosigkeit halte ich eine weitere Erörterung derartiger positiver Aspekte für sinnlos. Im Folgenden 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)
Es wurde ein Fehler in der Bedingung gemacht, wahrscheinlich aufgrund der Verwendung von Copy-Paste. Gemessen an der Tatsache, dass weiter im Codepuffer in das
Array kopiert
wird , sollte die Prüfung wie
folgt aussehen:
if (array == null || array.Length < buffer.Length)
Aber, wie ich immer sage, sollte der Autor des Codes solche Fehler beheben.
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); } }
Unkritisch, aber ein Fehler. Zwischen der Überprüfung des Ereignisses auf
Null und seinem Aufruf kann das Ereignis abgemeldet werden. Dann wird die Variable
_onChange auf
null gesetzt und eine Ausnahme ausgelöst. Code muss sicherer umgeschrieben werden. Zum Beispiel so:
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));
Verfolgen wir, was mit dem Wert der Variablen
eventPropertyValue im obigen
Codeausschnitt geschieht. Zu Beginn der Methode ist die Variable
null . Ferner wird in einer der Bedingungen des ersten
Schalters 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. Im
Standardblock wird die Variable
eventPropertyValue jedoch einfach ohne Überprüfung verwendet.
Dies ist 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 vermutete, dass beim Aufrufen des Konstruktors der
EventHubConsumer- Klasse die Reihenfolge der dritten und vierten Argumente
vertauscht wurde. Schauen wir uns die Konstruktordeklaration an:
internal EventHubConsumer(TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, // <= 3 string partitionId, // <= 4 EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... }
Die Argumente sind wirklich durcheinander. Ich vermute, wie dieser Fehler gemacht wurde. Der Fehler ist wahrscheinlich die schlechte Formatierung des Codes. Schauen Sie sich die
EventHubConsumer- Konstruktordeklaration noch einmal an. Aufgrund der Tatsache, dass sich der erste
transportConsumer- Parameter in der gleichen Zeile wie der Klassenname befindet, scheint der
partitionId- Parameter bei einer kurzen Betrachtung des Codes an dritter und nicht an vierter Stelle zu stehen (meine Kommentare mit den Seriennummern der Parameter sind im Originalcode nicht enthalten) )
Dies ist nur eine Vermutung, aber ich würde die Formatierung des Konstruktordeklarationscodes 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 ähnlichen Fehler bei der Codeüberprüfung zu finden, ist ziemlich schwierig. Die richtige Prüfoption:
.... 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
Genau derselbe Fehler in einem sehr ähnlichen Code. Der Code wurde wahrscheinlich 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 Teilzeichenfolge "Liste" nach der Suche nach "IList" redundant. In der Tat ist die Bedingung:
this.Type.Contains("IList") || this.Type.Contains("List")
kann dadurch ersetzt werden:
this.Type.Contains("List")
Im zweiten Fall ist die Suche nach dem Teilstring "IReadOnlyList" bedeutungslos, da zuvor die Suche nach dem kürzeren Teilstring "List" durchgeführt wurde.
Es besteht auch die Möglichkeit, dass die Teilzeichenfolgen selbst für die Suche Fehler gemacht haben und es etwas anderes geben sollte. In jedem Fall kann die korrekte Version der Bedingungskorrektur unter Berücksichtigung beider Bemerkungen nur vom Autor des Codes angeboten 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; .... }
Zuerst wird die Variable
httpRequest.Content.Headers ohne Prüfung verwendet, aber später im Code wird auf diese Variable mit dem Operator für bedingten Zugriff zugegriffen.
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)) .... }
Die umgekehrte Situation. Ein Codeblock enthält eine sichere Zugriffsoption auf den möglicherweise Null-
omPropertyData- Link. Weiter im Code mit dem gleichen Link funktionieren sie ohne Prüfung.
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(); .... } .... }
Als Ergebnis der
FirstOrDefault- Methode wird, wenn die Suche fehlschlägt, der Standardwert für den
Zeichenfolgentyp zurückgegeben,
dh null . Der Wert wird der Variablen value zugewiesen, die später im Code mit der Methode
Replace ohne Prüfung verwendet wird. Code sollte sicherer gemacht werden. Zum Beispiel so:
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, } .... } .... }
Eine Aufzählung wird mit dem Attribut
Flags deklariert. In diesem Fall bleiben die Werte der Konstanten standardmäßig erhalten (
MonitorEnter = 0 ,
MonitorWait = 1 ,
ManualResetEvent = 2 usw.). Dies kann dazu führen, dass beim Versuch, eine
Kombination von Flags zu verwenden, beispielsweise die zweite und dritte Konstante
MonitorWait (= 1) |
ManualResetEvent (= 2) , es wird kein eindeutiger Wert empfangen, sondern standardmäßig eine Konstante mit dem Wert 3 (
AutoResetEvent ). Dies kann den aufrufenden Code überraschen. Wenn die
BlocksUsing- Enumeration wirklich zum
Angeben von Flag-Kombinationen (Bitfeld) verwendet werden soll, sollten Sie die Konstantenwerte mit Zweierpotenzen angeben:
[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 {
Achten Sie auf die Arbeit mit der
Sitzungsvariablen im
catch-Block . Die
Abort- Methode wird sicher über die Conditional Access-Anweisung
aufgerufen . Dann rufen sie die
GetInnerException- Methode unsicher auf. In diesem Fall wird möglicherweise eine
NullReferenceException ausgelöst, anstatt eine Ausnahme des erwarteten Typs
auszulösen . Der Code muss repariert werden. Die
AmqpExceptionHelper.GetClientException- Methode unterstützt die Übergabe eines
Nullwerts für den
innerException- Parameter:
public static Exception GetClientException( Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false) { .... }
Daher ist es ausreichend, den Operator für den bedingten Zugriff beim Aufrufen von
session.GetInnerException () zu verwenden :
public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try {
Fazit
Wie Sie sehen, garantiert eine große Projektgröße nicht immer eine große Anzahl von Fehlern. Aber keine Notwendigkeit, sich zu entspannen - Sie können immer etwas finden. Auch in solch einem komplexen Projekt wie Azure SDK für .NET. Ja, dies erfordert zusätzliche Anstrengungen, aber je angenehmer das Ergebnis sein wird. Und damit Sie sich nicht zu sehr anstrengen müssen, empfehlen wir die Verwendung einer statischen Analyse und die Arbeit der Entwickler beim Schreiben von neuem Code. Dies ist der effektivste Ansatz.
Laden Sie PVS-Studio herunter und testen Sie es in Aktion. Viel Glück im Kampf gegen Bugs!
Wenn Sie diesen Artikel mit einem englischsprachigen Publikum teilen möchten, verwenden Sie bitte den Link zur Übersetzung: Sergey Khrenov.
Azure SDK für .NET: Geschichte über eine schwierige Fehlersuche .