Scansione del codice di Orchard CMS alla ricerca di bug

Questo articolo esamina i risultati di un secondo controllo del progetto Orchard con l'analizzatore statico PVS-Studio. Orchard è un sistema di gestione dei contenuti open source fornito come parte della ASP.NET Open Source Gallery sotto la Outercurve Foundation senza scopo di lucro. Il controllo di oggi è particolarmente interessante perché sia ​​il progetto che l'analizzatore hanno fatto molta strada dal primo controllo e questa volta esamineremo nuovi messaggi diagnostici e alcuni bei bug.

Informazioni su CMS Frutteto

Abbiamo controllato Orchard tre anni fa. Da allora l'analizzatore C# di PVS-Studio si è notevolmente evoluto:abbiamo migliorato l'analisi del flusso di dati, aggiunto l'analisi interprocedurale e una nuova diagnostica e corretto un certo numero di falsi positivi. Inoltre, il secondo controllo ha rivelato che gli sviluppatori di Orchard avevano corretto tutti i bug segnalati nel primo articolo, il che significa che avevamo raggiunto il nostro obiettivo, ovvero li avevamo aiutati a migliorare il loro codice.

Spero che presteranno attenzione anche a questo articolo e apportino le correzioni necessarie o, meglio ancora, adottino PVS-Studio per l'uso su base regolare. Come promemoria, forniamo agli sviluppatori open source una licenza gratuita. A proposito, ci sono anche altre opzioni di cui potrebbero beneficiare i progetti proprietari.

Il codice sorgente di Orchard è disponibile per il download qui. La descrizione completa del progetto si trova qui. Se non hai ancora una copia di PVS-Studio, puoi scaricare la versione di prova. Ho usato PVS-Studio 7.05 Beta e includerò alcuni dei suoi avvisi in questo articolo. Spero che questa recensione ti convincerà che PVS-Studio è uno strumento utile. Tieni presente che è pensato per essere utilizzato regolarmente.

Risultati dell'analisi

Ecco alcune delle cifre del primo controllo di Orchard in modo da non dover passare da un articolo all'altro per il confronto.

Durante il controllo precedente "abbiamo fatto l'analisi di tutti i file di codice sorgente (3739 elementi) con estensione .cs. In totale si trattava di 214.564 righe di codice. Il risultato del controllo è stato di 137 avvisi. Per essere più precisi, ci c'erano 39 avvisi di primo livello (alto). C'erano anche 60 avvisi di secondo livello (medio)".

L'attuale versione di Orchard è composta da 2.767 file .cs, ovvero circa un migliaio di file più piccoli. Il ridimensionamento e la ridenominazione del repository suggeriscono che gli sviluppatori hanno isolato il core del progetto (commit 966), che è lungo 108.287 LOC. L'analizzatore ha emesso 153 allarmi:33 di primo livello e 70 di secondo livello. In genere non includiamo avvisi di terzo livello e mi atterrò alla tradizione.

Messaggio diagnostico di PVS-Studio: V3110 Possibile ricorsione infinita all'interno del metodo 'TryValidateModel'. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix)
{
  return TryValidateModel(model, Prefix(prefix));
}

Iniziamo con un bug di ricorsione infinita, come abbiamo fatto nel primo articolo. Questa volta le intenzioni esatte dello sviluppatore non sono chiare, ma ho notato che il TryValidateModel il metodo aveva una versione sovraccaricata con un parametro:

public bool TryValidateModel(object model)
{
  return _updateModel.TryValidateModel(model);
}

Penso che, proprio come nel caso della versione sovraccaricata, lo sviluppatore intendesse chiamare il metodo tramite _updateModel. Il compilatore non ha notato l'errore; _updateModel è di tipo IUpdateModel e anche la classe corrente implementa questa interfaccia. Poiché il metodo non include alcun controllo contro StackOverflowException , probabilmente non è mai stato chiamato, anche se non ci conto. Se la mia ipotesi è corretta, la versione fissa dovrebbe apparire così:

public bool TryValidateModel(object model, string prefix)
{
  return _updateModel.TryValidateModel(model, Prefix(prefix));
}

Messaggio diagnostico di PVS-Studio: V3008 Alla variabile 'content' vengono assegnati valori due volte di seguito. Forse questo è un errore. Righe di controllo: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);
  ....
}

