PHP è compilabile?! PVS-Studio cerca gli errori in PeachPie

PHP è ampiamente conosciuto come linguaggio di programmazione interpretato utilizzato principalmente per lo sviluppo di siti Web. Tuttavia, poche persone sanno che PHP ha anche un compilatore per .NET – PeachPie. Ma quanto è fatto bene? L'analizzatore statico sarà in grado di trovare bug reali in questo compilatore? Scopriamolo!

È passato un po' di tempo da quando abbiamo pubblicato articoli sul controllo dei progetti C# utilizzando PVS-Studio... E dobbiamo ancora fare la Top list dei bug del 2021 (a proposito, la Top 10 dei bug del 2020, puoi trovarla qui)! Bene, dobbiamo riparare i nostri modi. Sono entusiasta di mostrarti una recensione dei risultati del controllo PeachPie.

Per cominciare, lascia che ti parli un po' del progetto. PeachPie è un moderno compilatore di linguaggio PHP open source e runtime per .NET Framework e .NET. È costruito sulla piattaforma del compilatore Microsoft Roslyn e si basa sul progetto Phalanger di prima generazione. Nel luglio 2017 il progetto è diventato membro della .NET Foundation. Il codice sorgente è disponibile nel repository GitHub.

A proposito, il nostro analizzatore C# fa anche un ampio uso delle capacità di Roslyn, quindi in un certo senso, PeachPie e PVS-Studio hanno qualcosa in comune :). Abbiamo già lavorato con Roslyn. Inoltre, abbiamo scritto un intero articolo sulle basi per lavorare con questa piattaforma.

Per controllare PeachPie, abbiamo dovuto installare l'analizzatore, aprire il progetto in Visual Studio o Rider ed eseguire l'analisi utilizzando il plug-in PVS-Studio. Per maggiori dettagli, consulta la documentazione.

È stato divertente controllare un progetto così grande e serio. Spero che apprezzerai anche la mia recensione dei bug trovati in PeachPie. Buona lettura!

Problemi con WriteLine

Bene, iniziamo con uno facile :) A volte i bug possono apparire nei posti più inaspettati e allo stesso tempo più semplici. Ad esempio, un errore può anche apparire in un facile WriteLine chiamata di funzione:

public static bool mail(....)
{
  // to and subject cannot contain newlines, replace with spaces
  to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
  subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
                              : "";

  Debug.WriteLine("MAILER",
                  "mail('{0}','{1}','{2}','{3}')",
                  to,
                  subject,
                  message, 
                  additional_headers);

  var config = ctx.Configuration.Core;
  
  ....
}

L'avviso V3025:formato errato. È previsto un numero diverso di elementi di formato durante la chiamata alla funzione 'WriteLine'. Argomenti non utilizzati:1°, 2°, 3°, 4°, 5°. Mail.cs 25

Penseresti, cosa è andato storto? Tutto sembra andare bene. Aspetta un minuto, però! Quale argomento dovrebbe passare il formato?

Bene, diamo un'occhiata a Debug.WriteLine dichiarazione:

public static void WriteLine(string format, params object[] args);

La stringa di formato deve essere passata come primo argomento e il primo argomento nel codice è "MAILER" . Ovviamente, lo sviluppatore ha confuso i metodi e ha passato gli argomenti in modo errato.

Stessi casi in switch

Questa sezione è dedicata agli avvisi associati all'esecuzione delle stesse azioni in diversi rami del caso:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.DisallowNullAttribute":
        result |= FlowAnalysisAnnotations.DisallowNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullAttribute":
        result |= FlowAnalysisAnnotations.MaybeNull;
        break;
      case "System.Diagnostics.CodeAnalysis.MaybeNullWhenAttribute":
        if (TryGetBoolArgument(attr, out bool maybeNullWhen))
        {
          result |= maybeNullWhen ? FlowAnalysisAnnotations.MaybeNullWhenTrue
                                  : FlowAnalysisAnnotations.MaybeNullWhenFalse;
        }
        break;
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
    }
  }
}

Questo frammento contiene se non un errore, almeno una cosa strana. Quanto velocemente riesci a trovarlo?

Tuttavia, non perdere tempo, l'analizzatore ha trovato tutto per noi:

private static FlowAnalysisAnnotations DecodeFlowAnalysisAttributes(....)
{
  var result = FlowAnalysisAnnotations.None;

  foreach (var attr in attributes)
  {
    switch (attr.AttributeType.FullName)
    {
      case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;
        break;
      ....
      case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
        result |= FlowAnalysisAnnotations.AllowNull;              // <=
        break;
    }
  }
}

L'avviso V3139:due o più case branch eseguono le stesse azioni. ReflectionUtils.Nullability.cs 170

Non è strano che due casi diversi vengano gestiti allo stesso modo? In realtà no, questo accade abbastanza spesso. Tuttavia, ci sono 2 particolarità.

In primo luogo, vale la pena notare che esiste un modo più aggraziato per trattare due casi diversi allo stesso modo. Puoi riscrivere il frammento sopra come segue:

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
}

