Geekly articles weekly

Comprobación de Emby con PVS-Studio

Cuadro 3

Emby es un servidor de medios bastante popular junto con Plex y Kodi. En este artículo, discutiremos los errores encontrados en su código fuente con el analizador estático PVS-Studio. El comentario "Construido con ReSharper" en el sitio web oficial del proyecto hace que el análisis sea aún más interesante.

PVS-Studio


PVS-Studio se ejecuta en sistemas Windows, Linux y macOS de 64 bits. Puede detectar errores en el código fuente del software escrito en C, C ++, C # y Java.

El proyecto bajo análisis.


Emby es un servidor de medios; su código fuente está disponible en GitHub . Permite al usuario transmitir y acceder a su contenido multimedia (video, audio, fotos) en cualquier dispositivo. Aquí hay un breve resumen de las características de Emby según el sitio web oficial del proyecto:

  • Emby convierte y transmite automáticamente sus medios sobre la marcha para reproducirlos en cualquier dispositivo;
  • Amplias opciones de control parental para facilitar el control de acceso al contenido, que es una característica importante para las familias con niños pequeños;
  • Emby organiza su contenido en presentaciones fáciles y elegantes. Sus medios personales nunca se verán iguales;
  • Streaming con soporte de sincronización en la nube;
  • Fácil intercambio de contenido con sus amigos y familiares;
  • Y mucho mas.

Los fragmentos de código más interesantes reportados por el analizador


Mensaje de diagnóstico de PVS-Studio: V3001 Hay subexpresiones idénticas 'c! =' <'' A la izquierda y a la derecha del operador '&&'. 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; .... }

El analizador ha detectado una subexpresión duplicada c! = '<' . Una explicación es que este es un error de programación y el desarrollador tenía la intención de escribir algo más en lugar de '<' . Otra explicación más probable es que la segunda subexpresión no estaba destinada a estar allí y debería eliminarse.

Mensaje de diagnóstico de PVS-Studio: V3001 Hay subexpresiones idénticas 'SmbConstants.AttrHidden' a la izquierda y a la derecha de '|' operador SmbComDelete.cs 29

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

Otro error tipográfico que tiene que ver con subexpresiones duplicadas. Como nota al margen, hay demasiados problemas como ese en el código fuente de Emby: errores causados ​​por la falta de atención. Sin embargo, no culpo a los desarrolladores; todos podemos estar distraídos a veces ( ejemplos , ejemplos , ejemplos ), y esto es exactamente por qué existe el análisis estático: para protegernos de nuestros propios errores.

Mensaje de diagnóstico de PVS-Studio: V3004 La declaración 'then' es equivalente a la declaración '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 + "))"; } .... }

Y este se parece mucho a un error de copiar y pegar porque los bloques if y else tienen los mismos cuerpos. ¿De qué sirve comprobar el tamaño de la matriz typesToCount si no afecta a la lógica de ejecución posterior? Esto es algo que solo los autores saben.

Mensaje de diagnóstico de PVS-Studio: V3005 La variable '_validProviderIds' se asigna a sí misma. BaseNfoParser.cs 77

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

Otro error tipográfico, lo que resulta en asignar a una variable su propio valor. Este código necesita revisión.

Mensaje de diagnóstico de PVS-Studio: V3008 A la variable 'Capítulos' se le asignan valores dos veces seguidas. Quizás esto sea un error. Líneas de verificación: 29, 28. Título.cs 29

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

Se trata de falta de atención y errores tipográficos de nuevo ... Las variables de los capítulos se les asigna un valor dos veces. Claro, esta asignación duplicada no hará ningún daño, pero aún así no quieres cosas como esas en tu código. No tiene sentido demorarse en este, así que sigamos adelante.

Mensaje de diagnóstico de PVS-Studio: V3013 Es extraño que el cuerpo de la función 'Leer' sea completamente equivalente al cuerpo de la función 'Escribir' (407, línea 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; } } }

Las funciones Leer y Escribir tienen los mismos cuerpos, y eso es lo que nos dice el analizador. El método EnterWriteLock se usa para ingresar el bloqueo en modo de escritura. Si desea ingresar el bloqueo en modo de lectura, use el método EnterReadLock , que permite que un recurso sea leído por varios hilos a la vez.

