Geekly articles weekly

Vérification d'Emby avec PVS-Studio

Image 3

Emby est un serveur multimédia assez populaire avec Plex et Kodi. Dans cet article, nous allons discuter des bogues trouvés dans son code source avec l'analyseur statique PVS-Studio. La remarque "Construit avec ReSharper" sur le site officiel du projet rend l'analyse encore plus intéressante.

PVS-Studio


PVS-Studio fonctionne sur les systèmes Windows, Linux et macOS 64 bits. Il peut détecter des bogues dans le code source d'un logiciel écrit en C, C ++, C # et Java.

Le projet en cours d'analyse


Emby est un serveur multimédia; son code source est disponible sur GitHub . Il permet à l'utilisateur de diffuser et d'accéder à son contenu multimédia (vidéo, audio, photos) sur n'importe quel appareil. Voici un bref résumé des fonctionnalités d' Emby selon le site officiel du projet:

  • Emby convertit et diffuse automatiquement vos médias à la volée pour les lire sur n'importe quel appareil;
  • Options de contrôle parental étendues pour un contrôle d'accès au contenu facile, ce qui est une caractéristique importante pour les familles avec de petits enfants;
  • Emby organise votre contenu en présentations simples et élégantes. Vos médias personnels ne seront plus jamais les mêmes;
  • Streaming avec prise en charge de la synchronisation cloud;
  • Partage facile du contenu avec vos amis et votre famille;
  • Et bien plus.

Les extraits de code les plus intéressants rapportés par l'analyseur


Message de diagnostic PVS-Studio: 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! = '<' . Une explication est qu'il s'agit d'une erreur de programmation et que le développeur voulait écrire autre chose à la place de '<' . Une autre explication, plus probable, est que la deuxième sous-expression n'était pas censée être là du tout et devrait être supprimée.

Message de diagnostic 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; }

Une autre faute de frappe qui a à voir avec les sous-expressions en double. En remarque, il y a trop de problèmes comme celui-ci dans le code source d'Emby - des erreurs causées par l'inattention. Je ne blâme cependant pas les développeurs; nous pouvons tous être distraits parfois ( exemples , exemples , exemples ), et c'est exactement pourquoi l'analyse statique existe - pour nous protéger de nos propres erreurs.

Message de diagnostic PVS-Studio: 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 celui-ci ressemble beaucoup à un bogue copier-coller parce que les blocs if et else ont les mêmes corps. À quoi bon vérifier la taille du tableau typesToCount si cela n'affecte pas la logique d'exécution suivante? C'est quelque chose que seuls les auteurs savent.

Message de diagnostic PVS-Studio: V3005 La variable '_validProviderIds' est affectée à elle-même. BaseNfoParser.cs 77

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

Une autre faute de frappe qui se traduit par l'attribution d'une variable à sa propre valeur. Ce code doit être révisé.

Message de diagnostic PVS-Studio: V3008 La variable 'Chapitres' reçoit des valeurs successives deux fois. 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; }

Il s'agit à nouveau d'inattention et de fautes de frappe ... Les variables des chapitres se voient attribuer une valeur deux fois. Bien sûr, cette tâche en double ne fera aucun mal, mais vous ne voulez toujours pas de choses comme ça dans votre code. Inutile de s'attarder sur celui-ci, alors passons à autre chose.

Message de diagnostic 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 fonctions de lecture et d' écriture ont les mêmes corps, et c'est ce que l'analyseur nous dit. La méthode EnterWriteLock est utilisée pour entrer le verrou en mode écriture. Si vous souhaitez entrer le verrou en mode lecture, utilisez la méthode EnterReadLock , qui permet à une ressource d'être lue par plusieurs threads à la fois.

Les développeurs devraient vérifier ce code car il est très probable qu'il contienne un bug - d'autant plus qu'il y a une classe inutilisée trouvée dans le code:

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

Message de diagnostic PVS-Studio: 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"); } .... }

Le développeur doit avoir cloné les quatre premières lignes mais a oublié de changer le nom de la variable vérifiée de inputPath en outputPath . Il y a plusieurs lignes plus loin où outputPath est utilisé sans vérification nulle préalable, ce qui signifie qu'une exception peut être levée.

Messages de diagnostic 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; }

J'ai vérifié moins de projets que mes coéquipiers PVS-Studio jusqu'à présent, et cela explique probablement pourquoi je n'ai jamais vu auparavant d'extrait de code de 13 lignes qui déclencherait 7 avertissements à la fois (c'est-à-dire un peu plus d'un avertissement pour deux lignes). C'est pourquoi j'inclus ce cas dans l'article. Vous trouverez ci-dessous une analyse étape par étape du fragment de problème.

  1. L'expression frame.IsCompressed && _compression == CompressionMethod.None est évaluée en premier. Si c'est vrai, la méthode processUnsupportedFrame s'exécutera et retournera false dans tous les cas (c'est le premier avertissement). Si la vérification est fausse , nous passons à la suivante.
  2. La valeur frame.IsFragmented est vérifiée. Aucun problème ici.
  3. La valeur frame.IsData est vérifiée. Si c'est vrai, la méthode processDataFrame retournera true dans tous les cas. Ceci est le deuxième avertissement.
  4. La valeur frame.IsPing est vérifiée. Si c'est vrai, la méthode processPingFrame retournera vrai . Ceci est le troisième avertissement.
  5. La valeur frame.IsPong est vérifiée. Identique à la précédente.
  6. Le dernier: frame.IsClose . processCloseFrame et processUnsupportedFrame renvoient false dans tous les cas.