Tuttavia, gli sviluppatori spesso trascurano questo metodo conveniente e preferiscono il copia-incolla. Pertanto, la presenza di due rami identici non sembra così terribile. Il fatto che le FlowAnalysisAnnotations l'enumerazione ha, tra l'altro, FlowAnalysisAnnotations.NotNull il valore è molto più sospetto. Questo valore sembra essere utilizzato quando "System.Diagnostics.CodeAnalysis.NotNullAttribute" il valore viene elaborato:

switch (attr.AttributeType.FullName)
{
  case "System.Diagnostics.CodeAnalysis.AllowNullAttribute":
    result |= FlowAnalysisAnnotations.AllowNull;
    break;
  ....
  case "System.Diagnostics.CodeAnalysis.NotNullAttribute":
    result |= FlowAnalysisAnnotations.NotNull;              // <=
    break;
}

DataOra immutabile

Gli sviluppatori spesso commettono errori perché non capiscono come funzionano le caratteristiche dei metodi di "modifica". Ecco il bug trovato in PeachPie:

using System_DateTime = System.DateTime;

internal static System_DateTime MakeDateTime(....) { .... }

public static long mktime(....)
{
  var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
  var local = MakeDateTime(hour, minute, second, month, day, year);

  switch (daylightSaving)
  {
    case -1:
      if (zone.IsDaylightSavingTime(local))
        local.AddHours(-1);                   // <=
      break;
    case 0:
      break;
    case 1:
      local.AddHours(-1);                     // <=
      break;
    default:
      PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
      break;
  }
  return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local, 
                                                                   ....));
}

Gli avvisi di PVS-Studio:

• V3010 È necessario utilizzare il valore di ritorno della funzione 'AddHours'. DateTimeFunctions.cs 1232
• V3010 È necessario utilizzare il valore di ritorno della funzione 'AddHours'. DateTimeFunctions.cs 1239

L'analizzatore segnala che i risultati delle chiamate dovrebbero essere registrati da qualche parte, altrimenti non hanno alcun senso. Il fatto è che metodi come AddHours non modificare l'oggetto originale, al contrario, viene restituito un nuovo oggetto e di conseguenza differisce da quello originale. È difficile dire quanto sia critico questo errore, ma è chiaro che il frammento di codice non funziona correttamente.

Metodi di prova con particolarità

I metodi di prova sono spesso molto convenienti per lo sviluppo di app in C#. I metodi di prova più noti sono int.TryParse , Dizionario.TryGetValue , ecc. Di solito questi metodi restituiscono un flag che indica l'esito positivo dell'operazione. Il risultato viene scritto nel parametro out. Gli sviluppatori di PeachPie hanno deciso di implementare i loro metodi try che avrebbero dovuto funzionare allo stesso modo. Cosa ne è venuto fuori? Diamo un'occhiata al seguente codice:

internal static bool TryParseIso8601Duration(string str,
                                             out DateInfo result,
                                             out bool negative)
{
  ....
  if (pos >= length) goto InvalidFormat;

  if (s[pos++] != 'P') goto InvalidFormat;

  if (!Core.Convert.TryParseDigits(....))
    goto Error;
  
  if (pos >= length) goto InvalidFormat;

  if (s[pos] == 'Y')
  {
    ....

    if (!Core.Convert.TryParseDigits(....)) 
      goto Error;

    if (pos >= length) goto InvalidFormat;
  }
  ....
  InvalidFormat:
  Error:

    result = default;
    negative = default;
    return false;
}

Questo metodo è abbreviato per la leggibilità. Puoi trovare il metodo completo facendo clic sul collegamento. Core.Convert.TryParseDigits viene chiamato molte volte nel metodo. Nei casi in cui tale chiamata restituisce false , il thread di esecuzione salta all'Errore etichetta, che è logico.

Su Errore etichetta, valori predefiniti sono assegnati a out- parametri. Quindi, il metodo restituisce false . Tutto sembra logico:la TryParseIso8601Duration il metodo si comporta esattamente come i metodi try standard. Beh... Almeno, è così che sembra. In effetti, non è così :(.

Come accennato in precedenza, se Core.Convert.TryParseDigits restituisce falso , il codice salta all'Errore label, dove viene eseguita la gestione di bug/problemi. Tuttavia, ecco il problema:l'analizzatore segnala che TryParseDigits non restituisce mai falso :

L'avviso V3022:l'espressione '!Core.Convert.TryParseDigits(s, ref pos, false, out value, out numDigits)' è sempre falsa. DateTimeParsing.cs 1440

Se la negazione del risultato della chiamata è sempre falsa , la chiamata restituisce sempre true . Che comportamento specifico per il metodo try! L'operazione va sempre a buon fine? Diamo infine un'occhiata a TryParseDigits :

public static bool TryParseDigits(....)
{
  Debug.Assert(offset >= 0);

  int offsetStart = offset;
  result = 0;
  numDigits = 0;

  while (....)
  {
    var digit = s[offset] - '0';

    if (result > (int.MaxValue - digit) / 10)
    {
      if (!eatDigits)
      {
        // overflow
        //return false;
        throw new OverflowException();
      }

      ....

      return true;
    }

    result = result * 10 + digit;
    offset++;
  }

  numDigits = offset - offsetStart;
  return true;
}

Il metodo restituisce sempre true . Ma l'operazione potrebbe non riuscire, in questo caso, un'eccezione di OverflowException il tipo viene lanciato. Per quanto mi riguarda, questo non è chiaramente quello che ti aspetti da un metodo di prova :). A proposito, c'è una riga con return false , ma è commentato.

