Geekly articles weekly

Incruste el análisis del código fuente con el analizador PVS-Studio

Cuadro 3

Emby es un servidor de medios bastante conocido, junto con Plex y Kodi. En este artículo, intentaremos verificar su código fuente utilizando el analizador estático PVS-Studio. En el sitio web oficial de Emby hay una pequeña marca "Construido con ReSharper". Bueno, es aún más interesante.

Analizador de código PVS-Studio


PVS-Studio se ejecuta en sistemas de 64 bits en Windows, Linux y macOS. Él sabe cómo buscar errores en el código fuente de los programas escritos en C, C ++, C # y Java.

Proyecto auditado


Emby es un servidor de medios cuyo código fuente se puede encontrar en GitHub . Le permite organizar transmisiones y le permite acceder a contenido multimedia (video, audio, fotos) desde cualquier dispositivo. Squeeze del sitio web oficial de Emby :

  • Emby convierte automáticamente sus archivos multimedia sobre la marcha para reproducirlos en cualquier dispositivo;
  • Los amplios controles parentales facilitan el control del acceso al contenido, que será relevante para las familias con niños pequeños;
  • Emby organiza sus medios en presentaciones simples y elegantes. Nunca se verán iguales;
  • Transmisión de datos con sincronización en la nube;
  • Comparta fácilmente sus archivos multimedia con amigos y familiares;
  • Y mucho mas.

Piezas de código que llamaron la atención al examinar un informe del analizador


Advertencia 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 detectó una subexpresión duplicada c! = '<' . Tal vez un error se deslizó en este código, y algo más debería haber estado en lugar de '<' . O, lo más probable, la segunda subexpresión es superflua y solo necesita eliminarla.

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

El analizador nuevamente encontró un error tipográfico asociado con subexpresiones repetidas. Mirando hacia el futuro, diré que en este proyecto hay demasiados errores similares cometidos por descuido. No reprocho a los creadores de Emby en absoluto, a veces estamos dispersos de esta manera ( ejemplos , ejemplos , ejemplos ), y para estar a salvo de tales errores, solo hay un análisis estático.

Advertencia 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 esto es muy similar al problema que apareció debido a copiar y pegar, porque los cuerpos de bloque if y else son completamente idénticos. ¿Por qué necesita verificar los tamaños de la matriz typesToCount si, independientemente del número de elementos escritos en él, se ejecutará el mismo código? Es conocido solo por los desarrolladores.

Advertencia 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<....>(....); .... }

De nuevo, un error tipográfico, que implicaba la asignación de una variable a sí mismo. Vale la pena volver a comprobar este código.

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

Falta de atención y errores tipográficos, de nuevo ... A la variable Capítulos se le asigna un valor dos veces. Está claro que este código adicional no amenaza nada malo, pero no obstante. No tiene sentido detenerse en este ejemplo, sigamos adelante.

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

Los cuerpos de las funciones de lectura y escritura son absolutamente idénticos entre sí, como lo demuestra el texto de advertencia del analizador. El método EnterWriteLock es necesario para ingresar el bloqueo en modo de escritura. Para el modo de lectura, vale la pena usar el método EnterReadLock , que permite la capacidad de leer un recurso simultáneamente en varios hilos.

El desarrollador debería echar un vistazo más de cerca a esta sección del código, quizás haya un error. Además, se encontró la siguiente clase en el código, que no se utiliza:

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

Advertencia 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"); } .... }

Aquí, aparentemente, el desarrollador copió las primeras cuatro líneas, pero olvidó cambiar la variable que se está probando de inputPath a outputPath . Hay varias líneas en el código que usan outputPath sin verificar null , lo que puede provocar una excepción.

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

En mi cuenta, no hay tantos, en comparación con el resto del equipo de PVS-Studio, verificados por el analizador de proyectos, y tal vez esto esté de alguna manera relacionado con el hecho de que primero veo que se emiten hasta 13 advertencias de analizadores en 13 líneas de código (un poco más que cada segunda línea). Por lo tanto, creo que este código merece ser incluido en el artículo. A continuación, escribiré lo que el analizador jura específicamente.

  1. Primero, se considera la expresión frame.IsCompressed && _compression == CompressionMethod.None y, si la expresión es verdadera, se ejecutará el método processUnsupportedFrame , que en cualquier caso devuelve falso (de ahí la primera advertencia del analizador). Si era falso, entonces pasa al siguiente punto.
  2. A continuación, se verifica el valor de frame.IsFragmented . No hay problemas, así que sigamos adelante.
  3. Aquí se verifica el valor de frame.IsData . Si es verdadero, el método processDataFrame devolverá verdadero de todos modos. De ahí la segunda advertencia del analizador.
  4. Luego verificamos frame.IsPing . Verdadero: processPingFrame devolverá verdadero . Esta es la tercera advertencia del analizador.
  5. Nos fijamos en el marco . IsPong . Todo es similar al anterior.
  6. Y el último: frame.IsClose . processCloseFrame y processUnsupportedFrame en cualquier caso devuelve falso .

