Geekly articles weekly

Verificando o Emby com o PVS-Studio

Quadro 3

Emby é um servidor de mídia bastante popular, juntamente com Plex e Kodi. Neste artigo, discutiremos os erros encontrados em seu código-fonte com o analisador estático PVS-Studio. A observação "Built with ReSharper" no site oficial do projeto torna a análise ainda mais interessante.

PVS-Studio


O PVS-Studio é executado em sistemas Windows, Linux e macOS de 64 bits. Ele pode detectar erros no código fonte do software escrito em C, C ++, C # e Java.

O projeto em análise


Emby é um servidor de mídia; seu código fonte está disponível no GitHub . Ele permite ao usuário transmitir e acessar seu conteúdo de mídia (vídeo, áudio, fotos) em qualquer dispositivo. Aqui está um breve resumo dos recursos de Emby , de acordo com o site oficial do projeto:

  • O Emby converte e transmite automaticamente sua mídia on-the-fly para reproduzir em qualquer dispositivo;
  • Extensas opções de controle parental para facilitar o controle de acesso ao conteúdo, que é um recurso importante para famílias com crianças pequenas;
  • Emby organiza seu conteúdo em apresentações fáceis e elegantes. Sua mídia pessoal nunca será a mesma;
  • Streaming com suporte à sincronização na nuvem;
  • Fácil compartilhamento de conteúdo com seus amigos e familiares;
  • E muito mais

Os trechos de código mais interessantes relatados pelo analisador


Mensagem de diagnóstico do PVS-Studio: V3001 Existem subexpressões idênticas 'c! =' <'' À esquerda e à direita do 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; .... }

O analisador detectou uma subexpressão duplicada c! = '<' . Uma explicação é que este é um erro de programação e o desenvolvedor pretendeu escrever outra coisa no lugar de '<' . Outra explicação mais provável é que a segunda subexpressão não deveria estar lá e deveria ser removida.

Mensagem de diagnóstico do PVS-Studio: V3001 Existem subexpressões idênticas 'SmbConstants.AttrHidden' à esquerda e à direita da '|' operador. SmbComDelete.cs 29

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

Outro erro de digitação relacionado a subexpressões duplicadas. Como uma observação lateral, há muitos problemas como esse no código fonte de Emby - erros causados ​​por desatenção. Não estou culpando os desenvolvedores; todos nós podemos nos distrair às vezes ( exemplos , exemplos , exemplos ), e é exatamente por isso que a análise estática existe - para nos proteger de nossos próprios erros.

Mensagem de diagnóstico do PVS-Studio: V3004 A instrução 'then' é equivalente à instrução '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 + "))"; } .... }

E este parece muito com um bug de copiar e colar, porque os blocos if e else têm os mesmos corpos. Qual é a vantagem de verificar o tamanho da matriz typesToCount se ela não afeta a lógica de execução subsequente? Isso é algo que apenas os autores sabem.

Mensagem de diagnóstico do PVS-Studio: V3005 A variável '_validProviderIds' é atribuída a si mesma. BaseNfoParser.cs 77

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

Outro erro de digitação, que resulta em atribuir a uma variável seu próprio valor. Esse código precisa ser revisado.

Mensagem de diagnóstico do PVS-Studio: V3008 A variável 'Capítulos' recebe valores duas vezes sucessivamente. Talvez isso seja um erro. Verifique as linhas: 29, 28. Title.cs 29

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

Trata-se de desatenção e erros de digitação novamente ... As variáveis Capítulos recebem um valor duas vezes. Claro, essa atribuição duplicada não fará nenhum mal, mas você ainda não deseja coisas assim no seu código. Não adianta ficar com este, então vamos seguir em frente.

Mensagem de diagnóstico do PVS-Studio: V3013 É estranho que o corpo da função 'Read' seja totalmente equivalente ao corpo da função 'Write' (407, linha 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; } } }

As funções Ler e Escrever têm os mesmos corpos, e é isso que o analisador está nos dizendo. O método EnterWriteLock é usado para inserir o bloqueio no modo de gravação. Se você deseja inserir o bloqueio no modo de leitura, use o método EnterReadLock , que permite que um recurso seja lido por vários threads por vez.

