Geekly articles weekly

Emby mit PVS-Studio überprüfen

Bild 3

Emby ist neben Plex und Kodi ein sehr beliebter Medienserver. In diesem Artikel diskutieren wir die Fehler, die in seinem Quellcode mit dem statischen Analysator PVS-Studio gefunden wurden. Die Bemerkung "Built with ReSharper" auf der offiziellen Website des Projekts macht die Analyse noch interessanter.

PVS-Studio


PVS-Studio kann auf 64-Bit-Windows-, Linux- und MacOS-Systemen ausgeführt werden. Es kann Fehler im Quellcode von Software erkennen, die in C, C ++, C # und Java geschrieben wurde.

Das zu analysierende Projekt


Emby ist ein Medienserver; Der Quellcode ist auf GitHub verfügbar. Der Benutzer kann auf jedem Gerät Medieninhalte (Video, Audio, Fotos) streamen und darauf zugreifen. Hier ist eine kurze Zusammenfassung der Funktionen von Emby gemäß der offiziellen Website des Projekts:

  • Emby konvertiert und streamt Ihre Medien automatisch, um sie auf jedem Gerät wiederzugeben.
  • Umfangreiche Optionen für die Kindersicherung für eine einfache Inhaltszugriffskontrolle, die für Familien mit kleinen Kindern wichtig ist.
  • Emby organisiert Ihre Inhalte in einfachen und eleganten Präsentationen. Ihre persönlichen Medien werden niemals gleich aussehen.
  • Streaming mit Cloud-Sync-Unterstützung;
  • Einfaches Teilen von Inhalten mit Freunden und Familie;
  • Und vieles mehr.

Die interessantesten Codefragmente, die vom Analysator gemeldet wurden


PVS-Studio-Diagnosemeldung: V3001 Es gibt identische Unterausdrücke 'c! =' <'' Links und rechts vom Operator '&&'. 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; .... }

Der Analysator hat einen doppelten Unterausdruck c! = '<' Festgestellt. Eine Erklärung ist, dass dies ein Programmierfehler ist und der Entwickler etwas anderes anstelle von '<' schreiben wollte. Eine andere, wahrscheinlichere Erklärung ist, dass der zweite Unterausdruck überhaupt nicht vorhanden war und entfernt werden sollte.

PVS-Studio Diagnosemeldung: V3001 Es gibt identische Unterausdrücke 'SmbConstants.AttrHidden' links und rechts vom '|' Betreiber. SmbComDelete.cs 29

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

Ein weiterer Tippfehler, der mit doppelten Unterausdrücken zu tun hat. Nebenbei bemerkt, es gibt zu viele Probleme wie diese in Embys Quellcode - Fehler, die durch Unachtsamkeit verursacht werden. Ich beschuldige die Entwickler jedoch nicht. Manchmal können wir alle geistesabwesend sein ( Beispiele , Beispiele , Beispiele ), und genau deshalb gibt es statische Analysen - um uns vor unseren eigenen Fehlern zu schützen.

PVS-Studio Diagnosemeldung: V3004 Die Anweisung 'then' entspricht der Anweisung '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 + "))"; } .... }

Und dieser sieht einem Copy-Paste-Bug sehr ähnlich, da die if- und else- Blöcke dieselben Körper haben. Was nützt es, die Größe des typesToCount- Arrays zu überprüfen, wenn dies die nachfolgende Ausführungslogik nicht beeinflusst? Das wissen nur die Autoren.

PVS-Studio Diagnosemeldung: V3005 Die Variable '_validProviderIds' ist sich selbst zugeordnet. BaseNfoParser.cs 77

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

Ein weiterer Tippfehler, der dazu führt, dass eine Variable einen eigenen Wert erhält. Dieser Code muss überarbeitet werden.

PVS-Studio-Diagnosemeldung: V3008 Der Variablen 'Chapters' werden zweimal nacheinander Werte zugewiesen. Vielleicht ist das ein Fehler. Check lines: 29, 28. Title.cs 29

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

Es geht wieder um Unaufmerksamkeit und Tippfehler ... Den Chapters- Variablen wird zweimal ein Wert zugewiesen. Sicher, diese doppelte Zuweisung wird keinen Schaden anrichten, aber Sie möchten immer noch nicht, dass in Ihrem Code solche Dinge vorkommen. Es hat keinen Sinn, sich mit dieser Sache zu beschäftigen, also lasst uns weitermachen.

PVS-Studio-Diagnosemeldung: V3013 Es ist merkwürdig, dass der Hauptteil der Funktion 'Lesen' dem Hauptteil der Funktion 'Schreiben' (407, Zeile 415) vollständig entspricht. 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; } } }

Die Funktionen Lesen und Schreiben haben die gleichen Körper, und das sagt uns der Analysator. Die EnterWriteLock- Methode wird verwendet, um die Sperre im Schreibmodus einzugeben. Wenn Sie die Sperre im Lesemodus eingeben möchten, verwenden Sie die EnterReadLock- Methode, mit der eine Ressource von mehreren Threads gleichzeitig gelesen werden kann.