Imagen 1

Espero no haberte aburrido. Será más sencillo aún más.

Advertencia 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 usa un campo con un nombre similar al usado en la clase externa HdHomerunUdpStream . Esto no es un error, pero es una buena razón para verificar si todo está bien con este código. Su problema es que existe la posibilidad de confundir estas dos variables, y el código no funcionará según lo previsto por el desarrollador, aunque no causará quejas del compilador.

Advertencias 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 que se utiliza un bloqueo inseguro en este fragmento de código. El uso del bloqueo no es deseable porque el objeto utilizado para el bloqueo se comparte y puede utilizarse para bloquear en otro lugar sin el conocimiento del desarrollador que utilizó este objeto por primera vez. El resultado será un punto muerto en el mismo objeto.

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

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

Advertencia 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 indica que la última línea en este fragmento de código nunca se ejecutará. ¿Y por qué el desarrollador declara y establece la variable enableHttpStream en true y luego verifica inmediatamente esta variable en la instrucción if?

Quizás este código sea simplemente redundante. En cualquier caso, vale la pena verificarlo dos veces.

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

Este método utiliza una llamada potencialmente insegura al controlador de eventos RefreshStarted , como indica el analizador.

Ahora explicaré por qué el analizador consideró esta llamada RefreshStarted in code inseguro. Supongamos que, en el momento entre verificar nulo y llamar directamente al controlador de eventos en el cuerpo if , se realiza una cancelación de suscripción del evento en otro hilo. Y, si no hay más suscriptores, RefreshStarted será nulo . Pero en otro hilo, donde ya se ha producido una comprobación nula , se ejecutará la siguiente línea:
 RefreshStarted(this, new GenericEventArgs<BaseItem>(item));

Esto arrojará una NullReferenceException .

Advertencia 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(....); } .... }

En este fragmento de código, uno al lado del otro, hay dos declaraciones if con las mismas condiciones. Los cuerpos de las declaraciones if son diferentes. Es difícil decir si esto es un error o un código redundante. Quizás todo esté bien y el autor solo quería separar lógicamente dos acciones diferentes, una de las cuales está asociada con un cierto "Logotipo", y la segunda con un cierto "Arte".

Advertencia 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 la operación de dividir tipos de datos enteros. El valor resultante se convierte implícitamente en un tipo de punto flotante, lo que es extremadamente sospechoso.

En realidad, la variable progressPerService será igual a 1.0 solo si _services.Length = 1 . Para cualquier otro _services.Length valores, el resultado será 0.0 .

Aparentemente, debería escribirse aquí:

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


Advertencia 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" }, .... } }; }

Presta atención a la línea.

 <u>underline</u>

Ya tiene una etiqueta de cierre </u> . A continuación vemos este texto:
 </s></u></i></b> HTML tags"

Aquí la etiqueta de cierre </u> es superflua, como lo indica el analizador.
Advertencia 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; }

Y aquí, francamente, no está del todo claro lo que el desarrollador generalmente quería lograr. El analizador advierte que la segunda instrucción if verifica la compatibilidad del objeto raíz con su propio tipo. Lo más probable es que esto sea solo un código redundante, pero parece sospechoso, el desarrollador debería echarle un vistazo más de cerca.

Conclusión


En todos los sentidos, los desarrolladores hicieron un gran trabajo creando Emby (el proyecto tiene 215 539 líneas de código, 4.6% - comentarios). Son geniales, de verdad. Sin embargo, el analizador también lo intentó, emitiendo advertencias de nivel Alto - 113, Medio - 213 y Bajo - 112. Algunos de ellos eran falsos positivos, pero la mayoría de los errores no se incluyeron en el artículo debido a que eran similares entre sí.

Por ejemplo, las advertencias V3022 solo (la expresión siempre es falsa / verdadera) obtuvieron 106 piezas. Puede, por supuesto, verificar qué parte de ellos eran falsos positivos y agregar el resto al artículo, pero esto dará como resultado un texto muy aburrido que nadie leerá.

Espero haber logrado demostrar la utilidad del analizador estático en el desarrollo. Por supuesto, las verificaciones únicas no son suficientes, y un analizador estático debe usarse de manera continua. Puede leer más sobre esto en el artículo Godot: Sobre el uso regular de analizadores de código estático .



Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Ekaterina Nikiforova. Comprobación de Emby con PVS-Studio .

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

More articles:


All Articles