Os desenvolvedores devem verificar esse código porque é muito provável que contenha um erro - ainda mais porque há uma classe não utilizada encontrada no 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; } } }

Mensagem de diagnóstico do PVS-Studio: V3021 Existem duas instruções 'if' com expressões condicionais idênticas. A primeira instrução 'if' contém retorno de método. Isso significa que a segunda declaração 'if' não faz 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"); } .... }

O desenvolvedor deve ter clonado as quatro primeiras linhas, mas esqueceu de alterar o nome da variável que está sendo verificada de inputPath para outputPath . Existem várias linhas mais adiante em que outputPath é usado sem uma verificação nula anterior, o que significa que uma exceção pode ser lançada.

Mensagens de diagnóstico do PVS-Studio:

  • A expressão V3022 'processUnsupportedFrame (quadro, CloseStatusCode.PolicyViolation, null)' sempre é falsa. WebSocket.cs 462
  • A expressão V3022 'processCloseFrame (frame)' é sempre falsa. WebSocket.cs 461
  • V3022 Expression 'frame.IsClose? processCloseFrame (frame): processUnsupportedFrame (frame, CloseStatusCode.PolicyViolation, null) 'é sempre falso. WebSocket.cs 460
  • A expressão V3022 'processPongFrame (quadro)' é sempre verdadeira. WebSocket.cs 459
  • A expressão V3022 'processPingFrame (frame)' é sempre verdadeira. WebSocket.cs 457
  • A expressão V3022 'processDataFrame (frame)' é sempre verdadeira. WebSocket.cs 455
  • A expressão V3022 é sempre 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; }

Verifiquei menos projetos do que meus colegas do PVS-Studio até agora, e isso provavelmente explica por que nunca vi um trecho de código de 13 linhas que acionaria 7 avisos de uma só vez (ou seja, um pouco mais de um aviso por duas linhas). É por isso que estou incluindo este caso no artigo. Abaixo está uma análise passo a passo do fragmento do problema.

  1. A expressão frame.IsCompressed && _compression == CompressionMethod.None é avaliada primeiro. Se for verdade, o método processUnsupportedFrame será executado e retornará false em qualquer caso (este é o primeiro aviso). Se o cheque for falso , passamos para o próximo.
  2. O valor frame.IsFragmented está marcado. Não há problemas aqui.
  3. O valor frame.IsData está marcado. Se for verdade, o método processDataFrame retornará true em qualquer caso. Este é o segundo aviso.
  4. O valor frame.IsPing está marcado. Se for verdade, o método processPingFrame retornará true . Este é o terceiro aviso.
  5. O valor frame.IsPong está marcado. Mesmo que o anterior.
  6. O último: frame.IsClose . processCloseFrame e processUnsupportedFrame retornam false em qualquer caso.

Quadro 1

Espero que não tenha sido muito entediante de seguir. Os exemplos restantes não são tão complicados.

Mensagem de diagnóstico do PVS-Studio: V3085 O nome do campo 'RtpHeaderBytes' em um tipo aninhado é ambíguo. O tipo externo contém campo estático com nome 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; .... } .... }

A classe aninhada UdpClientStream possui um campo cujo nome é idêntico ao de um campo da classe anexa HdHomerunUdpStream . Não é um bug, mas é um bom motivo para verificar esse código novamente para garantir que esteja correto. Ter variáveis ​​com nomes idênticos facilita o uso acidental de uma delas em vez da outra, resultando no comportamento inesperado do programa, enquanto o compilador não diz uma palavra.

Mensagens de diagnóstico do PVS-Studio:

  • V3090 Bloqueio inseguro em um tipo. Todas as instâncias de um tipo terão o mesmo objeto 'Tipo'. Lmhosts.cs 49
  • V3090 Bloqueio inseguro em um tipo. Todas as instâncias de um tipo terão o mesmo 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)) { .... } } }

O analisador está avisando sobre um bloqueio não seguro aqui. Usar o bloqueio de uma maneira assim não é recomendado, porque o objeto de bloqueio é acessível ao público e pode ser bloqueado em outro local, e o desenvolvedor que usou esse objeto pela primeira vez pode nunca saber disso. Isso pode levar a um impasse.

Idealmente, você deve usar um campo privado para bloquear, por exemplo:

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