Forse, l'uso di un'eccezione qui è in qualche modo giustificato. Ma secondo il codice, sembra che qualcosa sia andato storto. ProvaAnalisiDigit e TryParseIso8601Duration il suo utilizzo dovrebbe funzionare come i soliti metodi di prova:restituire false in caso di fallimento. Al contrario, generano eccezioni impreviste.

Valore argomento predefinito

Il seguente messaggio dell'analizzatore è più semplice, ma indica anche un frammento di codice piuttosto strano:

private static bool Put(Context context,
                        PhpResource ftp_stream,
                        string remote_file,
                        string local_file,
                        int mode,
                        bool append,
                        int startpos)
{ .... }

public static bool ftp_put(Context context,
                           PhpResource ftp_stream,
                           string remote_file,
                           string local_file,
                           int mode = FTP_IMAGE,
                           int startpos = 0)
{
    return Put(context,
               ftp_stream,
               remote_file,
               local_file,
               mode = FTP_IMAGE, // <=
               false,
               startpos);
}

L'avviso V3061:il parametro 'modalità' viene sempre riscritto nel corpo del metodo prima di essere utilizzato. FTP.cs 306

Il ftp_put accetta un numero di parametri come input, uno dei parametri è mode . Ha un valore predefinito, ma quando viene chiamato, chiaramente, puoi impostare un altro valore. Tuttavia, questo non influisce su nulla:modalità viene sempre sovrascritto e Put riceve sempre il valore di FTP_IMAGE costante.

È difficile dire perché tutto sia scritto in questo modo:il costrutto sembra privo di significato. È molto probabile che sia presente un errore.

Il copia-incolla invia i saluti

Il seguente frammento di codice sembra una vittima di copia-incolla:

public static PhpValue filter_var(....)
{
  ....
  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_PRIV_RANGE)
  {
    throw new NotImplementedException();
  }

  if ((flags & (int)FilterFlag.NO_PRIV_RANGE) == (int)FilterFlag.NO_RES_RANGE)
  {
    throw new NotImplementedException();
  }
  ....
}

Avviso V3127:sono stati trovati due frammenti di codice simili. Forse questo è un errore di battitura e la variabile 'NO_RES_RANGE' dovrebbe essere usata invece di 'NO_PRIV_RANGE' Filter.cs 771

A quanto pare, la seconda condizione doveva essere scritta in questo modo:

(flags &(int)FilterFlag.NO_RES_RANGE ) ==(int)FilterFlag.NO_RES_RANGE

Ad ogni modo, questa opzione sembra più logica e chiara.

Solo un controllo in più nell'istruzione if

Diversifichiamo il nostro articolo con il solito codice ridondante:

internal static NumberInfo IsNumber(....)
{
  ....
  int num = AlphaNumericToDigit(c);

  // unexpected character:
  if (num <= 15)
  {
    if (l == -1)
    {
      if (   longValue < long.MaxValue / 16 
          || (   longValue == long.MaxValue / 16 
              && num <= long.MaxValue % 16))         // <=
      {
        ....
      }
      ....
    }
    ....
  }
  ....
}

Avviso V3063:una parte dell'espressione condizionale è sempre vera se viene valutata:num <=long.MaxValue % 16. Conversions.cs 994

In primo luogo, vorrei dire che il codice della funzione è notevolmente abbreviato per la leggibilità. Fare clic sul collegamento per visualizzare il IsNumber completo codice sorgente – ma vi avverto – non è di facile lettura. La funzione contiene più di 300 righe di codice. Sembra andare oltre l'accettato "uno schermo" :).

Passiamo all'avviso. Nel blocco esterno il valore di num la variabile è selezionata – deve essere minore o uguale a 15. Nel blocco interno num è selezionato:deve essere minore o uguale a long.MaxValue % 16 . In tal modo, il valore di questa espressione è 15:è facile da controllare. Il codice risulta controllare due volte quel num è minore o uguale a 15.

Questo avviso difficilmente indica un vero bug:qualcuno ha appena scritto un controllo extra. Forse è stato fatto apposta, ad esempio per facilitare la lettura di questo codice esatto. Sebbene l'uso di alcune variabili o costanti per memorizzare il risultato del confronto sembra essere un'opzione più semplice. Ad ogni modo, il costrutto è ridondante ed è compito dell'analizzatore statico segnalarlo.

Potrebbe esserci un valore nullo?

Gli sviluppatori spesso mancano i controlli per null . La situazione è particolarmente interessante quando una variabile è stata verificata in un punto della funzione e in un altro (dove può essere ancora null ) – l'hanno dimenticato o non lo hanno ritenuto necessario. E qui possiamo solo intuire se il controllo fosse ridondante o se in alcuni punti fosse mancato. Null i controlli non comportano sempre l'uso di operatori di confronto, ad esempio, il frammento di codice riportato di seguito mostra che lo sviluppatore ha utilizzato l'operatore condizionale nullo:

public static string get_parent_class(....)
{
  if (caller.Equals(default))
  {
    return null;
  }

  var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
  return tinfo.BaseType?.Name;
}