L'analizzatore ha rilevato due assegnazioni alla variabile locale content. GetChildContentAsync è un metodo di libreria che viene utilizzato troppo raramente per permetterci di prenderci la briga di esaminarlo e annotarlo. Quindi, temo, né noi né l'analizzatore sappiamo nulla dell'oggetto di ritorno del metodo e degli effetti collaterali. Ma sappiamo per certo che assegnare il valore di ritorno a contenuto non ha senso se non viene utilizzato ulteriormente nel codice. Forse è solo un'operazione ridondante piuttosto che un errore. Non posso dire esattamente come dovrebbe essere risolto, quindi lo lascio agli sviluppatori.

Messaggio diagnostico di PVS-Studio: V3080 Possibile dereferenziazione nulla. Considera la possibilità di ispezionare '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;
}

L'analizzatore ha rilevato un dereferenziamento non sicuro di itemTag . Questo frammento è un buon esempio di come uno strumento di analisi statica sia diverso da uno sviluppatore umano che esegue la revisione del codice. Il metodo ha un parametro chiamato ItemTag e una variabile locale denominata itemTag . Non c'è bisogno di dirti che fa un'enorme differenza per il compilatore! Si tratta di due variabili diverse, anche se correlate. Il modo in cui sono correlati è tramite una terza variabile, itemTagName. Ecco la sequenza di passaggi che portano alla possibile eccezione:if the ItemTag argomento è uguale a "-", nessun valore verrà assegnato a itemTagName , quindi rimarrà un riferimento null e, se è un riferimento null, la variabile locale itemTag si trasformerà anche in un riferimento nullo. A mio parere, è meglio avere un'eccezione generata dopo il controllo della stringa.

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;
}

Messaggio diagnostico di PVS-Studio: V3095 L'oggetto 'remoteClient' è stato utilizzato prima di essere verificato rispetto a null. Righe di controllo: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 || ....)
  {
    ....
  }
  ....
}

L'analizzatore ha rilevato una dereferenziazione di remoteClient seguito da un controllo nullo un paio di righe dopo. Questa è davvero una potenziale NullReferenceException come FirstOrDefault il metodo può restituire un valore predefinito (che è null per i tipi di riferimento). Immagino che questo frammento di codice possa essere corretto semplicemente spostando il controllo in alto in modo che preceda l'operazione di dereferenziazione:

public async Task<IActionResult> Import(ImportViewModel model)
{
  ....
  var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....);
  if (remoteClient != null)
     var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey);
  else if (....)
  {
    ....
  }
  ....
}

O forse dovrebbe essere risolto sostituendo FirstOrDefault con Primo e rimuovendo del tutto l'assegno.

Avvertenze di PVS-Studio 7.05 Beta:

A questo punto, abbiamo annotato tutti i LINQ 's oPredefinito metodi. Queste informazioni verranno utilizzate dalla nuova diagnostica su cui stiamo lavorando:rileva i casi in cui i valori restituiti da questi metodi vengono dereferenziati senza un controllo preventivo. Ogni o Predefinito metodo ha una controparte che genera un'eccezione se non è stato trovato alcun elemento corrispondente. Questa eccezione sarà più utile per rintracciare il problema rispetto all'abstract NullReferenceException .

Non posso che condividere i risultati che ho ottenuto da questa diagnostica sul progetto Orchard. Ci sono 27 punti potenzialmente pericolosi. Eccone alcuni:

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);

Messaggio diagnostico di PVS-Studio: V3080 Possibile dereference null del valore restituito dal metodo. Prendi in considerazione l'ispezione:CreateScope(). SetupService.cs 136

public async Task<string> SetupInternalAsync(SetupContext context)
{
  ....
  using (var shellContext = await ....)
  {
    await shellContext.CreateScope().UsingAsync(....);
  }
  ....
}

L'analizzatore ha menzionato una dereferenziazione del valore restituito da CreateScope metodo. CreateScope è un metodo minuscolo, quindi ecco la sua implementazione completa:

public ShellScope CreateScope()
{
  if (_placeHolder)
  {
    return null;
  }
  var scope = new ShellScope(this);
  // A new scope can be only used on a non released shell.
  if (!released)
  {
    return scope;
  }
  scope.Dispose();
  return null;
}

Come puoi vedere, ci sono due casi in cui può restituire null . L'analizzatore non sa quale ramo seguirà il flusso di esecuzione, quindi gioca sul sicuro e segnala il codice come sospetto. Se dovessi scrivere un codice del genere, scriverei subito un controllo nullo.

