Azure SDK per .NET:storia di una difficile ricerca di errori

Quando abbiamo deciso di cercare errori nel progetto Azure SDK per .NET, siamo rimasti piacevolmente sorpresi dalle sue dimensioni. "Tre milioni e mezzo di righe di codice", continuavamo a dire, studiando le statistiche del progetto. Potrebbero esserci così tanti risultati. Ahimè e ahimè! Il progetto si è rivelato furbo. Quindi qual è stato il gusto del progetto e come è stato verificato - leggi questo articolo.

Informazioni sul progetto

Sto scrivendo questo articolo facendo seguito al mio precedente, che riguardava anche un progetto relativo a Microsoft Azure:Azure PowerShell:per lo più innocuo. Quindi, questa volta stavo scommettendo su un numero solido di errori diversi e interessanti. Dopotutto, la dimensione del progetto è un fattore molto importante in termini di analisi statica, in particolare quando si verifica un progetto per la prima volta. Infatti, in pratica, l'applicazione di controlli una tantum non è l'approccio giusto. Tuttavia, se gli sviluppatori lo fanno, avviene solo nella fase di introduzione dell'analizzatore. Allo stesso tempo, nessuno si dà da fare per risolvere l'enorme numero di avvertimenti e semplicemente indugiarli come debito tecnico usando meccanismi di soppressione degli allarmi di massa e immagazzinandoli in basi speciali. A proposito, avere un gran numero di avvisi va bene quando si esegue l'analizzatore per la prima volta. Quanto a noi, eseguiamo controlli una tantum per scopi di ricerca. Per questo motivo, per l'analisi successiva, i grandi progetti sono sempre più preferibili rispetto a quelli piccoli.

Tuttavia, il progetto Azure SDK per .NET si è immediatamente rivelato un banco di prova impraticabile. Anche le sue dimensioni impressionanti non hanno aiutato, ma hanno complicato il lavoro su di esso. Il motivo è fornito nelle seguenti statistiche del progetto:

• File sorgente .cs (esclusi i test):16 500
• Soluzioni Visual Studio (.sln):163
• Righe di codice non vuote:3 462 000
• Di questi generati automaticamente:circa 3.300.000
• Il repository del progetto è disponibile su GitHub.

Circa il 95% del codice viene generato automaticamente e gran parte di quel codice viene ripetuto molte volte. Il controllo di tali progetti con un analizzatore statico è solitamente dispendioso in termini di tempo e inutile, poiché esiste molto codice praticabile, ma illogico (almeno a prima vista) e ridondante. Questo porta a un gran numero di falsi positivi.

Tutta quella quantità di codice sparsa in tutte le 163 soluzioni di Visual Studio è diventata la "ciliegina sulla torta". Ci sono voluti alcuni sforzi per controllare il codice rimanente (non generato automaticamente). Ciò che ha davvero aiutato è stato il fatto che tutto il codice generato automaticamente è stato archiviato nelle sottodirectory delle soluzioni dal percorso relativo "\src\Generated". Inoltre ogni file .cs di questo tipo contiene un commento speciale nel tag :

// <auto-generated>
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for
// license information.
//
// Code generated by Microsoft (R) AutoRest Code Generator.
// Changes may cause incorrect behavior and will be lost if the code is
// regenerated.
// </auto-generated>

Per la purezza dell'esperimento, ho controllato in modo irregolare una decina di soluzioni generate automaticamente selezionate casualmente. Parlerò del risultato più avanti.

Quindi, nonostante la piccola quantità di codice "onesto" rimanente, sono comunque riuscito a trovare una serie di errori da ciò che è rimasto. Questa volta non citerò gli avvisi nell'ordine dei codici diagnostici PVS-Studio. Raggrupperò invece i messaggi sulle soluzioni in cui sono stati trovati.

Bene, vediamo cosa sono riuscito a trovare nell'SDK di Azure per il codice .NET.

Microsoft.Azure.Management.Advisor

Questa è una delle tante soluzioni che contiene codice generato automaticamente. Come ho detto prima, ho controllato casualmente una dozzina di tali soluzioni. In ogni caso, gli avvertimenti erano gli stessi e, come previsto, inutili. Ecco un paio di esempi.

L'espressione V3022 'Credenziali !=null' è sempre vera. AdvisorManagementClient.cs 204

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public ServiceClientCredentials Credentials { get; private set; }
....
public AdvisorManagementClient(ServiceClientCredentials credentials,
  params DelegatingHandler[] handlers) : this(handlers)
{
  if (credentials == null)
  {
    throw new System.ArgumentNullException("credentials");
  }
  Credentials = credentials;
  if (Credentials != null)    // <=
  {
    Credentials.InitializeServiceClient(this);
  }
}