L'avviso V3105:la variabile 'tinfo' è stata utilizzata dopo essere stata assegnata tramite l'operatore condizionale nullo. NullReferenceException è possibile. Objects.cs 189

Secondo lo sviluppatore, Type.GetTypeFromHandle(caller) la chiamata può restituire null - Ecco perchè "?." è stato utilizzato per chiamare GetPhpTypeInfo . La documentazione dimostra che è possibile.

Sìì, "?." salva da un'eccezione. Se GetTypeFromHandle la chiamata restituisce null , quindi tinfo variabile viene inoltre assegnato null . Ma quando provi ad accedere a BaseType proprietà, viene generata un'altra eccezione. Molto probabilmente, nell'ultima riga manca un altro "?":

restituisci info? .TipoBase?.Nome;

Avviso irreversibile ed eccezioni

Preparati, in questa parte troverai una vera indagine...

Qui abbiamo un altro avviso relativo a null dai un'occhiata. L'innesco si è rivelato molto più eccitante di quanto sembrasse a prima vista. Dai un'occhiata al frammento di codice:

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context));
  }

  return context.HashAlgorithm;
}

L'avviso V3125:l'oggetto "contesto" è stato utilizzato dopo che è stato verificato rispetto a null. Righe di controllo:3138, 3133. Hash.cs 3138

Sì, la variabile è controllata per null , e quindi si verifica l'accesso alla proprietà senza alcuna verifica. Tuttavia, guarda cosa succede se il valore della variabile è null :

PhpException.ArgumentNull(nameof(context));

Sembra che se il contesto è uguale a null , il thread di esecuzione non arriva a HashAlgorithm accesso alla proprietà. Pertanto, questo codice è sicuro. È un falso positivo?

Naturalmente, l'analizzatore può commettere errori. Tuttavia, so che PVS-Studio è in grado di gestire tali situazioni:l'analizzatore avrebbe dovuto saperlo al momento dell'accesso a HashAlgorithm , il contesto la variabile non può essere uguale a null .

Ma cosa fa esattamente PhpException.ArgumentNull chiamare fare? Diamo un'occhiata:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

Hmm, sembra che qualcosa sia stato lanciato. Presta attenzione al primo argomento della chiamata:PhpError.Warning . Hmm, bene, passiamo al Lancio metodo:

public static void Throw(PhpError error, string formatString, string arg0)
{
  Throw(error, string.Format(formatString, arg0));
}

Fondamentalmente, non c'è niente di interessante qui, dai un'occhiata a un altro Tiro sovraccarico:

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

Ed ecco cosa stiamo cercando! Si scopre che sotto il cofano di questo intero sistema c'è PhpFatalErrorException . L'eccezione sembra essere generata occasionalmente.

In primo luogo, vale la pena esaminare i punti in cui i gestori di OnError l'evento è registrato. Possono anche generare eccezioni:sarebbe un po' inaspettato, ma non si sa mai. Esistono alcuni gestori e tutti sono correlati alla registrazione dei messaggi corrispondenti. Un gestore è nel file PhpHandlerMiddleware:

PhpException.OnError += (error, message) =>
{
  switch (error)
  {
    case PhpError.Error:
      logger.LogError(message);
      break;

    case PhpError.Warning:
      logger.LogWarning(message);
      break;

    case PhpError.Notice:
    default:
      logger.LogInformation(message);
      break;
  }
};

Altri due gestori sono nella classe PhpException:

// trace output
OnError += (error, message) =>
{
  Trace.WriteLine(message, $"PHP ({error})");
};

// LogEventSource
OnError += (error, message) =>
{
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    LogEventSource.Log.HandleFatal(message);
  }
  else
  {
    LogEventSource.Log.HandleWarning(message);
  }
};

Pertanto, i gestori di eventi non generano eccezioni. Quindi, torniamo al Tiro metodo.

public static void Throw(PhpError error, string message)
{
  OnError?.Invoke(error, message);

  // throw PhpFatalErrorException
  // and terminate the script on fatal error
  if ((error & (PhpError)PhpErrorSets.Fatal) != 0)
  {
    throw new PhpFatalErrorException(message, innerException: null);
  }
}

Come tutto è chiaro con OnError , diamo un'occhiata più da vicino alla condizione:

(error & (PhpError)PhpErrorSets.Fatal) != 0

L'errore il parametro memorizza il valore di PhpError enumerazione. In precedenza, abbiamo notato che l'errore il parametro riceve PhpError.Warning . Viene generata un'eccezione se il risultato dell'applicazione di "AND bit per bit" all'errore e PhpErrorSets.Fatal è diverso da zero.

Il PhpErrorSets.Fatal value è una "unione" di PhpError elementi di enumerazione creati dall'operazione "OR bit per bit":

Fatal =   PhpError.E_ERROR | PhpError.E_COMPILE_ERROR
        | PhpError.E_CORE_ERROR | PhpError.E_USER_ERROR

Di seguito puoi vedere i valori di tutti gli elementi di enumerazione menzionati in precedenza:

E_ERROR = 1,
E_WARNING = 2,
E_CORE_ERROR = 16,
E_COMPILE_ERROR = 64,
E_USER_ERROR = 256,
Warning = E_WARNING