Los desarrolladores deben verificar este código porque es muy probable que contenga un error, sobre todo porque hay una clase no utilizada en el código:

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

Mensaje de diagnóstico de PVS-Studio: V3021 Hay dos declaraciones 'if' con expresiones condicionales idénticas. La primera instrucción 'if' contiene el método return. Esto significa que la segunda declaración 'if' no tiene sentido 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"); } .... }

El desarrollador debe haber clonado las primeras cuatro líneas, pero se olvidó de cambiar el nombre de la variable que se verifica de inputPath a outputPath . Hay varias líneas más allá donde outputPath se usa sin una verificación nula previa, lo que significa que se puede lanzar una excepción.

Mensajes de diagnóstico de PVS-Studio:

  • La expresión V3022 'processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null)' siempre es falsa. WebSocket.cs 462
  • V3022 La expresión 'processCloseFrame (frame)' siempre es falsa. WebSocket.cs 461
  • V3022 Expresión 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'siempre es falso. WebSocket.cs 460
  • V3022 La expresión 'processPongFrame (frame)' siempre es verdadera. WebSocket.cs 459
  • La expresión V3022 'processPingFrame (frame)' siempre es verdadera. WebSocket.cs 457
  • V3022 La expresión 'processDataFrame (frame)' siempre es verdadera. WebSocket.cs 455
  • V3022 La expresión siempre es falsa. 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; }

He verificado menos proyectos que mis compañeros de equipo de PVS-Studio hasta ahora, y esto probablemente explica por qué nunca antes había visto un fragmento de código de 13 líneas que activaría 7 advertencias a la vez (es decir, un poco más de una advertencia por dos líneas). Por eso incluyo este caso en el artículo. A continuación se muestra un análisis paso a paso del fragmento del problema.

  1. La expresión frame.IsCompressed && _compression == CompressionMethod.None se evalúa primero. Si es cierto, el método processUnsupportedFrame se ejecutará y devolverá falso en cualquier caso (esta es la primera advertencia). Si el cheque es falso , pasamos al siguiente.
  2. Se marca el valor frame.IsFragmented . No hay problemas aquí.
  3. El valor frame.IsData está marcado. Si es cierto, el método processDataFrame devolverá verdadero en cualquier caso. Esta es la segunda advertencia.
  4. El valor frame.IsPing está marcado. Si es cierto, el método processPingFrame devolverá verdadero . Esta es la tercera advertencia.
  5. El valor frame.IsPong está marcado. Igual que el anterior.
  6. El último: frame.IsClose . processCloseFrame y processUnsupportedFrame devuelven false en cualquier caso.

Imagen 1

Espero que no haya sido demasiado tedioso para seguir. Los ejemplos restantes no son tan complicados.

Mensaje de diagnóstico de PVS-Studio: V3085 El nombre del campo 'RtpHeaderBytes' en un tipo anidado es ambiguo. El tipo externo contiene un campo estático con un nombre idéntico. 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 clase anidada UdpClientStream tiene un campo cuyo nombre es idéntico al de un campo de la clase adjunta HdHomerunUdpStream . No es un error, pero es una buena razón para revisar este código nuevamente para asegurarse de que sea correcto. Tener variables con nombres idénticos facilita el uso accidental de una de ellas en lugar de la otra, lo que resulta en el comportamiento inesperado del programa, mientras que el compilador no dice una palabra.

Mensajes de diagnóstico de PVS-Studio:

  • V3090 Bloqueo inseguro en un tipo. Todas las instancias de un tipo tendrán el mismo objeto 'Tipo'. Lmhosts.cs 49
  • V3090 Bloqueo inseguro en un tipo. Todas las instancias de un tipo tendrán el mismo objeto 'Tipo'. 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)) { .... } } }

El analizador advierte sobre un bloqueo inseguro aquí. No se recomienda usar el bloqueo de esa manera porque el objeto de bloqueo es de acceso público y se puede bloquear en otro lugar, y el desarrollador que utilizó este objeto por primera vez puede no saberlo. Esto puede conducir a un punto muerto.

Idealmente, debe usar un campo privado para bloquear, por ejemplo:

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

Mensaje de diagnóstico de PVS-Studio: V3142 Código inalcanzable detectado. Es posible que haya un error presente. 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(....); }