Ovviamente, questo codice è ridondante e le Credenziali !=null il controllo è inutile. Comunque il codice funziona. Ed è generato automaticamente. Per questo motivo, nessuna lamentela qui.

V3022 L'espressione '_queryParameters.Count> 0' è sempre falsa. ConfigurationsOperations.cs 871

// Code generated by Microsoft (R) AutoRest Code Generator.
....
public async Task<AzureOperationResponse<IPage<ConfigData>>>
  ListBySubscriptionNextWithHttpMessagesAsync(....)
{
  ....
  List<string> _queryParameters = new List<string>();
  if (_queryParameters.Count > 0)
  {
    ....
  }
  ....
}

Ancora una volta, sembra una costruzione illogica. Per qualche motivo, gli autori del codice controllano la dimensione del vuoto appena creato elenco. In effetti, è tutto corretto. A questo punto, il controllo non ha senso, ma nel caso in cui gli sviluppatori aggiungano la generazione di liste, ad esempio, sulla base di un'altra raccolta, il controllo sarà sicuramente utile. Ancora una volta - nessuna pretesa al codice, ovviamente, per quanto riguarda la sua origine.

Centinaia di avvisi simili sono stati emessi per ogni soluzione generata automaticamente. Data la loro futilità, penso che non abbia senso discutere ulteriormente di tali casi. Successivamente, verranno presi in considerazione solo gli errori reali nel codice "normale".

Azure.Core

V3001 Sono presenti sottoespressioni identiche 'buffer.Length' a sinistra ea destra dell'operatore '<'. AzureBaseBuffersExtensions.cs 30

public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....)
{
  byte[]? array = null;
  ....
  if (array == null || buffer.Length < buffer.Length)  // <=
  {
    if (array != null)
      ArrayPool<byte>.Shared.Return(array);
    array = ArrayPool<byte>.Shared.Rent(buffer.Length);
  }
  if (!buffer.TryCopyTo(array))
    throw new Exception("could not rent large enough buffer.");
  ....
}

L'errore nella condizione era probabilmente il risultato del copia-incolla. Secondo il fatto che buffer viene copiato in array , l'assegno dovrebbe essere simile a:

if (array == null || array.Length < buffer.Length)

Ad ogni modo, come dico sempre, l'autore del codice dovrebbe occuparsi di correggere tali errori.

V3083 Richiamo non sicuro dell'evento '_onChange', NullReferenceException è possibile. Prendere in considerazione l'assegnazione di un evento a una variabile locale prima di richiamarla. ClientOptionsMonitor.cs 44

private event Action<TOptions, string> _onChange;
....
private void InvokeChanged(....)
{
  ....
  if (_onChange != null)
  {
    _onChange.Invoke(options, name);
  }
}

Non critico, ma c'è un errore qui. Il consumatore potrebbe annullare l'iscrizione all'evento tra la verifica dell'evento per null e la sua invocazione. Quindi _onChange la variabile sarà null e verrà generata un'eccezione. Questo codice deve essere riscritto in modo più sicuro. Ad esempio, come segue:

private void InvokeChanged(....)
{
  ....
  _onChange?.Invoke(options, name);
}

Azure.Messaging.EventHubs

V3080 Possibile dereferenziazione nulla. Prendi in considerazione l'ispezione di "eventPropertyValue". 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));  // <=
      ....
  }

  return (eventPropertyValue != null);
}

Vediamo cosa succede con eventPropertyValue valore variabile nel frammento di codice specificato. La variabile viene assegnata null all'inizio del metodo. Inoltre, in uno dei primi switch condizioni, la variabile viene inizializzata, dopodiché il metodo esce. Il secondo interruttore block contiene molte condizioni, in ognuna delle quali la variabile riceve anche un nuovo valore. Mentre nel predefinito blocco, il eventPropertyValue variabile viene utilizzata senza alcun controllo, il che è un errore, poiché la variabile è null in questo momento.

V3066 Possibile ordine errato degli argomenti passati al costruttore 'EventHubConsumer':'partitionId' e 'consumerGroup'. TrackOneEventHubClient.cs 394

public override EventHubConsumer CreateConsumer(....)
{
  return new EventHubConsumer
  (
    new TrackOneEventHubConsumer(....),
    TrackOneClient.EventHubName,
    partitionId,                  // <= 3
    consumerGroup,                // <= 4
    eventPosition,
    consumerOptions,
    initialRetryPolicy
  );
}

L'analizzatore sospettava l'ordine confuso del terzo e del quarto argomento quando chiamava EventHubConsumer costruttore di classe. Quindi esaminiamo questa dichiarazione del costruttore:

internal EventHubConsumer(TransportEventHubConsumer transportConsumer,
                          string eventHubName,
                          string consumerGroup,         // <= 3
                          string partitionId,           // <= 4
                          EventPosition eventPosition,
                          EventHubConsumerOptions consumerOptions,
                          EventHubRetryPolicy retryPolicy)
{
  ....
}

In effetti, le argomentazioni sono confuse. Mi permetto di suggerire come è stato commesso l'errore. Forse la colpa è della formattazione del codice errata. Dai un'altra occhiata a EventHubConsumer dichiarazione del costruttore. A causa del fatto che il primo transportConsumer è sulla stessa riga del nome della classe, può sembrare che partitionId il parametro è al terzo posto, non al quarto (i miei commenti con i numeri dei parametri non sono disponibili nel codice originale). Questa è solo un'ipotesi, ma cambierei la formattazione del codice del costruttore come segue:

internal EventHubConsumer
(
  TransportEventHubConsumer transportConsumer,
  string eventHubName,
  string consumerGroup,
  string partitionId,
  EventPosition eventPosition,
  EventHubConsumerOptions consumerOptions,
  EventHubRetryPolicy retryPolicy)
{
  ....
}

Azure.Storage

V3112 Un'anomalia all'interno di confronti simili. È possibile che sia presente un errore di battitura all'interno dell'espressione 'ContentLanguage ==other.ContentEncoding'. 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 &&         // <=
    ContentLanguage == other.ContentEncoding &&         // <=
    ContentType == other.ContentType &&
    ExpiryTime == other.ExpiryTime &&
    Identifier == other.Identifier &&
    IPRange == other.IPRange &&
    Permissions == other.Permissions &&
    Protocol == other.Protocol &&
    StartTime == other.StartTime &&
    Version == other.Version;
}

Un errore fatto per disattenzione. Trovare un tale errore con la revisione del codice è abbastanza difficile. Ecco la versione corretta del codice:

    ....
    ContentEncoding == other.ContentEncoding &&
    ContentLanguage == other.ContentLanguage &&
    ....

V3112 Un'anomalia all'interno di confronti simili. È possibile che sia presente un errore di battitura all'interno dell'espressione 'ContentLanguage ==other.ContentEncoding'. FileSasBuilder.cs 265

public struct FileSasBuilder : IEquatable<FileSasBuilder>
{
  ....
  public bool Equals(FileSasBuilder other)
    => CacheControl == other.CacheControl
    && ContentDisposition == other.ContentDisposition
    && ContentEncoding == other.ContentEncoding         // <=
    && ContentLanguage == other.ContentEncoding         // <=
    && ContentType == other.ContentType
    && ExpiryTime == other.ExpiryTime
    && FilePath == other.FilePath
    && Identifier == other.Identifier
    && IPRange == other.IPRange
    && Permissions == other.Permissions
    && Protocol == other.Protocol
    && ShareName == other.ShareName
    && StartTime == other.StartTime
    && Version == other.Version
    ;

C'è esattamente lo stesso errore in un pezzo di codice molto simile. Il codice potrebbe essere stato copiato e parzialmente modificato. Ma l'errore è rimasto.

Microsoft.Azure.Batch

V3053 Espressione eccessiva. Esaminare le sottostringhe 'IList' e 'List'. PropertyData.cs 157

V3053 Espressione eccessiva. Esaminare le sottostringhe 'List' e 'IReadOnlyList'. PropertyData.cs 158

public class PropertyData
{
  ....
  public bool IsTypeCollection => this.Type.Contains("IList") ||
                                  this.Type.Contains("IEnumerable") ||
                                  this.Type.Contains("List") ||        // <=
                                  this.Type.Contains("IReadOnlyList"); // <=
}

L'analizzatore ha emesso due avvertimenti su controlli inutili o errati. Nel primo caso, cercare la sottostringa "List" dopo che la ricerca di "IList" sembra ridondante. È vero, questa condizione:

this.Type.Contains("IList") || this.Type.Contains("List")

può essere ben modificato per quanto segue:

this.Type.Contains("List")

Nel secondo caso, la ricerca della sottostringa "IReadOnlyList" non ha senso, poiché in precedenza viene cercata una sottostringa più breve "List".

C'è anche la possibilità che le stesse sottostringhe di ricerca abbiano errori e che dovrebbe esserci qualcos'altro. Ad ogni modo, solo l'autore del codice può suggerire la versione corretta del codice tenendo conto di entrambi i commenti.

V3095 L'oggetto 'httpRequest.Content.Headers' è stato utilizzato prima che fosse verificato rispetto a null. Righe di controllo: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;
  ....
}