Forse la mia opinione è parziale, ma credo che ogni metodo asincrono dovrebbe essere protetto da NullReferenceException il più possibile perché il debug di cose del genere è tutt'altro che divertente.

In questo caso particolare, il CreateScope viene chiamato quattro volte:due di queste chiamate sono accompagnate da assegni e le altre due no. Le ultime due chiamate (senza controlli) sembrano essere cloni copia-incolla (stessa classe, stesso metodo, stesso modo di dereferenziare il risultato per chiamare UsingAsync). La prima di queste due chiamate è stata mostrata sopra e puoi star certo che la seconda ha attivato lo stesso avviso:

V3080 Possibile dereference null del valore restituito dal metodo. Prendi in considerazione l'ispezione:CreateScope(). SetupService.cs 192

Messaggio diagnostico di PVS-Studio: V3127 Sono stati trovati due frammenti di codice simili. Forse si tratta di un errore di battitura e la variabile "AccessTokenSecret" dovrebbe essere utilizzata al posto di "ConsumerSecret" TwitterClientMessageHandler.cs 52

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);
  ....
}

Questo è un classico errore di copia-incolla. ConsumerSecret è stato controllato due volte, mentre AccessTokenSecret non è stato affatto controllato. Ovviamente, questo è risolto come segue:

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);
  ....
}

Messaggio diagnostico di PVS-Studio: V3139 Due o più case branch eseguono le stesse azioni. SerialDocumentExecuter.cs 23

Un altro bug del copia-incolla. Per chiarezza, ecco l'implementazione completa della classe (è piccola).

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 ....;
    }
  }
}

All'analizzatore non sono piaciuti i due casi identici rami. In effetti, la classe ha tre entità, mentre l'istruzione switch ne restituisce solo due. Se questo comportamento è previsto e la terza entità non è effettivamente pensata per essere utilizzata, il codice può essere migliorato rimuovendo il terzo ramo dopo aver unito i due come segue:

