Geekly articles weekly

Analyse du code source Emby avec l'analyseur PVS-Studio

Image 3

Emby est un serveur multimédia assez bien connu, avec Plex et Kodi. Dans cet article, nous allons essayer de vérifier son code source à l'aide de l'analyseur statique PVS-Studio. Sur le site officiel d'Emby, il y a une petite marque «Built with ReSharper». Eh bien, c'est encore plus intéressant.

Analyseur de code PVS-Studio


PVS-Studio fonctionne sur des systèmes 64 bits sous Windows, Linux et macOS. Il sait rechercher les erreurs dans le code source des programmes écrits en C, C ++, C # et Java.

Projet audité


Emby est un serveur multimédia dont le code source se trouve sur GitHub . Il vous permet d'organiser des diffusions et permet d'accéder au contenu multimédia (vidéo, audio, photos) depuis n'importe quel appareil. Squeeze depuis le site officiel d' Emby :

  • Emby convertit automatiquement vos médias à la volée pour les lire sur n'importe quel appareil;
  • Des contrôles parentaux étendus facilitent le contrôle de l'accès au contenu, qui sera pertinent pour les familles avec de jeunes enfants;
  • Emby organise vos médias en présentations simples et élégantes. Ils ne se ressembleront jamais;
  • Streaming de données avec synchronisation dans le cloud;
  • Partagez facilement vos fichiers multimédias avec vos amis et votre famille;
  • Et bien plus.

Morceaux de code qui ont retenu l'attention lors de l'examen d'un rapport d'analyseur


PVS-Studio Warning: V3001 Il existe des sous-expressions identiques 'c! =' <'' À gauche et à droite de l'opérateur '&&'. HttpListenerRequest.Managed.cs 49