Die Entwickler sollten diesen Code überprüfen, da er sehr wahrscheinlich einen Fehler enthält - zumal im Code eine nicht verwendete Klasse enthalten ist:

 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-Diagnosemeldung: V3021 Es gibt zwei "if" -Anweisungen mit identischen Bedingungsausdrücken. Die erste 'if'-Anweisung enthält die Methode return. Dies bedeutet, dass die zweite "if" -Anweisung sinnlos ist. 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"); } .... }

Der Entwickler muss die ersten vier Zeilen geklont haben, aber vergessen haben, den Namen der geprüften Variablen von inputPath in outputPath zu ändern . Es gibt mehrere Zeilen weiter, in denen outputPath ohne vorherige Nullprüfung verwendet wird, was bedeutet, dass möglicherweise eine Ausnahme ausgelöst wird.

PVS-Studio Diagnosemeldungen:

  • V3022 Der Ausdruck 'processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null)' ist immer falsch. WebSocket.cs 462
  • V3022 Der Ausdruck 'processCloseFrame (frame)' ist immer falsch. WebSocket.cs 461
  • V3022 Ausdruck 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'ist immer false. WebSocket.cs 460
  • V3022 Der Ausdruck 'processPongFrame (frame)' ist immer wahr. WebSocket.cs 459
  • V3022 Der Ausdruck 'processPingFrame (frame)' ist immer wahr. WebSocket.cs 457
  • V3022 Der Ausdruck 'processDataFrame (frame)' ist immer wahr. WebSocket.cs 455
  • V3022 Ausdruck ist immer falsch. 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; }

Ich habe bisher weniger Projekte überprüft als meine PVS-Studio-Teamkollegen, und dies erklärt wahrscheinlich, warum ich noch nie einen Codeausschnitt von 13 Zeilen gesehen habe, der 7 Warnungen gleichzeitig auslösen würde (d. H. Etwas mehr als eine Warnung pro zwei Zeilen). Deshalb füge ich diesen Fall in den Artikel ein. Im Folgenden finden Sie eine schrittweise Analyse des Problemfragments.

  1. Der Ausdruck frame.IsCompressed && _compression == CompressionMethod.None wird zuerst ausgewertet. Wenn dies der Fall ist, wird die processUnsupportedFrame- Methode ausgeführt und in jedem Fall false zurückgegeben (dies ist die erste Warnung). Wenn die Prüfung falsch ist , fahren wir mit der nächsten fort.
  2. Der Wert frame.IsFragmented wird geprüft. Keine Probleme hier.
  3. Der Wert frame.IsData wird geprüft. Wenn dies der Fall ist, gibt die processDataFrame- Methode in jedem Fall true zurück. Dies ist die zweite Warnung.
  4. Der Wert frame.IsPing wird geprüft. Wenn dies der Fall ist, gibt die processPingFrame- Methode true zurück . Dies ist die dritte Warnung.
  5. Der Wert frame.IsPong wird geprüft. Gleich wie der vorherige.
  6. Der letzte: frame.IsClose . processCloseFrame und processUnsupportedFrame geben in jedem Fall false zurück.

Bild 1

Ich hoffe, es war nicht zu langweilig, um zu folgen. Die restlichen Beispiele sind nicht so kompliziert.

PVS-Studio-Diagnosemeldung: V3085 Der Name des Felds 'RtpHeaderBytes' in einem verschachtelten Typ ist nicht eindeutig. Der äußere Typ enthält ein statisches Feld mit identischem Namen. 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; .... } .... }

Die verschachtelte Klasse UdpClientStream verfügt über ein Feld, dessen Name mit dem eines Felds der einschließenden Klasse HdHomerunUdpStream identisch ist . Es ist kein Fehler, aber es ist ein guter Grund, diesen Code erneut zu überprüfen, um sicherzustellen, dass er korrekt ist. Wenn Sie Variablen mit identischen Namen haben, können Sie leicht versehentlich eine von ihnen anstelle der anderen verwenden, was zu einem unerwarteten Verhalten des Programms führt, während der Compiler kein Wort sagt.

PVS-Studio Diagnosemeldungen:

  • V3090 Unsichere Verriegelung eines Typs. Alle Instanzen eines Typs haben dasselbe 'Type'-Objekt. Lmhosts.cs 49
  • V3090 Unsichere Verriegelung eines Typs. Alle Instanzen eines Typs haben dasselbe 'Type'-Objekt. 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)) { .... } } }

Der Analysator warnt hier vor einer unsicheren Sperre. Die Verwendung der Sperre auf diese Weise wird nicht empfohlen, da das Sperrobjekt öffentlich zugänglich ist und an einer anderen Stelle gesperrt werden kann, und der Entwickler, der dieses Objekt zum ersten Mal verwendet hat, dies möglicherweise nie weiß. Dies kann zu einem Deadlock führen.

Idealerweise sollten Sie ein privates Feld zum Sperren verwenden, zum Beispiel:

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

PVS-Studio Diagnosemeldung: V3142 Unerreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. 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(....); }