switch (context.Operation.OperationType)
{
  case OperationType.Query:
  case OperationType.Mutation:
    return SerialExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Se si tratta di un bug di copia-incolla, il primo dei campi restituiti duplicati dovrebbe essere corretto come segue:

switch (context.Operation.OperationType)
{
  case OperationType.Query:
    return ParallelExecutionStrategy;

  case OperationType.Mutation:
    return SerialExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

O dovrebbe essere il secondo ramo del caso. Non conosco i dettagli del progetto e quindi non riesco a determinare la correlazione tra i nomi dei tipi di operazioni e le strategie.

switch (context.Operation.OperationType)
{
  case OperationType.Query:
    return SerialExecutionStrategy; 

  case OperationType.Mutation:
    return ParallelExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Messaggio diagnostico di PVS-Studio: V3080 Possibile dereferenziazione nulla. Considera di ispezionare la "richiesta". 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;
  ....
}

La richiesta alla variabile viene assegnato un valore diverso da null più volte nel primo se blocco, ma ogni volta con condizioni nidificate. Includere tutte queste condizioni renderebbe l'esempio troppo lungo, quindi andremo solo con le prime, che controllano il tipo del metodo http IsGet o IsPost . Microsoft.AspNetCore.Http.HttpMethods class ha nove metodi statici per controllare il tipo di query. Pertanto, passando, ad esempio, un Elimina o Imposta interrogare ExecuteAsync metodo porterebbe a sollevare una NullReferenceException . Anche se tali metodi non sono attualmente supportati, sarebbe comunque saggio aggiungere un controllo di generazione delle eccezioni. Dopotutto, i requisiti di sistema potrebbero cambiare. Ecco un esempio di tale controllo:

private async Task ExecuteAsync(HttpContext context....)
{
  ....
  if (request == null)
    throw ....;
  var queryToExecute = request.Query;
  ....
}

Messaggio diagnostico di PVS-Studio: V3080 Possibile dereference null del valore restituito dal metodo. Prendere in considerazione l'ispezione:Get(...). ContentPartHandlerCoordinator.cs 190

La maggior parte degli avvisi del V3080 è più comoda da visualizzare all'interno dell'ambiente di sviluppo perché è necessaria la navigazione del metodo, l'evidenziazione del tipo e l'atmosfera amichevole dell'IDE. Sto cercando di ridurre il più possibile il testo degli esempi per mantenerli leggibili. Ma se non lo sto facendo bene o se vuoi testare le tue capacità di programmazione e capire tutto da solo, ti consiglio di controllare il risultato di questa diagnostica su qualsiasi progetto open source o solo sul tuo codice.

public override async Task LoadingAsync(LoadContentContext context)
{
  ....
  context.ContentItem.Get<ContentPart>(typePartDefinition.Name)
                     .Weld(fieldName, fieldActivator.CreateInstance());
  ....
}

L'analizzatore riporta questa riga. Diamo un'occhiata a Get metodo:

public static TElement Get<TElement>(this ContentElement contentElement....)
        where TElement : ContentElement
{
    return (TElement)contentElement.Get(typeof(TElement), name);
}

Chiama la sua versione sovraccaricata. Controlliamolo anche noi:

public static ContentElement Get(this ContentElement contentElement....)
{
  ....
  var elementData = contentElement.Data[name] as JObject;
  if (elementData == null)
  {
    return null;
  }
  ....
}

Si scopre che se otteniamo un'entità di un tipo incompatibile con JObject da Dati utilizzando il nome indicizzatore, il Ottieni il metodo restituirà null . Non so con certezza quanto sia probabile perché questi tipi provengono da Newtonsoft.Json libreria, con cui non ho lavorato molto. Ma l'autore del codice sospettava che l'elemento cercato potesse non esistere, quindi dovremmo tenerlo a mente anche quando accediamo al risultato dell'operazione di lettura. Personalmente, farei un'eccezione nel primo Get se riteniamo che il nodo debba essere presente, oppure aggiungiamo un segno di spunta prima della dereferenziazione se la non esistenza del nodo non cambia la logica generale (otteniamo ad esempio un valore di default).

Soluzione 1:

public static ContentElement Get(this ContentElement contentElement....)
{
  ....
  var elementData = contentElement.Data[name] as JObject;
  if (elementData == null)
  {
    throw....
  }
  ....
}

Soluzione 2:

public override async Task LoadingAsync(LoadContentContext context)
{
  ....
  context.ContentItem.Get<ContentPart>(typePartDefinition.Name)
                     ?.Weld(fieldName, fieldActivator.CreateInstance());
  ....
}

Messaggio diagnostico di PVS-Studio: V3080 Possibile dereferenziazione nulla. Prendi in considerazione l'ispezione dei "risultati". ContentQueryOrchardRazorHelperExtensions.cs 19

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....)
{
  var results = await orchardHelper.QueryAsync(queryName, parameters);
  ....
  foreach (var result in results)
  {
    ....
  }
 ....
}

Questo è un esempio abbastanza semplice rispetto al precedente. L'analizzatore sospetta che QueryAsync il metodo potrebbe restituire un riferimento null. Ecco l'implementazione del metodo:

public static async Task<IEnumerable> QueryAsync(....)
{
  ....
  var query = await queryManager.GetQueryAsync(queryName);
  if (query == null)
  {
    return null;
  }
  ....
}

Da GetQueryAsync è un metodo di interfaccia, non puoi essere sicuro di ogni implementazione, soprattutto se consideriamo che il progetto include anche la seguente versione:

public async Task<Query> GetQueryAsync(string name)
{
  var document = await GetDocumentAsync();
  if(document.Queries.TryGetValue(name, out var query))
  {
    return query;
  }
  return null;
}

Le molteplici chiamate a funzioni esterne e gli accessi alla cache rendono l'analisi di GetDocumentAsync difficile, quindi diciamo solo che il controllo è necessario, tanto più che il metodo è asincrono.

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....)
{
  var results = await orchardHelper.QueryAsync(queryName, parameters);
  if(results == null)
    throw ....;
  ....
  foreach (var result in results)
  {
    ....
  }
 ....
}

Conclusione

Non posso non menzionare l'alta qualità del codice di Orchard! È vero, c'erano altri difetti, di cui non ho parlato qui, ma ti ho mostrato tutti i bug più gravi. Naturalmente, questo non vuol dire che sia sufficiente controllare il codice sorgente una volta ogni tre anni. Otterrai il massimo dall'analisi statica se la utilizzi regolarmente, perché questo è il modo in cui hai la garanzia di rilevare e correggere i bug nelle prime fasi di sviluppo, dove la correzione dei bug è più economica e semplice.

Anche se i controlli una tantum non aiutano molto, ti incoraggio comunque a scaricare PVS-Studio e provarlo sul tuo progetto:chissà, forse troverai anche dei bug interessanti.