Media Portal 2 è un software aperto di una classe media center, che consente all'utente di ascoltare musica, guardare video, visualizzare immagini e molto altro. Per noi, gli sviluppatori dell'analizzatore statico PVS-Studio, questa è un'altra possibilità per verificare un progetto interessante, raccontare alle persone (e agli sviluppatori) gli errori che troviamo e, naturalmente, dimostrare le capacità del nostro analizzatore.
Informazioni sul progetto Media Portal 2
Sul progetto Media Portal 2T, la descrizione del progetto tratta da Wikipedia:
MediaPortal fornisce un'interfaccia utente di 10 piedi per eseguire le tipiche funzionalità PVR/TiVo, inclusa la riproduzione, la messa in pausa e la registrazione di TV in diretta; riproduzione di DVD, video e musica; visualizzazione di immagini; e altre funzioni. I plug-in gli consentono di eseguire attività aggiuntive, come guardare video online, ascoltare musica da servizi online come Last.fm e avviare altre applicazioni come giochi. Si interfaccia con l'hardware che si trova comunemente negli HTPC, come sintonizzatori TV, ricevitori a infrarossi e display LCD.
Gran parte del progetto è scritto in C#. Ci sono unità separate scritte in C++. Inoltre, per quanto ho capito, gli sviluppatori di Media Portal 2 stanno già utilizzando ReSharper nel loro progetto. Ho tratto questa conclusione vedendone la menzione nel file .gitignore. Non ci piace l'idea di confrontare PVS-Studio e ReSharper, perché si tratta di diversi tipi di strumenti. Tuttavia, come puoi vedere, l'utilizzo di ReSharper non ci ha impedito di trovare veri e propri errori nel codice.
I risultati dell'analisi
Durante l'analisi abbiamo controllato 3321 file. In totale, c'erano 512.435 righe di codice. Come risultato del controllo abbiamo avuto 72 avvisi di alto livello. 57 di loro hanno indicato errori reali, refusi, problemi e strani frammenti nel codice. C'erano anche 79 avvisi di secondo livello (medio). A mio parere, 53 avvisi hanno indicato punti problematici o strani nel codice. Non esamineremo gli avvisi di livello più basso, perché questi avvisi di solito non indicano errori reali, hanno un numero piuttosto elevato di falsi positivi e contengono avvisi che non sono rilevanti per la maggior parte dei progetti.
Quindi l'analizzatore ha rilevato 0,2 errori per 1000 righe di codice. La percentuale di falsi positivi è solo del 27%, che è un ottimo risultato.
Dovrei dire subito che a volte è molto difficile determinare cosa esattamente il programmatore intendeva ottenere quando scriveva un particolare pezzo di codice. Ecco perché quei frammenti che ho considerato errati possono avere una logica distorta, ma nell'ambito di un certo algoritmo possono funzionare abbastanza normalmente. Ma se questo codice viene riutilizzato in un'altra applicazione, da un'altra persona che non è a conoscenza di tutte le sfumature dell'implementazione, molto probabilmente porterà a bug nel loro sistema.
Inoltre, voglio notare che l'articolo non copre tutti gli errori, poiché ce ne sono troppi per un singolo articolo.
Quindi, diamo un'occhiata ai bug più interessanti che abbiamo trovato; gli autori del progetto possono fare una revisione più dettagliata dei bug eseguendo il controllo del progetto da soli o richiedendo una licenza temporanea. Inoltre, cari lettori, se siete sviluppatori non commerciali o individuali, suggerisco di utilizzare la versione gratuita del nostro analizzatore statico. Le sue capacità funzionali sono assolutamente identiche alla versione a pagamento e quindi è la soluzione perfetta per studenti, singoli sviluppatori e team di appassionati.
Errori di battitura quando si utilizza il Copia-Incolla
Inizierò la descrizione con errori abbastanza diffusi quando un programmatore ha copiato un blocco di codice, ma ha dimenticato di modificare una o più variabili in esso per disattenzione.
V3127 Sono stati trovati due frammenti di codice simili. Forse si tratta di un errore di battitura e la variabile "AllocinebId" dovrebbe essere utilizzata invece di "CinePassionId" MovieRelationshipExtractor.cs 126
if (movie.CinePassionId > 0)
ids.Add(ExternalIdentifierAspect.SOURCE_CINEPASSION,
movie.CinePassionId.ToString());
if (movie.CinePassionId > 0) // <=
ids.Add(ExternalIdentifierAspect.SOURCE_ALLOCINE,
movie.AllocinebId.ToString());È molto difficile trovare tali bug eseguendo una semplice revisione del codice. Poiché il codice è molto bloccato insieme, molto probabilmente il programmatore non ha notato il difetto. Osservando la riga contrassegnata da un commento, noterai che la parola Allocine viene utilizzato al posto di CinePassion ovunque nel secondo blocco if, ma nella condizione del check la variabile CinePassionId non è stato sostituito con AllocinebId.
Il diagnostico V3127 ha riscontrato diversi errori di battitura più interessanti che mostrano il pericolo del Copia-Incolla.
V3127 Sono stati trovati due frammenti di codice simili. Forse questo è un errore di battitura e la variabile 'Y' dovrebbe essere usata invece di 'X' PointAnimation.cs 125
double distx = (to.X - from.X) / duration;
distx *= timepassed;
distx += from.X;
double disty = (to.X - from.Y) / duration; // <=
disty *= timepassed;
disty += from.Y;
V3127 Sono stati trovati due frammenti di codice simili. Forse questo è un errore di battitura e la variabile 'X' dovrebbe essere usata invece di 'Y' Point2DList.cs 935
double dx1 = this[upper].Y - this[middle].X; // <=
double dy1 = this[upper].Y - this[middle].Y;
V3127 Sono stati trovati due frammenti di codice simili. Forse questo è un errore di battitura e la variabile 'attrY' dovrebbe essere usata invece di 'attrX' AbstractSortByComparableValueAttribute.cs 94
if (attrX != null)
{
valX = (T?)aspectX.GetAttributeValue(attrX);
}
if (attrY != null)
{
valX = (T?)aspectX.GetAttributeValue(attrX); // <=
}In tutti i casi nel primo blocco le valutazioni sono sull'asse x; nel secondo blocco con l'asse Y. Se osserviamo le righe commentate, possiamo vedere che il programmatore ha dimenticato di cambiare X in Y o viceversa, quando copia e incolla uno dei blocchi.
Accesso tramite riferimento null
Il linguaggio di programmazione è in continua evoluzione, ma il modo principale per spararsi ai piedi rimane lo stesso. Nell'esempio del codice citato di seguito, il programmatore verifica prima il BannerPath variabile contro null. Se è null, verifica che sia uguale a una stringa vuota con un metodo Equals, che potrebbe causare una potenziale NullReferenceException .
V3080 Possibile dereferenziazione nulla. Prendi in considerazione l'ispezione di "BannerPath". TvdbBannerWithThumb.cs 91
if (ThumbPath == null &&
(BannerPath != null || BannerPath.Equals(""))) // <=
{
ThumbPath = String.Concat("_cache/", BannerPath);
}Questo frammento di codice è piuttosto strano, se prendiamo in considerazione il codice dopo il controllo. A mio avviso, il controllo dovrebbe essere attivato se la variabile BannerPath non è null e non è una stringa vuota.
La variante corretta può essere così:
if (ThumbPath == null &&
!string.IsNullOrEmpty(BannerPath))
{
ThumbPath = String.Concat("_cache/", BannerPath);
}Precedenza operatore errata
Suggerisco di dare un'occhiata a un altro avviso abbastanza divertente relativo alla precedenza errata degli operatori logici.
V3130 La priorità dell'operatore '&&' è maggiore di quella dell'operatore '||' operatore. Possibili parentesi mancanti. BinaryCacheProvider.cs 495
return config.EpisodesLoaded || !checkEpisodesLoaded &&
config.BannersLoaded || !checkBannersLoaded &&
config.ActorsLoaded || !checkActorsLoaded;Il programmatore che ha scritto questo codice apparentemente non ha tenuto conto del fatto che l'operatore AND logico (&&) ha una precedenza maggiore rispetto all'operatore OR logico (||). Ancora una volta, consiglierei di specificare esplicitamente la precedenza delle operazioni e di mettere tra parentesi.
Ecco un altro errore causato da una precedenza errata dell'operatore. Il programmatore ignora il fatto che l'operatore + ha una priorità maggiore rispetto a ?? operatore.
V3022 Espressione '"Nome intestazione non valido:" + nome' non è sempre nullo. L'operatore '??' è eccessivo. HttpRequest.cs 309
...("Invalid header name: " + name ?? "<null>");
Di conseguenza, se la variabile nome è zero, verrà aggiunto alla stringa "Nome intestazione non valido:" come stringa vuota e non verrà sostituita dall'espressione "
La variante corretta sarà la seguente.
...("Invalid header name: " + (name ?? "<null>"));Un errore di battitura dopo il casting del tipo
Un altro errore di battitura comune causato dalla disattenzione. Nota le variabili altro e oggetto .
V3019 È possibile che una variabile errata venga confrontata con null dopo la conversione del tipo utilizzando la parola chiave 'as'. Controlla le variabili 'obj', 'other'. EpisodeInfo.cs 560
EpisodeInfo other = obj as EpisodeInfo;
if (obj == null) return false; // <=
if (TvdbId > 0 && other.TvdbId > 0)
return TvdbId == other.TvdbId;
....In questo frammento di codice viene eseguito il cast di una variabile obj in modo esplicito a EpisodeInfo type e il risultato viene restituito alla variabile other. Si noti che più avanti vediamo una variabile other utilizzata, ma la variabile obj viene verificata rispetto a null. Nel caso in cui la variabile obj che avremo sia di un tipo diverso da quello a cui viene eseguito il cast, quindi lavorare con l'altra variabile porterà a un'eccezione.
Ecco come potrebbe apparire un frammento di codice fisso:
EpisodeInfo other = obj as EpisodeInfo;
if (other == null) return false;
if (TvdbId > 0 && other.TvdbId > 0)
return TvdbId == other.TvdbId;
....Doppio incarico
Un altro errore divertente che è stato trovato dall'analizzatore. Il seguente frammento di codice sarebbe privo di significato, perché Rilasciato variabile sarà sempre uguale a null.
V3008 Alla variabile 'Rilasciato' vengono assegnati valori due volte in successione. Forse questo è un errore. Righe di controllo:57, 56. OmDbSeasonEpisode.cs 57
DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
Released = releaseDate; // <=
Released = null; // <=Molto probabilmente questa affermazione con l'annullamento dovrebbe essere scritta nel blocco else. Quindi il frammento di codice corretto sarà questo:
DateTime releaseDate;
if (DateTime.TryParse(value, out releaseDate))
Released = releaseDate; // <=
else
Released = null; // <=Quando un minuto non sempre ha 60 secondi
V3118 Viene utilizzato il componente Milliseconds di TimeSpan, che non rappresenta l'intervallo di tempo completo. Probabilmente era invece previsto il valore "TotalMilliseconds". Default.cs 60
private void WaitForNextFrame()
{
double msToNextFrame = _msPerFrame -
(DateTime.Now - _frameRenderingStartTime).Milliseconds;
if (msToNextFrame > 0)
Thread.Sleep(TimeSpan.FromMilliseconds(msToNextFrame));
}Un altro errore di battitura abbastanza comune che si verifica a causa di TimeSpan tipo di implementazione. Ma a quanto pare, il programmatore non sapeva che i Secondi proprietà dell'oggetto di TimeSpan type restituisce non il numero totale di secondi in questo intervallo, ma il numero di secondi rimanenti.
Ad esempio, se l'intervallo di tempo è 1 minuto, 150 secondi, la chiamata dei Millisecondi il metodo restituirà solo 150 millisecondi. Se è necessario restituire un numero totale di secondi, dovremmo utilizzare il metodo TotalMilliseconds. Per questo esempio saranno 1150 millisecondi.
Quindi il codice corretto potrebbe essere il seguente:
double msToNextFrame = _msPerFrame -
(DateTime.Now - _frameRenderingStartTime).TotalMilliseconds;Ordine degli argomenti errato
Un altro errore causato dalla disattenzione. Il metodo TryCreateMultimediaCDDrof iveHandler ottiene enumerazioni di identificatori per video, immagini e audio nella sequenza specificata.
V3066 Possibile ordine errato degli argomenti passati al metodo 'TryCreateMultimediaCDDriveHandler'. RemovableMediaManager.cs 109
public static MultimediaDriveHandler
TryCreateMultimediaCDDriveHandler(DriveInfo driveInfo,
IEnumerable<Guid> videoMIATypeIds,
IEnumerable<Guid> imageMIATypeIds, // <=
IEnumerable<Guid> audioMIATypeIds) // <=
{ .... }Poiché questi parametri hanno gli stessi tipi, il programmatore non ha prestato attenzione al fatto che quando stava passando argomenti al metodo, ha smarrito immagini e audio:
public static ....()
{
MultimediaDriveHandler.TryCreateMultimediaCDDriveHandler(driveInfo,
Consts.NECESSARY_VIDEO_MIAS,
Consts.NECESSARY_AUDIO_MIAS, // <=
Consts.NECESSARY_IMAGE_MIAS) // <=
}Una condizione che è sempre falsa
Questo codice è piuttosto strano, quindi non ero sicuro di doverlo inserire qui o meno.
V3022 L'espressione 'IsVignetteLoaded' è sempre falsa. TvdbFanartBanner.cs 219
if (IsVignetteLoaded) // <=
{
Log.Warn(....);
return false;
}
try
{
if (IsVignetteLoaded) // <=
{
LoadVignette(null);
}
....Posso presumere che il primo controllo sia stato aggiunto per il debug e che molto probabilmente il programmatore si sia dimenticato di rimuoverlo. Di conseguenza blocca il secondo controllo che può portare a un'esecuzione errata del programma.
Controllo ridondante o errore grave?
V3001 Sono presenti sottoespressioni identiche 'screenWidth !=_screenSize.Width' a sinistra ea destra di '||' operatore. MainForm.cs 922
if (bitDepth != _screenBpp ||
screenWidth != _screenSize.Width ||
screenWidth != _screenSize.Width) // <=
{
....
}Presta attenzione all'ultimo controllo:molto probabilmente, il programmatore voleva controllare la larghezza e l'altezza, ma dopo il Copia-Incolla si è dimenticato di sostituire Larghezza con Altezza nell'ultimo controllo.
L'analizzatore ha trovato un altro bug simile:
V3001 Sono presenti sottoespressioni identiche 'p ==null' a sinistra ea destra di '||' operatore. Vincolo di triangolazione.cs 141
public static uint CalculateContraintCode(
TriangulationPoint p, TriangulationPoint q)
{
if (p == null || p == null) // <=
{
throw new ArgumentNullException();
}
....
}Osservando più a fondo il corpo del metodo, potresti notare che il p il parametro viene verificato rispetto a null due volte, allo stesso tempo la logica di questo metodo presuppone l'utilizzo di q parametro. Molto probabilmente, la parte destra del controllo dovrebbe contenere un controllo di q variabile invece di p .
Una condizione dimenticata e altro Copia-Incolla
Come avrai notato, la maggior parte degli errori in questo articolo sono causati dal copia-incolla, il seguente bug non fa eccezione.
V3003 È stato rilevato l'utilizzo del pattern 'if (A) {...} else if (A) {...}'. C'è una probabilità di presenza di un errore logico. Righe di controllo:452, 462. Scanner.cs 452
if (style == NumberStyles.Integer)
{
int ivalue;
if (int.TryParse(num, out ivalue))
return ivalue;
....
}
else if (style == NumberStyles.Integer) // <=
{
return double.Parse(num);
}In entrambi i controlli lo stile della variabile viene confrontato con lo stesso valore nell'enumerazione. Di conseguenza, il secondo controllo non verrà mai eseguito. Se teniamo presente il fatto che durante il primo controllo la stringa viene convertita in un numero intero, e nel secondo controllo in un numero in virgola mobile. Posso presumere che la condizione del secondo controllo dovrebbe essere la seguente:
....
}
else if (style == NumberStyles.Double) // <=
{
return double.Parse(num);
}Conclusione
In questo progetto sono stati rilevati molti altri errori, refusi e problemi. Ma non sembravano abbastanza interessanti da descrivere in questo articolo. In generale posso dire che la codebase del progetto è poco leggibile e contiene molti strani frammenti. La maggior parte non è stata citata nell'articolo, ma li considererei comunque un cattivo stile di codifica. Ciò potrebbe includere l'utilizzo di un foreach loop, per ottenere il primo elemento della raccolta ed uscire utilizzando l'interruzione alla fine della prima iterazione, numerosi controlli ridondanti, grandi blocchi di codice non separati, ecc.
Il Portale multimediale 2 gli sviluppatori possono trovare facilmente tutti i problemi, utilizzando lo strumento PVS-Studio. Potresti anche trovare bug nei tuoi progetti con l'aiuto dello strumento menzionato.
Vorrei ricordare che il più grande vantaggio dell'analisi statica si ottiene con un uso regolare. Non è sufficiente scaricare lo strumento ed eseguire un controllo una tantum. Per analogia, i programmatori rivedono regolarmente gli avvisi del compilatore, non solo 3 volte all'anno prima del rilascio. Se l'analizzatore viene utilizzato regolarmente, risparmierà molto tempo che di solito viene dedicato alla ricerca di errori di battitura ed errori.