Mensagem de diagnóstico do PVS-Studio: V3142 Código inacessível detectado. É possível que haja um erro. 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(....); }

O analisador diz que a última linha deste trecho nunca será executada. E qual é o objetivo de declarar a variável enableHttpStream, atribuindo true a ela e verificando-a logo depois?

Talvez esse código seja simplesmente redundante, mas precise ser revisto de qualquer maneira.

Mensagem de diagnóstico do PVS-Studio: V3083 Chamada não segura do evento 'RefreshStarted', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. ProviderManager.cs 943

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

O analisador nos avisa sobre uma chamada potencialmente insegura do manipulador de eventos RefreshStarted .

Vamos descobrir por que essa ligação não é segura. Suponha que o evento seja cancelado a inscrição em outro encadeamento no momento entre verificar o evento como nulo e chamar o manipulador de eventos no corpo da instrução if . Se não houver mais assinantes, o evento RefreshStarted se tornará nulo , mas no encadeamento em que a verificação nula já passou, a chamada será executada de qualquer maneira:

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

Isso resultará no lançamento de uma NullReferenceException .

Mensagem de diagnóstico do PVS-Studio: V3029 As expressões condicionais das instruções 'if' localizadas uma ao lado da outra são idênticas. Verifique as linhas: 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(....); } .... }

As duas declarações if têm condições idênticas, mas seus corpos são diferentes. Não tenho certeza se isso é um bug ou apenas um código redundante. Talvez esteja tudo bem e o desenvolvedor simplesmente queira diferenciar explicitamente entre duas ações, uma das quais tem a ver com "Logo" e a outra com "Art", quaisquer que sejam.

Mensagem de diagnóstico do PVS-Studio: V3041 A expressão foi implicitamente convertida do tipo 'int' para o tipo 'double'. Considere utilizar uma conversão de tipo explícita para evitar a perda de uma parte fracionária. Um exemplo: double A = (double) (X) / Y; LiveTvManager.cs 1085

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

Esse código contém uma divisão inteira, com o valor resultante sendo convertido para um tipo de ponto flutuante, o que não parece o correto.

Na verdade, a variável progressPerService terá o valor 1.0 somente se _services.Length = 1 . Com qualquer outro valor de _services.Length , o resultado será 0,0 .

Eu acho que o que deveria ser escrito é o seguinte:

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

Mensagem de diagnóstico do PVS-Studio: V3050 Possivelmente um HTML incorreto. A tag de fechamento </u> foi encontrada, enquanto a tag </i> era esperada. 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" }, .... } }; }

Observe esta linha:

 <u>underline</u>

Ele já tem uma tag de fechamento </u> .
Então vemos o seguinte texto:
 </s></u></i></b> HTML tags"

Há uma tag de fechamento extra </u> aqui, que é o que o analisador aponta.

Mensagem de diagnóstico do PVS-Studio: V3051 Uma verificação de tipo excessiva. O objeto já é do tipo 'Exceção'. 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, não entendo bem o que o desenvolvedor quis dizer com esse código. O analisador diz que a segunda condição da instrução if verifica se o objeto raiz é compatível com seu próprio tipo. Provavelmente é apenas um código redundante, mas parece estranho e eu recomendo revisá-lo.

Conclusão


Os desenvolvedores do Emby fizeram um ótimo trabalho em todos os aspectos (o projeto tem 215.539 LOC de comprimento, 4,6% dos quais são comentários). Eles fizeram bem, quero dizer. Mas o PVS-Studio também merece elogios: produziu 113 avisos de alto nível , 213 médio e 112 de baixo nível. Alguns deles eram falsos positivos, mas a maioria dos bugs não foram mencionados aqui porque eram praticamente os mesmos. Por exemplo, o diagnóstico V3022 (sempre condição falsa / verdadeira) sozinho foi acionado 106 vezes. É claro que eu poderia filtrar os falsos positivos e incluir o restante no artigo, mas seria muito chato de ler.

Espero ter conseguido mostrar como a análise estática ajuda no desenvolvimento de software. Obviamente, as verificações únicas não são suficientes; você deve usar a análise estática regularmente. Este tópico é discutido em mais detalhes no artigo " Godot: Sobre o uso regular de analisadores estáticos ".

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

More articles:


All Articles