Gli httpRequest.Content.Headers La variabile viene prima utilizzata senza alcun controllo, ma successivamente viene indirizzata utilizzando l'operatore di accesso condizionale.

V3125 L'oggetto 'omPropertyData' è stato utilizzato dopo che è stato verificato rispetto a null. Righe di controllo:156, 148. CodeGenerationUtilities.cs 156

private static string GetProtocolCollectionToObjectModelCollectionString(
  ...., PropertyData omPropertyData, ....)
{
  if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....))
  {
    ....
  }

  if (IsTypeComplex(omPropertyData.GenericTypeParameter))
  ....
}

Ed ecco una situazione inversa. Un blocco di codice contiene una variante di accesso sicuro a omPropertyData riferimento potenzialmente nullo. Più avanti nel codice, questo riferimento viene gestito senza alcun controllo.

V3146 Possibile dereferenziazione nulla di 'valore'. Il 'FirstOrDefault' può restituire il valore null predefinito. 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();
    ....
  }
  ....
}

A causa del FirstOrDefault metodo, se la ricerca non riesce, verrà restituito il valore predefinito, che è null per la stringa genere. Il valore verrà assegnato al valore variabile, che viene quindi utilizzata nel codice con Sostituisci metodo senza alcun controllo. Il codice dovrebbe essere reso più sicuro. Ad esempio, come segue:

foreach (string canonicalHeader in customHeaders)
{
  string value = httpRequest.Headers.
                 GetValues(canonicalHeader).FirstOrDefault();
  value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart();
  ....
}

Microsoft.Azure.ServiceBus

V3121 Un'enumerazione 'BlocksUsing' è stata dichiarata con l'attributo 'Flags', ma non imposta alcun inizializzatore per sovrascrivere i valori predefiniti. 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,
    }
    ....
  }
  ....
}

L'enumerazione viene dichiarata con i Flags attributo. Allo stesso tempo, i valori costanti vengono lasciati per impostazione predefinita (MonitorEnter =0 , MonitorWait =1 , ManualResetEvent =2 e così via). Ciò può comportare il seguente caso:quando si tenta di utilizzare la combinazione di flag, ad esempio, la seconda e la terza costante MonitorWait (=1) | ManualResetEvent (=2) , non verrà ricevuto un valore univoco, ma la costante con il valore 3 per impostazione predefinita (AutoResetEvent ). Questo potrebbe sorprendere per il codice del chiamante. Se il BlocksUsing l'enumerazione deve essere effettivamente utilizzata per impostare combinazioni di flag (campo di bit), alle costanti dovrebbero essere assegnati valori, uguali a numeri che sono potenze di due.

[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 L'oggetto 'sessione' è stato utilizzato dopo che è stato verificato rispetto a null. Righe di controllo:69, 68. AmqpLinkCreator.cs 69

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Presta attenzione alla sessione gestione delle variabili nel catch bloccare. Il Interrompi viene chiamato in modo sicuro dall'operatore di accesso condizionale. Ma dopo la GetInnerException il metodo viene chiamato in modo non sicuro. In tal modo, NullReferenceException potrebbe essere generata invece di un'eccezione del tipo previsto. Questo codice deve essere corretto. AmqpExceptionHelper.GetClientException il metodo supporta il passaggio di null valore per innerException parametro:

public static Exception GetClientException(
  Exception exception, 
  string referenceId = null, 
  Exception innerException = null, 
  bool connectionError = false)
{
  ....
}

Pertanto, è possibile utilizzare solo l'operatore di accesso condizionale quando si chiama session.GetInnerException() :

public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync()
{
  ....
  AmqpSession session = null;
  try
  {
    // Create Session
    ....
  }
  catch (Exception exception)
  {
    ....
    session?.Abort();
    throw AmqpExceptionHelper.GetClientException(exception, null,
      session?.GetInnerException(), amqpConnection.IsClosing());
  }
  ....
}

Conclusione

Come puoi vedere, un progetto di grandi dimensioni non garantisce sempre molti errori. Tuttavia, stiamo attenti poiché possiamo sempre trovare qualcosa. Anche in un progetto strutturalmente complesso come Azure SDK per .NET. Trovare alcuni difetti cruciali richiede sforzi aggiuntivi. Ma più difficoltà, più piacevole sarà il risultato. D'altra parte, per evitare sforzi inutili, consigliamo di utilizzare l'analisi statica direttamente sui computer degli sviluppatori durante la scrittura di nuovo codice. Questo è l'approccio più efficace. Scarica e prova PVS-Studio in azione. Buona fortuna nella lotta contro gli insetti!