Der Analysator sagt, dass die letzte Zeile in diesem Snippet niemals ausgeführt wird. Und was ist der Zweck, die Variable enableHttpStream zu deklarieren, ihr true zuzuweisen und sie anschließend zu überprüfen?

Vielleicht ist dieser Code einfach redundant, muss aber trotzdem überarbeitet werden.

PVS-Studio Diagnosemeldung: V3083 Unsicherer Aufruf des Ereignisses 'RefreshStarted', NullReferenceException ist möglich. Überlegen Sie, ob Sie einer lokalen Variablen ein Ereignis zuweisen möchten, bevor Sie sie aufrufen. ProviderManager.cs 943

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

Der Analyzer warnt uns vor einem möglicherweise unsicheren Aufruf des RefreshStarted- Ereignishandlers.

Lassen Sie uns herausfinden, warum dieser Aufruf unsicher ist. Angenommen, das Ereignis wird momentan in einem anderen Thread abbestellt, zwischen dem Überprüfen des Ereignisses auf Null und dem Aufrufen des Ereignishandlers im Hauptteil der if- Anweisung. Wenn keine Abonnenten mehr vorhanden sind, wird das RefreshStarted- Ereignis zu null , aber in dem Thread, in dem die Nullprüfung bereits bestanden wurde, wird der Aufruf trotzdem ausgeführt:

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

Dies führt dazu, dass eine NullReferenceException ausgelöst wird .

PVS-Studio Diagnosemeldung: V3029 Die bedingten Ausdrücke der nebeneinander stehenden ' if' -Anweisungen sind identisch. Überprüfen Sie die Zeilen: 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(....); } .... }

Die beiden if- Anweisungen haben identische Bedingungen, aber unterschiedliche Körper. Ich bin nicht sicher, ob dies ein Fehler oder nur redundanter Code ist. Vielleicht ist es in Ordnung und der Entwickler wollte einfach explizit zwischen zwei Aktionen unterscheiden, von denen eine mit "Logo" und die andere mit "Art" zu tun hat, was auch immer diese sind.

PVS-Studio-Diagnosemeldung: V3041 Der Ausdruck wurde implizit vom Typ 'int' in den Typ 'double' umgewandelt. Erwägen Sie die Verwendung einer expliziten Typumwandlung, um den Verlust eines Bruchteils zu vermeiden. Ein Beispiel: double A = (double) (X) / Y; LiveTvManager.cs 1085

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

Dieser Code enthält eine Ganzzahldivision, wobei der resultierende Wert in einen Gleitkommatyp umgewandelt wird, was anscheinend nicht richtig ist.

Tatsächlich hat die progressPerService- Variable nur dann den Wert 1.0, wenn _services.Length = 1 ist . Bei jedem anderen Wert von _services.Length ist das Ergebnis 0.0 .

Ich denke, stattdessen sollte folgendes geschrieben werden:

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

PVS-Studio Diagnosemeldung: V3050 Möglicherweise falsches HTML. Das </ u> schließende Tag wurde gefunden, während das </ i> -Tag erwartet wurde. 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" }, .... } }; }

Beachten Sie diese Zeile:

 <u>underline</u>

Es hat bereits ein schließendes Tag </ u> .
Dann sehen wir den folgenden Text:
 </s></u></i></b> HTML tags"

Es gibt hier ein zusätzliches schließendes Tag </ u> , worauf der Analysator hinweist.

PVS-Studio Diagnosemeldung: V3051 Eine übermäßige Typprüfung. Das Objekt ist bereits vom Typ 'Ausnahme'. 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; }

Ehrlich gesagt verstehe ich nicht ganz, was der Entwickler mit diesem Code gemeint hat. Der Analysator sagt, dass die Bedingung der zweiten if- Anweisung prüft, ob das Stammobjekt mit seinem eigenen Typ kompatibel ist. Dies ist wahrscheinlich nur redundanter Code, aber er sieht seltsam aus und ich empfehle, ihn zu überarbeiten.

Fazit


Die Entwickler von Emby haben in jeder Hinsicht großartige Arbeit geleistet (das Projekt hat eine Länge von 215.539 LOC, wovon 4,6% Kommentare sind). Sie haben es gut gemacht, ich meine es ernst. Aber PVS-Studio verdient auch Lob: Es wurden 113 High- Level- , 213 Medium-Level- und 112 Low-Level-Warnungen ausgegeben. Einige von ihnen waren falsch positiv, aber die meisten Fehler wurden hier nicht erwähnt, weil sie sehr ähnlich waren. Beispielsweise wurde allein die Diagnose V3022 (immer falsch / wahr) 106-mal ausgelöst. Natürlich hätte ich die Fehlalarme herausfiltern und den Rest in den Artikel aufnehmen können, aber es wäre zu langweilig geworden, um ihn zu lesen.

Ich hoffe, ich konnte zeigen, wie statische Analyse bei der Softwareentwicklung hilft. Einmalige Kontrollen reichen natürlich nicht aus. Sie sollten regelmäßig statische Analysen durchführen. Dieses Thema wird im Artikel " Godot: Zur regelmäßigen Verwendung von statischen Analysatoren " ausführlicher behandelt.

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

More articles:


All Articles