L'errore e (PhpError)PhpErrorSets.Fatal l'operazione restituisce un valore diverso da zero solo se l'errore parametro ha uno dei seguenti valori o una loro combinazione:

PhpError.E_ERROR,
PhpError.E_COMPILE_ERROR,
PhpError.E_CORE_ERROR,
PhpError.E_USER_ERROR

Se l'errore contiene il PhpError.Warning valore uguale a PhpError.E_WARNING , il risultato dell'operazione "AND bit per bit" è zero. Quindi la condizione per lanciare PhpFatalErrorException non è soddisfatto.

Torniamo a PhpException.ArgumentNull metodo:

public static void ArgumentNull(string argument)
{
  Throw(PhpError.Warning, ErrResources.argument_null, argument);
}

Lo abbiamo scoperto quando PhpError.Warning il valore è passato, non ci sono eccezioni. Forse lo sviluppatore non voleva che l'eccezione fosse generata nei casi in cui un null imprevisto è passato. È solo...

static HashPhpResource ValidateHashResource(HashContext context)
{
  if (context == null)
  {
    PhpException.ArgumentNull(nameof(context)); // no exceptions
  }

  return context.HashAlgorithm; // context is potential null
}

Se PhpException.ArgumentNull non genera un'eccezione (che è inaspettata), quindi quando accediamo a HashAlgorithm proprietà, NullReferenceException avviene comunque!

Potresti chiedere:dovrebbe essere generata un'eccezione o no? Se dovrebbe, allora ha più senso usare la stessa PhpFatalErrorException . Se nessuno si aspetta un'eccezione qui, devi elaborare correttamente il null valore del contesto parametro. Ad esempio, puoi usare "?.". Ad ogni modo, l'analizzatore ha affrontato questa situazione e ha persino aiutato a capire il problema.

Un altro controllo extra? Ancora un'eccezione!

L'ultimo caso dimostra che aspettandosi un'eccezione, puoi ottenere un null imprevisto . Il frammento seguente mostra il caso opposto:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    return node.Value;
  else
    return PhpValue.Null;
}

L'avviso V3022:l'espressione 'node !=null' è sempre vera. Datastructures.cs 432

Bene, non c'è null ecco, allora così sia! Perché brontolare? Tuttavia, di solito null è previsto nei casi in cui qualcosa non va. Il codice mostra che questo è esattamente il caso. Ma l'analizzatore insiste sul fatto che non potrebbe esserci null .

Potresti pensare che sia tutto incentrato su Debug.Assert chiamare in questo caso. Nel bene e nel male, questa chiamata non influisce sugli avvisi dell'analizzatore.

Se non si tratta di Debug.Assert , allora di cosa si tratta? Perché l'analizzatore "pensa" a quel nodo non è mai uguale a null ? Diamo un'occhiata a GetNodeAtIndex metodo, che restituisce il valore scritto su nodo :

private LinkedListNode<PhpValue> GetNodeAtIndex(PhpValue index)
{
  return GetNodeAtIndex(GetValidIndex(index));
}

Bene, andiamo più a fondo. Dai un'occhiata a GetNodeAtIndex metodo chiamato qui:

private LinkedListNode<PhpValue> GetNodeAtIndex(long index)
{
  var node = _baseList.First;
  while (index-- > 0 && node != null)
  {
    node = node.Next;
  }

  return node ?? throw new OutOfRangeException();
}

Aspetto! Sembra che il metodo possa restituire null ... Non molta fortuna! Se il ciclo è terminato, e node è uguale a null , viene generata un'eccezione. In questo modo, nessun null può essere restituito.

In caso di una situazione imprevista, GetNodeAtIndex il metodo non restituisce null , come previsto in offsetGet codice metodo:

public PhpValue offsetGet(PhpValue offset)
{
  var node = GetNodeAtIndex(offset); // potential null expected

  Debug.Assert(node != null);

  if (node != null) // always true
    return node.Value;
  else
    return PhpValue.Null; // unreachable
}

Quando uno sviluppatore esamina questo metodo, può facilmente essere ingannato. Secondo il frammento di codice, sembra che il valore corretto o PhpValue.Null viene restituito. In effetti, questo metodo può generare un'eccezione.

Il comportamento inaspettato di un solo metodo nella catena di chiamate porta al comportamento inaspettato di tutti questi metodi:un tale piantagrane! Questo esempio illustra quanto sia utile l'analisi statica. Trova automaticamente tali problemi.

A proposito, c'è un problema simile in offsetSet metodo della stessa classe:

public void offsetSet(PhpValue offset, PhpValue value)
{
  var node = GetNodeAtIndex(offset);

  Debug.Assert(node != null);

  if (node != null)
    node.Value = value;
}

L'avviso V3022:l'espressione 'node !=null' è sempre vera. Datastructures.cs 444

Assegnazioni e riassegnazioni

Perché non prendiamo una piccola pausa da tutte queste indagini e non prendiamo una tazza di caffè?

Mentre beviamo un caffè, diamo un'occhiata a un semplice avviso che indica uno strano frammento di codice:

internal StatStruct(Mono.Unix.Native.Stat stat)
{
  st_dev = (uint)stat.st_dev;
  st_ctime = stat.st_ctime_nsec;
  st_mtime = stat.st_mtime_nsec;
  st_atime = stat.st_atime_nsec;
  st_ctime = stat.st_ctime;
  st_atime = stat.st_atime;
  //stat.st_blocks;
  //stat.st_blksize;
  st_mtime = stat.st_mtime;
  st_rdev = (uint)stat.st_rdev;
  st_gid = (short)stat.st_gid;
  st_uid = (short)stat.st_uid;
  st_nlink = (short)stat.st_nlink;
  st_mode = (FileModeFlags)stat.st_mode;
  st_ino = (ushort)stat.st_ino;
  st_size = stat.st_size;
}

Gli avvisi di PVS-Studio:

• V3008 Alla variabile 'st_ctime' vengono assegnati valori due volte in successione. Forse questo è un errore. Righe di controllo:78, 75. StatStruct.cs 78
• V3008 Alla variabile 'st_atime' vengono assegnati valori due volte di seguito. Forse questo è un errore. Linee di controllo:79, 77. StatStruct.cs 79

Sembra che lo sviluppatore si sia impigliato in tutti questi incarichi e abbia commesso un errore di battitura da qualche parte. Per questo, st_ctime e st_atime i campi ricevono i valori due volte e il secondo valore non è uguale al primo.

È un errore, vero? Ma non è divertente! Ti suggerisco di esercitare le tue abilità e cercare un significato più profondo. Quindi prova a spiegare nei commenti perché tutto è com'è.

Intanto andiamo avanti :)

Queste stringhe immutabili...

All'inizio di questo articolo, mentre stavi leggendo i primi avvisi, abbiamo menzionato l'immutabilità di DateTime istanze della struttura. I seguenti avvisi ci ricordano una funzione di stringhe simile:

public TextElement Filter(IEncodingProvider enc,
                          TextElement input,
                          bool closing)
{
  string str = input.AsText(enc.StringEncoding);

  if (pending)
  {
    if (str.Length == 0) str = "\r";
    else if (str[0] != '\n') str.Insert(0, "\r"); // <=
  }

  str = str.Replace("\r\n", "\n");
  if (str.Length != 0)
  {
    pending = str[str.Length - 1] == '\r';

    if (!closing && pending) str.Remove(str.Length - 1, 1); // <=
  }

    
  return new TextElement(str);
}

Gli avvisi di PVS-Studio:

• V3010 È necessario utilizzare il valore di ritorno della funzione 'Inserisci'. Filtri.cs 150
• V3010 È necessario utilizzare il valore di ritorno della funzione 'Rimuovi'. Filters.cs 161

Tutto è semplice e chiaro:volevamo modificare la stringa, ma qualcosa... è andato storto :(.

o lancia !=o null

Di recente, abbiamo analizzato un caso in cui uno sviluppatore si aspettava la funzione per restituire null ma invece ho ottenuto un'eccezione. Ecco qualcosa di simile ma più semplice:

public static bool stream_wrapper_register(....)
{
  // check if the scheme is already registered:
  if (   string.IsNullOrEmpty(protocol)
      || StreamWrapper.GetWrapperInternal(ctx, protocol) == null)
  {
    // TODO: Warning?
    return false;
  }

  var wrapperClass = ctx.GetDeclaredTypeOrThrow(classname, true);
  if (wrapperClass == null) // <=
  {
    return false;
  }

  ....
}

L'avviso V3022:l'espressione 'wrapperClass ==null' è sempre falsa. Streams.cs 555

Certo, puoi analizzarlo in dettaglio, ma... Il nome del metodo dice tutto! GetDeclaredTypeOrThrow sorta di suggerimenti che genererà un'eccezione se qualcosa va storto. Ancora una volta, ecco il punto:questo comportamento viene anche passato allo stream_wrapper_register metodo. Ma lo sviluppatore voleva che questo metodo restituisse false . Nessuna tale fortuna, ecco un'eccezione!

In effetti, abbiamo già incontrato nomi ingannevoli in precedenza. Ricordi quando il PhpException.ArgumentNull la chiamata al metodo in realtà non ha generato un'eccezione? Quindi, controlliamo se GetDeclaredTypeOrThrow genera un'eccezione:

PhpTypeInfo GetDeclaredTypeOrThrow(string name, bool autoload = false)
{
  return GetDeclaredType(name, autoload) ??
         throw PhpException.ClassNotFoundException(name);
}

Bene, gli sviluppatori di PeachPie non hanno cercato di ingannarti qui:è una vera eccezione :).

Strano 'mentre vero'

In alcuni casi, gli sviluppatori usano true valore come il mentre condizione di continuazione del ciclo. Sembra essere una cosa normale:per uscire dal ciclo, puoi usare break , ritorno, o eccezioni. In realtà, il ciclo che ha qualche espressione (invece di true parola chiave) in quanto una condizione sembra molto più che strana. Il valore di questa espressione ha sempre true valore:

public static int stream_copy_to_stream(...., int offset = 0)
{
  ....
  if (offset > 0)
  {
    int haveskipped = 0;

    while (haveskipped != offset)  // <=
    {
      TextElement data;

      int toskip = offset - haveskipped;
      if (toskip > from.GetNextDataLength())
      {
        data = from.ReadMaximumData();
        if (data.IsNull) break;
      }
      else
      {
        data = from.ReadData(toskip, false);
        if (data.IsNull) break; // EOF or error.
        Debug.Assert(data.Length <= toskip);
      }

      Debug.Assert(haveskipped <= offset);
    }
  }
  ....
}