Image 1

J'espère que ce n'était pas trop fastidieux à suivre. Les exemples restants ne sont pas si compliqués.

Message de diagnostic 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 possède un champ dont le nom est identique à celui d'un champ de la classe englobante HdHomerunUdpStream . Ce n'est pas un bug mais c'est une bonne raison de vérifier à nouveau ce code pour s'assurer qu'il est correct. Avoir des variables avec des noms identiques facilite l'utilisation accidentelle de l'une d'entre elles au lieu de l'autre, ce qui entraîne un comportement inattendu du programme, tandis que le compilateur ne dira pas un mot.

Messages de diagnostic 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 met en garde contre un verrouillage dangereux ici. L'utilisation de verrou d'une manière comme celle-ci n'est pas recommandée car l'objet verrou est accessible au public et peut être verrouillé à un autre endroit, et le développeur qui a utilisé cet objet pour la première fois peut ne jamais le savoir. Cela peut conduire à un blocage.

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

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

Message de diagnostic 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 cet extrait ne s'exécutera jamais. Et quel est le but de déclarer la variable enableHttpStream, de lui attribuer la valeur true et de la vérifier juste après?

Peut-être que ce code est simplement redondant, mais il doit quand même être révisé.

Message de diagnostic 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)); } }

L'analyseur nous avertit d'un appel potentiellement dangereux du gestionnaire d'événements RefreshStarted .

Voyons pourquoi cet appel n'est pas sûr. Supposons que l'événement soit désabonné dans un autre thread pour le moment entre la vérification de l'événement pour null et l'appel du gestionnaire d'événements dans le corps de l'instruction if . S'il n'y a plus d'abonnés, l'événement RefreshStarted deviendra nul , mais dans le thread où la vérification nulle a déjà été passée, l'appel sera quand même exécuté:

 RefreshStarted(this, new GenericEventArgs<BaseItem>(item));

Cela entraînera le lancement d'une exception NullReferenceException .

Message de diagnostic 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(....); } .... }

Les deux déclarations if ont des conditions identiques, mais leurs corps sont différents. Je ne sais pas s'il s'agit d'un bogue ou simplement d'un code redondant. Peut-être que c'est OK et le développeur voulait simplement différencier explicitement entre deux actions, dont l'une a à voir avec "Logo" et l'autre avec "Art", quelles qu'elles soient.

Message de diagnostic PVS-Studio: 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 une division entière, la valeur résultante étant convertie en un type à virgule flottante, ce qui ne semble pas une bonne chose à faire.

En fait, la variable progressPerService n'aura la valeur 1.0 que si _services.Length = 1 . Avec toute autre valeur de _services.Length , le résultat sera 0,0 .

Je pense que ce qui devrait être écrit à la place est le suivant:

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

Message de diagnostic 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" }, .... } }; }

Notez cette ligne:

 <u>underline</u>

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

Il y a une balise de fermeture supplémentaire </u> ici, ce que l'analyseur souligne.

Message de diagnostic 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; }

Franchement, je ne comprends pas très bien ce que le développeur voulait dire par ce code. L'analyseur indique que la seconde condition de l' instruction if vérifie si l'objet racine est compatible avec son propre type. Il s'agit probablement d'un code redondant, mais il semble étrange et je recommande de le réviser.

Conclusion


Les développeurs d'Emby ont fait un excellent travail à tous points de vue (le projet est long 215 539 LOC, dont 4,6% sont des commentaires). Ils ont bien fait, je le pense. Mais PVS-Studio mérite également des éloges: il a généré 113 avertissements de haut niveau , 213 de niveau moyen et 112 de bas niveau. Certains d'entre eux étaient des faux positifs, mais la plupart des bugs n'étaient pas mentionnés ici car ils étaient à peu près les mêmes. Par exemple, le diagnostic V3022 (toujours condition fausse / vraie) a été déclenché seul 106 fois. Bien sûr, j'aurais pu filtrer les faux positifs et inclure le reste dans l'article, mais cela serait devenu trop ennuyeux à lire.

J'espère avoir réussi à montrer comment l'analyse statique aide au développement de logiciels. De toute évidence, les contrôles ponctuels ne suffisent pas; vous devez utiliser régulièrement une analyse statique. Ce sujet est traité plus en détail dans l'article " Godot: sur l'utilisation régulière des analyseurs statiques ".

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

More articles:


All Articles