internal void SetRequestLine(string req) { .... if ((ic >= 'A' && ic <= 'Z') || (ic > 32 && c < 127 && c != '(' && c != ')' && c != '<' && // <= c != '<' && c != '>' && c != '@' && c != ',' && c != ';' && // <= c != ':' && c != '\\' && c != '"' && c != '/' && c != '[' && c != ']' && c != '?' && c != '=' && c != '{' && c != '}')) continue; .... }

L'analyseur a détecté une sous-expression en double c! = '<' . Peut-être qu'une erreur s'est glissée dans ce code, et quelque chose d'autre aurait dû être à la place de «<» . Ou, plus probablement, la deuxième sous-expression est superflue et il vous suffit de la supprimer.

Avertissement PVS-Studio: V3001 Il existe des sous-expressions identiques «SmbConstants.AttrHidden» à gauche et à droite de «|» opérateur. SmbComDelete.cs 29

 internal SmbComDelete(string fileName) { Path = fileName; Command = SmbComDelete; _searchAttributes = SmbConstants.AttrHidden | SmbConstants.AttrHidden | SmbConstants.AttrSystem; }

L'analyseur a de nouveau trouvé une faute de frappe associée à des sous-expressions répétées. Pour l'avenir, je dirai que dans ce projet, il y a un peu trop d'erreurs similaires faites par négligence. Je ne reproche pas du tout aux créateurs d'Emby, nous sommes parfois parfois dispersés de cette façon ( exemples , exemples , exemples ), et pour être à l'abri de telles erreurs, il n'y a qu'une analyse statique.

PVS-Studio Warning: V3004 L' instruction 'then' est équivalente à l'instruction 'else'. SqliteItemRepository.cs 5648

 private QueryResult<Tuple<BaseItem, ItemCounts>> GetItemValues(....) { .... if (typesToCount.Length == 0) { whereText += " And CleanName In (Select CleanValue from ItemValues where " + typeClause + " AND ItemId in (select guid from TypedBaseItems" + innerWhereText + "))"; } else { //whereText += " And itemTypes not null"; whereText += " And CleanName In (Select CleanValue from ItemValues where " + typeClause + " AND ItemId in (select guid from TypedBaseItems" + innerWhereText + "))"; } .... }

Et cela est très similaire au problème qui est apparu en raison du copier-coller, car si et sinon les corps de bloc sont complètement identiques. Pourquoi avez-vous eu besoin de vérifier les tailles du tableau typesToCount si, quel que soit le nombre d'éléments qui y sont écrits, le même code sera exécuté - il n'est connu que des développeurs.

PVS-Studio Warning: V3005 La variable '_validProviderIds' est assignée à elle-même. BaseNfoParser.cs 77

 private Dictionary<string, string> _validProviderIds; .... public void Fetch(....) { .... _validProviderIds = _validProviderIds = new Dictionary<....>(....); .... }

Encore une fois, une faute de frappe, qui impliquait l'affectation d'une variable à elle-même. Il vaut la peine de revérifier ce morceau de code.

PVS-Studio Warning: V3008 La variable 'Chapitres' se voit attribuer des valeurs deux fois de suite. C'est peut-être une erreur. Lignes de contrôle: 29, 28. Title.cs 29

 public Title(uint titleNum) { ProgramChains = new List<ProgramChain>(); Chapters = new List<Chapter>(); Chapters = new List<Chapter>(); TitleNumber = titleNum; }

Inattention et fautes de frappe, encore une fois ... La variable Chapitres se voit attribuer une valeur deux fois. Il est clair que ce code supplémentaire ne menace rien de mauvais, mais néanmoins. Cela n'a aucun sens de s'attarder sur cet exemple, passons à autre chose.

Avertissement PVS-Studio: V3013 Il est étrange que le corps de la fonction 'Lecture' soit entièrement équivalent au corps de la fonction 'Ecriture' (407, ligne 415). BaseSqliteRepository.cs 407

 public static IDisposable Read(this ReaderWriterLockSlim obj) { return new WriteLockToken(obj); } public static IDisposable Write(this ReaderWriterLockSlim obj) { return new WriteLockToken(obj); } private sealed class WriteLockToken : IDisposable { private ReaderWriterLockSlim _sync; public WriteLockToken(ReaderWriterLockSlim sync) { _sync = sync; sync.EnterWriteLock(); } public void Dispose() { if (_sync != null) { _sync.ExitWriteLock(); _sync = null; } } }

Les corps des fonctions de lecture et d' écriture sont absolument identiques les uns aux autres, comme en témoigne le texte d'avertissement de l'analyseur. La méthode EnterWriteLock est nécessaire pour entrer le verrou en mode écriture. Pour le mode de lecture, il vaut la peine d'utiliser la méthode EnterReadLock , qui permet de lire une ressource simultanément dans plusieurs threads.

Le développeur devrait examiner de plus près cette section du code, il y a peut-être une erreur. De plus, la classe suivante a été rencontrée dans le code, qui n'est pas utilisée:

 private sealed class ReadLockToken : IDisposable { private ReaderWriterLockSlim _sync; public ReadLockToken(ReaderWriterLockSlim sync) { _sync = sync; sync.EnterReadLock(); } public void Dispose() { if (_sync != null) { _sync.ExitReadLock(); _sync = null; } } }

PVS-Studio Warning: V3021 Il existe deux instructions 'if' avec des expressions conditionnelles identiques. La première instruction «if» contient la méthode return. Cela signifie que la deuxième instruction «si» est insensée SkiaEncoder.cs 537

 public string EncodeImage(string inputPath, DateTime dateModified, string outputPath, bool autoOrient, ImageOrientation? orientation, int quality, ImageProcessingOptions options, ImageFormat selectedOutputFormat) { if (string.IsNullOrWhiteSpace(inputPath)) { throw new ArgumentNullException("inputPath"); } if (string.IsNullOrWhiteSpace(inputPath)) { throw new ArgumentNullException("outputPath"); } .... }

Ici, apparemment, le développeur a copié les quatre premières lignes, mais a oublié de changer la variable testée de inputPath en outputPath . Il y a plusieurs lignes dans le code qui utilisent outputPath sans vérifier la valeur null , ce qui peut entraîner la levée d'une exception.

Avertissements de PVS-Studio:

  • V3022 L'expression 'processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null)' est toujours false. WebSocket.cs 462
  • V3022 L'expression 'processCloseFrame (frame)' est toujours fausse. WebSocket.cs 461
  • V3022 Expression 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'est toujours false. WebSocket.cs 460
  • V3022 L'expression 'processPongFrame (frame)' est toujours vraie. WebSocket.cs 459
  • V3022 L'expression 'processPingFrame (frame)' est toujours vraie. WebSocket.cs 457
  • V3022 L'expression 'processDataFrame (frame)' est toujours vraie. WebSocket.cs 455
  • V3022 L'expression est toujours fausse. WebSocket.cs 448

 private bool processWebSocketFrame(WebSocketFrame frame) { return frame.IsCompressed && _compression == CompressionMethod.None ? processUnsupportedFrame(....) : frame.IsFragmented ? processFragmentedFrame(frame) : frame.IsData ? processDataFrame(frame) : frame.IsPing ? processPingFrame(frame) : frame.IsPong ? processPongFrame(frame) : frame.IsClose ? processCloseFrame(frame) : processUnsupportedFrame(....); } private bool processUnsupportedFrame(....) { processException(....); return false; } private bool processDataFrame(WebSocketFrame frame) { .... return true; } private bool processPingFrame(WebSocketFrame frame) { var mask = Mask.Unmask; return true; } private bool processPongFrame(WebSocketFrame frame) { _receivePong.Set(); return true; } private bool processCloseFrame(WebSocketFrame frame) { var payload = frame.PayloadData; close(payload, !payload.ContainsReservedCloseStatusCode, false); return false; }

Dans mon compte, il n'y en a pas tellement, en comparaison avec le reste de l'équipe PVS-Studio, vérifié par l'analyseur de projet, et peut-être que cela est en quelque sorte lié au fait que je vois d'abord que pas moins de 13 avertissements d'analyseur sont émis sur 13 lignes de code (un peu plus que toutes les deux lignes). Par conséquent, je pense que ce code mérite d'être inclus dans l'article. Ensuite, je vais écrire ce que l'analyseur jure spécifiquement.

  1. Tout d'abord, l'expression frame.IsCompressed && _compression == CompressionMethod.None est prise en compte et, si l'expression est vraie, la méthode processUnsupportedFrame sera exécutée, ce qui en tout cas renvoie false (d'où le premier avertissement de l'analyseur). Si c'était faux, passez au point suivant.
  2. Ensuite, la valeur de frame.IsFragmented est vérifiée . Il n'y a aucun problème, alors passons à autre chose.
  3. Ici, la valeur de frame.IsData est vérifiée . Si vrai, la méthode processDataFrame retournera quand même vrai . D'où le deuxième avertissement de l'analyseur.
  4. Ensuite, nous vérifions frame.IsPing . True - processPingFrame renvoie true . Il s'agit du troisième avertissement de l'analyseur.
  5. Nous regardons frame.IsPong . Tout est similaire au précédent.
  6. Et le dernier: frame.IsClose . processCloseFrame et processUnsupportedFrame retournent dans tous les cas false .

Image 1

J'espère que je ne t'ai pas ennuyé. Ce sera encore plus simple.

Avertissement PVS-Studio: V3085 Le nom du champ 'RtpHeaderBytes' dans un type imbriqué est ambigu. Le type externe contient un champ statique avec un nom identique. HdHomerunUdpStream.cs 200

 public class HdHomerunUdpStream : LiveStream, IDirectStreamProvider { .... private static int RtpHeaderBytes = 12; public class UdpClientStream : Stream { private static int RtpHeaderBytes = 12; private static int PacketSize = 1316; private readonly MediaBrowser.Model.Net.ISocket _udpClient; bool disposed; .... } .... }

La classe imbriquée UdpClientStream utilise un champ avec un nom similaire à celui utilisé dans la classe externe HdHomerunUdpStream . Ce n'est pas une erreur, mais c'est une bonne raison de vérifier si tout va bien avec ce code. Son problème est qu'il y a une chance de confondre ces deux variables, et le code ne fonctionnera pas comme prévu par le développeur, bien qu'il ne provoque pas de plaintes du compilateur.

Avertissements de PVS-Studio:

  • V3090 Verrouillage non sécurisé sur un type. Toutes les instances d'un type auront le même objet 'Type'. Lmhosts.cs 49
  • V3090 Verrouillage non sécurisé sur un type. Toutes les instances d'un type auront le même objet 'Type'. Lmhosts.cs 57

 public class Lmhosts { public static NbtAddress GetByName(string host) { lock (typeof(Lmhosts)) { return GetByName(new Name(host, 0x20, null)); } } internal static NbtAddress GetByName(Name name) { lock (typeof(Lmhosts)) { .... } } }

L'analyseur avertit qu'un verrou non sécurisé est utilisé dans ce fragment de code. L'utilisation de verrou est indésirable car l'objet utilisé pour le verrouillage est partagé et peut être utilisé pour verrouiller ailleurs à l'insu du développeur qui a utilisé cet objet pour la première fois. Le résultat sera un blocage sur le même objet.

Idéalement, vous devez utiliser un champ privé pour le verrouillage, par exemple, comme ceci:

 private Object locker = new Object(); public static NbtAddress GetByName(string host) { lock (locker) { return GetByName(new Name(host, 0x20, null)); } }

Avertissement PVS-Studio: V3142 Code inaccessible détecté. Il est possible qu'une erreur soit présente. HdHomerunHost.cs 621

 protected override async Task<ILiveStream> GetChannelStream(....) { .... var enableHttpStream = true; if (enableHttpStream) { mediaSource.Protocol = MediaProtocol.Http; var httpUrl = channelInfo.Path; // If raw was used, the tuner doesn't support params if (!string.IsNullOrWhiteSpace(profile) && !string.Equals(profile, "native", StringComparison.OrdinalIgnoreCase)) { httpUrl += "?transcode=" + profile; } mediaSource.Path = httpUrl; return new SharedHttpStream(....); } return new HdHomerunUdpStream(....); }

L'analyseur indique que la dernière ligne de ce fragment de code ne sera jamais exécutée. Et pourquoi le développeur déclare et définit la variable enableHttpStream sur true, puis vérifie immédiatement cette variable dans l'instruction if?

Peut-être que ce code est simplement redondant. Dans tous les cas, cela mérite une double vérification.

Avertissement PVS-Studio: V3083 Appel non sécurisé de l'événement 'RefreshStarted', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. ProviderManager.cs 943

 public void OnRefreshStart(BaseItem item) { .... if (RefreshStarted != null) { RefreshStarted(this, new GenericEventArgs<BaseItem>(item)); } }

Cette méthode utilise un appel potentiellement dangereux au gestionnaire d' événements RefreshStarted , comme l'indique l'analyseur.

Je vais maintenant expliquer pourquoi l'analyseur a considéré cet appel RefreshStarted dans le code comme non sûr. Supposons, au moment entre la vérification de null et l'appel direct du gestionnaire d'événements dans le corps if , une désinscription de l'événement dans un autre thread est effectuée. Et, s'il n'y a plus d'abonnés, RefreshStarted sera nul . Mais dans un autre thread, où une vérification nulle s'est déjà produite, la ligne suivante sera exécutée:
 RefreshStarted(this, new GenericEventArgs<BaseItem>(item));

Cela lèvera une exception NullReferenceException .

Avertissement PVS-Studio: V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 142, 152. LocalImageProvider.cs 142

 private void PopulateImages(....) { .... // Logo if (!isEpisode && !isSong && !isPerson) { added = AddImage(....); if (!added) { added = AddImage(....); } } // Art if (!isEpisode && !isSong && !isPerson) { AddImage(....); } .... }

Dans ce morceau de code côte à côte se trouvent deux instructions if avec les mêmes conditions. Les corps des instructions if sont différents. Difficile de dire s'il s'agit d'une erreur ou d'un code redondant. Peut-être que tout va bien et que l'auteur voulait juste séparer logiquement deux actions différentes, dont l'une est associée à un certain «Logo» et la seconde à un certain «Art».

PVS-Studio Warning: V3041 L'expression a été implicitement convertie du type 'int' en type 'double'. Pensez à utiliser un transtypage de type explicite pour éviter la perte d'une partie fractionnaire. Un exemple: double A = (double) (X) / Y;. LiveTvManager.cs 1085

 private async Task RefreshChannelsInternal(....) { .... double progressPerService = _services.Length == 0 ? 0 : 1 / _services.Length; .... }

Ce code contient l'opération de division des types de données entiers. La valeur résultante est implicitement convertie en un type à virgule flottante, ce qui est extrêmement suspect.

En fait, la variable progressPerService ne sera égale à 1.0 que si _services.Length = 1 . Pour toute autre valeur _services.Length , le résultat sera 0,0 .

Apparemment, il devrait être écrit ici:

 double progressPerService = _services.Length == 0 ? 0 : (double)1 / _services.Length;


Avertissement PVS-Studio: V3050 Peut-être un HTML incorrect. La balise de fermeture </u> a été rencontrée, tandis que la balise </i> était attendue. SrtParserTests.cs 64

 public void TestParse() { var expectedSubs = new SubtitleTrackInfo { TrackEvents = new SubtitleTrackEvent[] { .... new SubtitleTrackEvent { Id = "6", StartPositionTicks = 330000000, EndPositionTicks = 339990000, Text = "This contains nested <b>bold, <i>italic, <u>underline</u> and <s>strike-through</s></u></i></b> HTML tags" }, .... } }; }

Faites attention à la ligne

 <u>underline</u>

Il a déjà une balise de fermeture </u> . Ensuite, nous voyons ce texte:
 </s></u></i></b> HTML tags"

Ici, la balise de fermeture </u> est superflue, comme l'indique l'analyseur.
Avertissement PVS-Studio: V3051 Une vérification de type excessive. L'objet est déjà du type 'Exception'. SmbFileInputStream.cs 107

 protected internal virtual IOException SeToIoe(SmbException se) { IOException ioe = se; Exception root = se.GetRootCause(); if (root is TransportException) { ioe = (TransportException)root; root = ((TransportException)ioe).GetRootCause(); } if (root is Exception) { ioe = new IOException(root.Message); ioe.InitCause(root); } return ioe; }

Et ici, franchement, ce n'est pas tout à fait clair ce que le développeur voulait généralement réaliser. L'analyseur avertit que la deuxième instruction if vérifie la compatibilité de l'objet racine avec son propre type. Il s'agit très probablement d'un code redondant, mais il semble suspect, le développeur devrait y regarder de plus près.

Conclusion


Dans tous les sens, les développeurs ont fait un excellent travail en créant Emby (le projet a 215 539 lignes de code, 4,6% - commentaires). Ils sont super, vraiment. Cependant, l'analyseur a également essayé en émettant des avertissements des niveaux élevé - 113, moyen - 213 et bas - 112. Certains d'entre eux étaient des faux positifs, mais la plupart des erreurs n'apparaissaient pas dans l'article parce qu'elles se ressemblaient.

Par exemple, les avertissements V3022 seuls (l'expression est toujours fausse / vraie) ont marqué 106 pièces. Vous pouvez, bien sûr, vérifier quelle partie d'entre eux était de faux positifs et ajouter le reste à l'article, mais cela se révélera un texte très ennuyeux que personne ne lira.

J'espère avoir réussi à démontrer l'utilité de l'analyseur statique dans le développement. Bien sûr, les contrôles ponctuels ne suffisent pas et un analyseur statique doit être utilisé de façon continue. Vous pouvez en savoir plus à ce sujet dans l'article Godot: Sur l'utilisation régulière des analyseurs de code statique .



Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Ekaterina Nikiforova. Vérification d'Emby avec PVS-Studio .

Source: https://habr.com/ru/post/fr484050/

More articles:


All Articles