L'avviso V3022:l'espressione 'haveskipped !=offset' è sempre vera. Streams.cs 769

I hanno saltato la variabile viene dichiarata prima del ciclo. Viene inizializzato con il valore 0. Questo valore rimane con esso... fino alla sua morte. Suona cupo ma è quello che è. In effetti, hanno saltato è una costante. Il valore dell'offset il parametro rimane lo stesso anche durante l'esecuzione del loop. E rimane lo stesso in qualsiasi punto della funzione (puoi verificarlo qui).

Lo sviluppatore ha pianificato di rendere sempre vera la condizione di continuazione del ciclo? Teoricamente è possibile. Ma dai un'occhiata più da vicino al ciclo. Il seguente compito sembra strano:

int toskip = offset - haveskipped;

Che senso ha se hanno saltato è sempre uguale a 0?

Qualcosa non va con il loop. O viene commesso un grave errore o tutti questi sono saltati le cose strane sono i resti di alcune vecchie idee incompiute.

data ==null &&genera NullReferenceException

Spesso, l'uso di operatori errati nelle condizioni porta a bug. C'è una situazione simile nel compilatore PHP:

public string ReadStringContents(int maxLength)
{
  if (!CanRead) return null;
  var result = StringBuilderUtilities.Pool.Get();

  if (maxLength >= 0)
  {
    while (maxLength > 0 && !Eof)
    {
      string data = ReadString(maxLength);
      if (data == null && data.Length > 0) break; // EOF or error.
      maxLength -= data.Length;
      result.Append(data);
    }
  }
  ....
}

L'avviso V3080:possibile dereferenziazione nulla. Prendi in considerazione l'ispezione dei "dati". PhpStream.cs 1382

Il valore dei dati la variabile è controllata nel ciclo. Se la variabile è uguale a null e la sua Lunghezza proprietà ha un valore positivo, quindi si verifica l'uscita dal ciclo. Chiaramente, è impossibile. Inoltre, abbiamo un'eccezione quando accediamo a Length variabile con null valore. Qui, l'accesso avviene deliberatamente quando data =null .

Dato il commento dello sviluppatore, riscriverei la condizione in questo modo:

data == null || data.Length == 0

Tuttavia, ciò non significa che questa sia l'opzione di gestione corretta:per risolvere questo problema, è meglio eseguire un'analisi approfondita del codice.

Eccezione errata

Ci sono anche bug che non sembrano così terribili ma possono comunque causare problemi. Ad esempio, nel frammento seguente, copia-incolla colpisce di nuovo:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

L'avviso V3013:è strano che il corpo della funzione 'addGlob' sia completamente equivalente al corpo della funzione 'addPattern' (506, riga 515). ZipArchive.cs 506

addGlob la funzione chiaramente non è supportata, quindi quando la funzione viene chiamata, c'è un'eccezione che indica che addGlob la funzione non è supportata.

Credimi? Ti ho ingannato! Non ci sono eccezioni qui. Questo è il nostro vecchio amico:PhpException :

public static class PhpException
{
  ....
  public static void FunctionNotSupported(string/*!*/function)
  {
    Debug.Assert(!string.IsNullOrEmpty(function));

    Throw(PhpError.Warning,
          ErrResources.notsupported_function_called,
          function);
  }
  ....
}

Come abbiamo discusso in precedenza, se il Lancio riceve il PhpError.Warning valore, non ci sono eccezioni. Tuttavia, è probabile che l'errore apparso venga aggiunto al registro o gestito in altro modo.

Torniamo al frammento di codice originale:

public bool addGlob(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addGlob));
  return false;
}

addGlob la funzione non è supportata e quando viene chiamato, il messaggio corrispondente viene gestito in qualche modo:supponiamo che venga aggiunto al registro. Il addPattern anche la funzione non è supportata, tuttavia, il messaggio corrispondente è ancora indirizzato a addGlob .

Chiaramente, è un errore di copia-incolla. È facile da risolvere:devi solo segnalare addPattern e non su addGlob in addPattern metodo:

public bool addPattern(....)
{
  PhpException.FunctionNotSupported(nameof(addPattern));
  return false;
}

Non incolpare String.Join!

A volte gli sviluppatori dimenticano le caratteristiche di alcune funzioni. Ecco perché controllano valori sbagliati. Di conseguenza, il controllo risulta privo di significato e non c'è controllo dove deve essere. Sembra che la stessa cosa sia successa ai getallheader funzione:

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    if (headers != null)
    {
      var result = new PhpArray(16);

      foreach (var h in headers)
      {
        result[h.Key] = string.Join(", ", h.Value) ?? string.Empty;
      }

      return result;
    }
  }

  return null;
}

L'avviso V3022:l'espressione 'string.Join(", ", h.Value)' non è sempre nulla. L'operatore '??' è eccessivo. Web.cs 932

È inutile usare il "???" operatore qui dal string.Join il metodo non restituisce mai null . Ma può sempre generare ArgumentNullException (Prego!).