El analizador dice que la última línea de este fragmento nunca se ejecutará. ¿Y cuál es el propósito de declarar la variable enableHttpStream, asignarle verdadero y verificarla inmediatamente después?

Quizás este código sea simplemente redundante, pero de todos modos necesita una revisión.

Mensaje de diagnóstico de PVS-Studio: V3083 Invocación insegura del evento 'RefreshStarted', NullReferenceException es posible. Considere asignar un evento a una variable local antes de invocarlo. ProviderManager.cs 943

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

El analizador nos advierte sobre una llamada potencialmente insegura del controlador de eventos RefreshStarted .

Veamos por qué esta llamada no es segura. Suponga que el evento se da de baja de otro subproceso en el momento entre verificar el evento como nulo y llamar al controlador de eventos en el cuerpo de la instrucción if . Si no quedan suscriptores, el evento RefreshStarted se volverá nulo , pero en el hilo donde la verificación nula ya pasó, la llamada se ejecutará de todos modos:

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

Esto resultará en arrojar una NullReferenceException .

Mensaje de diagnóstico de PVS-Studio: V3029 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Líneas de verificación: 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(....); } .... }

Las dos declaraciones if tienen condiciones idénticas, pero sus cuerpos son diferentes. No estoy seguro de si esto es un error o solo un código redundante. Quizás está bien y el desarrollador simplemente quería diferenciar explícitamente entre dos acciones, una de las cuales tiene que ver con "Logo" y la otra con "Art", cualesquiera que sean.

Mensaje de diagnóstico de PVS-Studio: V3041 La expresión se convirtió implícitamente de tipo 'int' a tipo 'doble'. Considere utilizar un molde de tipo explícito para evitar la pérdida de una parte fraccional. Un ejemplo: doble A = (doble) (X) / Y;. LiveTvManager.cs 1085

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

Este código contiene una división entera, con el valor resultante convertido a un tipo de punto flotante, que no parece ser lo correcto.

En realidad, la variable progressPerService tendrá el valor 1.0 solo si _services.Length = 1 . Con cualquier otro valor de _services.Length , el resultado será 0.0 .

Creo que lo que debería escribirse es lo siguiente:

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

Mensaje de diagnóstico de PVS-Studio: V3050 Posiblemente un HTML incorrecto. Se encontró la etiqueta de cierre </u>, mientras que se esperaba la etiqueta </i>. 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" }, .... } }; }

Tenga en cuenta esta línea:

 <u>underline</u>

Ya tiene una etiqueta de cierre </u> .
Luego vemos el siguiente texto:
 </s></u></i></b> HTML tags"

Aquí hay una etiqueta de cierre adicional </u> , que es lo que señala el analizador.

Mensaje de diagnóstico de PVS-Studio: V3051 Una verificación de tipo excesiva. El objeto ya es del tipo 'Excepción'. 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; }

Francamente, no entiendo muy bien qué quería decir el desarrollador con este código. El analizador dice que la segunda condición de la instrucción if verifica si el objeto raíz es compatible con su propio tipo. Probablemente sea un código redundante, pero parece extraño y recomiendo revisarlo.

Conclusión


Los desarrolladores de Emby han hecho un gran trabajo en todos los sentidos (el proyecto tiene 215,539 LOC de largo, 4.6% de los cuales son comentarios). Lo hicieron bien, lo digo en serio. Pero PVS-Studio también merece elogios: produjo 113 advertencias de alto nivel , 213 de nivel medio y 112 de bajo nivel. Algunos de ellos eran falsos positivos, pero la mayoría de los errores no se mencionaron aquí porque eran muy parecidos. Por ejemplo, el diagnóstico V3022 (siempre condición falsa / verdadera) solo se activó 106 veces. Por supuesto, podría haber filtrado los falsos positivos e incluir el resto en el artículo, pero habría sido demasiado aburrido para leer.

Espero haber logrado mostrar cómo el análisis estático ayuda en el desarrollo de software. Obviamente, los cheques únicos no son suficientes; Debe utilizar el análisis estático de forma regular. Este tema se trata con más detalle en el artículo " Godot: sobre el uso regular de analizadores estáticos ".

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

More articles:


All Articles