string.Unisciti genera un'eccezione se il riferimento passato alla sequenza è null . Pertanto, è più sicuro scrivere in questa riga qualcosa del genere:

result[h.Key] = h.Value != null ? string.Join(", ",h.Value) : string.Empty;

In realtà, voglio sapere se è possibile per Valore essere nullo affatto? Forse, non dobbiamo controllare nulla qui. Per capirlo, in primo luogo, dobbiamo capire dove si trovano le intestazioni proveniva dalla raccolta.

public static PhpArray getallheaders(Context ctx)
{
  var webctx = ctx.HttpPhpContext;
  if (webctx != null)
  {
    var headers = webctx.RequestHeaders;
    ....
  }

  return null;
}

Le intestazioni il valore è preso da webctx.requestHeaders e il webctx il valore è preso da HttpPhpContext proprietà del ctx oggetto. E il HttpPhpContext proprietà... Dai un'occhiata a questo:

partial class Context : IEncodingProvider
{
  ....
  public virtual IHttpPhpContext? HttpPhpContext => null;
  ....
}

Questo, a quanto pare, è qualcosa lasciato per dopo. Se guardi le getallheader metodo di nuovo, vedi che non funziona mai e restituisce semplicemente null .

Credimi di nuovo? Ma la proprietà è virtuale! Pertanto, per capire cosa sono i getallheader metodo può restituire, è necessario analizzare i discendenti. Personalmente ho deciso di fermarmi a questo punto, devo ancora mostrare altri avvertimenti.

Piccolo compito in un metodo lungo

È probabile che metodi lunghi e complessi contengano bug. Nel tempo, è difficile per gli sviluppatori navigare in una grossa porzione di codice, mentre è sempre terrificante cambiarlo. I programmatori aggiungono un nuovo codice, quello vecchio rimane lo stesso. In qualche modo questo incredibile costrutto funziona, per fortuna. Quindi, nessuna sorpresa, c'è qualche stranezza in tale codice. Ad esempio, dai un'occhiata a inflate_fast metodo:

internal int inflate_fast(....)
{
  ....
  int r;
  ....
  if (c > e)
  {
    // if source crosses,
    c -= e; // wrapped copy
    if (q - r > 0 && e > (q - r))
    {
      do
      {
        s.window[q++] = s.window[r++];
      }
      while (--e != 0);
    }
    else
    {
      Array.Copy(s.window, r, s.window, q, e);
      q += e; r += e; e = 0;                     // <=
    }
    r = 0;                                       // <=
  }
  ....
}

L'avviso V3008:alla variabile 'r' vengono assegnati valori due volte con successo. Forse questo è un errore. Righe di controllo:621, 619. InfCodes.cs 621

Per cominciare, ecco un link al codice completo. Il metodo ha più di duecento righe di codice con un mucchio di costrutti nidificati. Sembra che sarebbe difficile risolverlo.

L'avviso non è ambiguo:in primo luogo, viene assegnato un nuovo valore a r variabile nel blocco, quindi viene definitivamente sovrascritta con zero. È difficile dire cosa c'è che non va esattamente qui. O l'annullamento funziona in qualche modo in modo errato o il r += e la costruzione è superflua qui.

dereferenziazione nulla in un'espressione booleana

In precedenza, abbiamo discusso il caso in cui un'espressione logica costruita in modo errato porta a un'eccezione. Ecco un altro esempio di tale avviso:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null && langVersion.Major > 7 
         || (langVersion.Major == 7 && langVersion.Minor >= 2);
}

L'avviso V3080:possibile dereferenziazione nulla. Prendi in considerazione l'ispezione di "langVersion". AnalysisFacts.cs 20

Il codice verifica che la langVersion passata il parametro non è uguale a null . Quindi, lo sviluppatore ha presupposto che null potrebbe essere passato durante la chiamata. Il controllo ti salva da un'eccezione?

Sfortunatamente, se la langVersion variabile è null , il valore della prima parte dell'espressione è false . Quando viene calcolata la seconda parte, viene generata un'eccezione.

In genere, per migliorare la leggibilità, è necessario formattare ulteriormente i frammenti di codice da pubblicare in un articolo. Questo caso non fa eccezione:l'espressione considerata in precedenza, infatti, era scritta come una riga:

Dato il commento, puoi facilmente capire che qui la precedenza dell'operatore è confusa o la parentesi è fuori posto. È molto probabile che il metodo abbia il seguente aspetto:

public static bool IsAutoloadDeprecated(Version langVersion)
{
  // >= 7.2
  return    langVersion != null 
         && (   langVersion.Major > 7 
             || langVersion.Major == 7 && langVersion.Minor >= 2);
}

Ecco fatto!

In realtà, no. L'analizzatore ha emesso circa 500 avvisi per l'intero progetto, e sono rimasti molti curiosi in attesa dell'indagine. Pertanto, ti suggerisco comunque di provare PVS-Studio e vedere cos'altro può trovare in questo o altri progetti. Chissà, forse riuscirai a trovare alcuni bug che sono ancora più eccitanti di tutti gli avvisi che ho risolto qui :). Non dimenticare di menzionare gli avvisi trovati nei commenti. I bug che hai trovato potrebbero entrare nella Top 10 del 2021!

Ti auguro buona fortuna!