Verificador RunUO do analisador PVS-Studio

Imagem 1

Este artigo é dedicado à verificação do projeto RunUO usando o analisador estático PVS-Studio. RunUO é um emulador de software de servidor para Ultima Online, um jogo que conquistou os corações de muitos fãs de MMORPG.

Introdução


RunUO é um emulador de software para servidor para o MMORPG Ultima Online. O objetivo deste projeto é criar software estável que possa competir com os servidores oficiais da EA Games. O RunUO foi criado em 2002, mas não perde relevância e é usado ativamente até hoje.

O objetivo da verificação do projeto é popularizar o tópico da análise estática. Verificamos vários projetos - jogos ( exemplo ), bibliotecas ( exemplo ), mensageiros ( exemplo ), navegadores ( exemplo ) e muito mais ( exemplo , exemplo , exemplo)) para atrair a atenção de um público diversificado. Com esses artigos, estamos tentando chamar a atenção para a importância do uso da análise estática no desenvolvimento. A análise estática torna o código mais confiável e seguro. Além disso, com seu uso regular, você pode encontrar e eliminar erros nos estágios iniciais. Isso economiza tempo e esforço aos desenvolvedores, porque ninguém deseja gastar 50 horas pesquisando o erro que o analisador pode encontrar.

Também ajudamos a comunidade de código aberto. Através de artigos com erros encontrados, contribuímos para o desenvolvimento de código aberto. No entanto, nos artigos não analisamos todos os avisos. Alguns avisos pareciam muito chatos para entrarmos no artigo, outros eram falsos positivos etc. Portanto, estamos prontos para fornecer uma licença gratuita para projetos de código aberto. Além disso, o que nos pareceu entediante para o artigo pode parecer bastante interessante para os desenvolvedores do projeto que está sendo testado, porque os desenvolvedores do projeto ainda estão mais conscientes dos problemas mais críticos.

Pedaços de código que chamaram a atenção ao examinar o relatório do analisador


PVS-Studio Warning: V3010 É necessário que o valor de retorno da função 'Intern' seja utilizado. BasePaintedMask.cs 49

public static string Intern( string str )
{
  if ( str == null )
    return null;
  else if ( str.Length == 0 )
    return String.Empty;

  return String.Intern( str );
}

public BasePaintedMask( string staffer, int itemid )
                            : base( itemid + Utility.Random( 2 ) )
{
  m_Staffer = staffer;

  Utility.Intern( m_Staffer );
}

O valor de retorno do método Intern () não é levado em consideração em nenhum lugar, conforme indicado pelo analisador. Talvez seja um bug ou código redundante.

Aviso do PVS-Studio: V3017 Um padrão foi detectado: (o item é BasePotion) || ((o item é BasePotion) && ...). A expressão é excessiva ou contém um erro lógico. Cleanup.cs 137

public static bool IsBuggable( Item item )
{
  if ( item is Fists )
    return false;

  if ( item is ICommodity || item is Multis.BaseBoat
    || item is Fish || item is BigFish
    || item is BasePotion || item is Food || item is CookableFood
    || item is SpecialFishingNet || item is BaseMagicFish
    || item is Shoes || item is Sandals
    || item is Boots || item is ThighBoots
    || item is TreasureMap || item is MessageInABottle
    || item is BaseArmor || item is BaseWeapon
    || item is BaseClothing
    || ( item is BaseJewel && Core.AOS )
    || ( item is BasePotion && Core.ML )
  {
    ....
  }
}

Existem subexpressões que podem ser simplificadas. Vou escrevê-los para que fique mais claro:
if (item is BasePotion || ( item is BasePotion && Core.ML ))

Suponha, artigo é BasePotion = a verdade , então a condição é verdadeira, apesar da Core.ML . Se o item for BasePotion = false , em seguida, a condição é falsa, mais uma vez, independentemente do valor Core.ML . Geralmente, esse código é simplesmente redundante; no entanto, existem situações piores em que o programador cometeu um erro e escreveu a variável errada na segunda subexpressão.

Aviso do PVS-Studio: V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador está cercado pelas expressões opostas 'bPlayerOnly' e '! bPlayerOnly'. BaseCreature.cs 3005

public virtual double GetFightModeRanking( Mobile m,
                                           FightMode acqType,
                                           bool bPlayerOnly )
{
  if ( ( bPlayerOnly && m.Player ) ||  !bPlayerOnly )
  {
    ....
  }
  ....
}

Este código é redundante ou incorreto. O problema dele é que nos dois lados de '||' são diferentes no sentido de subexpressão. Se cortarmos assim:

if ( m.Player || !bPlayerOnly )

então nada vai mudar.

PVS-Studio Warning: V3001 Existem sub-expressões idênticas 'ação é SmallBrickHouseDeed' à esquerda e à direita da '||' operador. RealEstateBroker.cs 132

public int ComputePriceFor( HouseDeed deed )
{
  int price = 0;

  if ( deed is SmallBrickHouseDeed ||    // <=
       deed is StonePlasterHouseDeed ||
       deed is FieldStoneHouseDeed ||
       deed is SmallBrickHouseDeed ||    // <=
       deed is WoodHouseDeed ||
       deed is WoodPlasterHouseDeed ||
       deed is ThatchedRoofCottageDeed )
      ....
}

Acho que não vale a pena explicar nada aqui, o próximo código ou o código errado ou redundante.

PVS-Studio Warning: V3067 É possível que o bloco 'else' tenha sido esquecido ou comentado, alterando a lógica de operação do programa. BaseHouse.cs 1558

private void SetLockdown( Item i, bool locked, bool checkContains )
{
  if ( m_LockDowns == null )
    return;

  #region Mondain's Legacy
  if ( i is BaseAddonContainer )
    i.Movable = false;
  else
  #endregion

  i.Movable = !locked;
  i.IsLockedDown = locked;

  ....
}

Um aviso raro. O analisador considerou suspeito a formatação do código após a diretiva #endregion. Se você não lê o código, parece uma linha

i.Movable = !locked;

será executado em qualquer caso, independentemente da variável i . Talvez chaves aqui tenham sido esquecidas ... Em geral, os desenvolvedores devem verificar esse código.

PVS-Studio Warning: V3043 A lógica operacional do código não corresponde à sua formatação. A instrução é recuada à direita, mas é sempre executada. É possível que estejam faltando colchetes. Terremoto.cs 57

public override void OnCast()
{
  if ( Core.AOS )
  {
    damage = m.Hits / 2;

    if ( !m.Player )
      damage = Math.Max( Math.Min( damage, 100 ), 15 );
      damage += Utility.RandomMinMax( 0, 15 );            // <=

  }
  else
  {
    ....
  }
}

Aparelhos cacheados podem estar ausentes neste código. Esta conclusão pode ser feita devido à formatação estranha do código no corpo de if (! M.Player) .

PVS-Studio Warning: V3083 Chamada não segura do evento 'ServerStarted', NullReferenceException é possível. Considere atribuir um evento a uma variável local antes de invocá-lo. EventSink.cs 921

public static void InvokeServerStarted()
{
  if ( ServerStarted != null )
    ServerStarted();
}

Esse método usa uma chamada potencialmente insegura para o manipulador de eventos RefreshStarted , como indica o analisador.

Por que isso é perigoso? Imagine essa situação. O evento ServerStarted possui apenas um assinante. E, no momento entre a verificação de nulo e a chamada direta do manipulador de eventos ServerStarted () em outro encadeamento, foi cancelada a inscrição deste evento. Isso lançará uma NullReferenceException .

A maneira mais fácil de evitar essa situação é fornecer uma chamada de evento segura usando o operador '?.':

public static void InvokeServerStarted()
{
  ServerStarted?.Invoke();
}

Aviso do PVS-Studio: V3054 Bloqueio potencialmente perigoso verificado duas vezes. Use variáveis ​​voláteis ou primitivas de sincronização para evitar isso. Item.cs 1624
private Packet m_RemovePacket;
....
private object _rpl = new object();
public Packet RemovePacket
{
  get
  {
    if (m_RemovePacket == null)
    {
      lock (_rpl)
      {
        if (m_RemovePacket == null)
        {
          m_RemovePacket = new RemoveItem(this);
          m_RemovePacket.SetStatic();
        }
      }
    }

    return m_RemovePacket;
  }
}

O aviso do analisador está associado ao uso inseguro do padrão de bloqueio verificado duas vezes. Como você pode ver no código acima, um bloqueio de verificação dupla foi aplicado para implementar o padrão generativo - Loners. Quando tentamos obter uma instância da classe Packet acessando a propriedade RemovePacket , o getter verifica o campo m_RemovePacket para que a igualdade seja zero. Se a verificação for aprovada, entraremos no corpo da instrução lock , onde o campo m_RemovePacket é inicializado . Uma situação interessante surge no momento em que o encadeamento principal já inicializou a variável m_RemovePacket por meio do construtor, mas ainda não chamou o métodoSetStatic (). Teoricamente, outro encadeamento pode acessar a propriedade RemovePacket no momento muito inconveniente. A verificação de igualdade de zero em m_RemovePacket não falhará mais, e o encadeamento de chamada receberá um link para um objeto incompletamente pronto para uso. Para resolver esse problema, você pode criar uma variável intermediária da classe Packet no corpo da instrução lock , inicializá-la por meio de uma chamada de construtor e do método SetStatic () e atribuí-la à variável m_RemovePacket . Nesse caso, o corpo da instrução lock pode ser assim:

lock (_rpl)
{
  if (m_RemovePacket == null)
  {
    Packet instance = new RemoveItem(this);
    instance.SetStatic();
    m_RemovePacket = instance;
  }
}

O problema parece estar corrigido e o código funcionará conforme o esperado. Mas isso não é verdade.

Há mais um ponto: o analisador não sugere apenas o uso da palavra-chave volátil . Na versão do programa, o compilador pode executar a otimização e reordenar as linhas da chamada para o método SetStatic () e atribuir a variável de instância ao campo m_RemovePacket (do ponto de vista do compilador, a semântica do programa não será violada). E voltamos novamente ao mesmo ponto em que começamos - a possibilidade de obter a variável não inicializada m_RemovePacket. É impossível dizer exatamente quando essa reordenação pode ocorrer e se ocorrerá: pode ser afetada pela versão do CLR, pela arquitetura do processador usado etc. Ainda vale a pena proteger desse cenário, e uma das soluções (no entanto, não a mais produtiva) será usar a palavra-chave volátil . Uma variável que será declarada com o modificador volátil não estará sujeita a permutações durante a otimização pelo compilador. Um código finalmente corrigido pode ser assim:

private volatile Packet m_RemovePacket;
....
private object _rpl = new object();
public Packet RemovePacket
{
  get
  {
    if (m_RemovePacket == null)
    {
      lock (_rpl)
      {
        if (m_RemovePacket == null)
        {
          Packet instance = new RemoveItem(this);
          instance.SetStatic();
          m_RemovePacket = instance;
        }
      }
    }

    return m_RemovePacket;
  }
}

Às vezes, o uso de um campo volátil pode ser indesejável devido à sobrecarga de acessar esse campo. Não vamos nos aprofundar neste tópico em detalhes, simplesmente observando que no exemplo em consideração, um registro de campo atômico é necessário apenas uma vez (quando a propriedade é acessada pela primeira vez); no entanto, declarar o campo como volátil fará com que o compilador faça sua leitura e gravação atômica isso pode não ser o ideal em termos de desempenho.

Portanto, outra abordagem para corrigir esse aviso do analisador para evitar sobrecarga adicional ao declarar um campo volátil é usar o tipo Lazy <T> para fazer backup do campo m_RemovePacketem vez de verificar novamente o bloqueio. Nesse caso, o corpo do getter pode ser substituído pelo método inicializador, que será passado ao construtor da instância Lazy <T> :

private Lazy<Packet> m_RemovePacket = new Lazy<Packet>(() =>
  {
    Packet instance = new RemoveItem(this);
    instance.SetStatic();
    return instance;
  }, LazyThreadSafetyMode.ExecutionAndPublication);

....
public Packet RemovePacket
{
  get
  {
    return m_RemovePacket.Value;
  }
}

O método inicializador será chamado uma vez, quando o tipo Lazy for acessado pela primeira vez, e a segurança de streaming no caso de acessar a propriedade de vários threads ao mesmo tempo será garantida pelo tipo Lazy <T> (o modo de segurança do thread é controlado pelo segundo parâmetro do construtor Lazy ).

PVS-Studio Warning: V3131 A expressão 'target' é verificada quanto à compatibilidade com o tipo 'IAxe', mas é convertida no tipo 'Item'. HarvestTarget.cs 61

protected override void OnTarget( Mobile from, object targeted )
{
  ....
  else if ( m_System is Lumberjacking &&
            targeted is IAxe && m_Tool is BaseAxe )
  {
    IAxe obj = (IAxe)targeted;
    Item item = (Item)targeted;
    ....
  }
  ....
}

O alvo variável foi verificada por pertencer a digitar iAXE , mas ninguém verificou por pertencer a item , como os relatórios de análise.

PVS-Studio Warning: V3070 A variável não inicializada 'Zero' é usada ao inicializar a variável 'm_LastMobile'. Serial.cs 29
public struct Serial : IComparable, IComparable<Serial>
{
  private int m_Serial;

  private static Serial m_LastMobile = Zero;                // <=
  private static Serial m_LastItem = 0x40000000;

  public static Serial LastMobile { .... }
  public static Serial LastItem { .... }

  public static readonly Serial MinusOne = new Serial( -1 );
  public static readonly Serial Zero = new Serial( 0 );     // <=
  ....
  private Serial( int serial )
  {
    m_Serial = serial;
  }
  ....
}

Como tal, não há erro aqui, no entanto, escrever isso não é muito bom. Devido à atribuição de m_LastMobile ao valor de um zero não inicializado , uma estrutura será criada com o construtor padrão Serial () , o que levará à inicialização de m_Serial = 0 . E isso, por sua vez, é equivalente a chamar o novo Serial (0) . De fato, os desenvolvedores têm sorte de que o número de série seja igual a 0 , se houvesse algum outro valor, isso levaria a um erro.

PVS-Studio Warning: V3063 Uma parte da expressão condicional sempre é verdadeira se for avaliada: m_Serial <= 0x7FFFFFFF. Serial.cs 83

public bool IsItem
{
  get
  {
    return ( m_Serial >= 0x40000000 && m_Serial <= 0x7FFFFFFF );
  }
}

0x7FFFFFFF é o valor máximo possível que Int32 pode conter . Portanto, qualquer que seja o valor da variável m_Serial , ela será menor ou igual a 0x7FFFFFFF .

PVS-Studio Warning: V3004 A instrução 'then' é equivalente à instrução 'else'. Serialization.cs 1571

public override void WriteDeltaTime( DateTime value )
{
  ....
  try 
  { 
    d = new TimeSpan( ticks-now ); 
  }
  catch 
  {
    if( ticks < now ) 
      d = TimeSpan.MaxValue; 
    else 
      d = TimeSpan.MaxValue;
  }
  ....
}

O analisador avisa sobre um código suspeito no qual as ramificações verdadeira e falsa da instrução if coincidem completamente. Talvez um dos ramos deva ter TimeSpan.MinValue. O mesmo código foi encontrado em vários locais:

V3004 A instrução 'then' é equivalente à instrução 'else'. Item.cs 2103

public virtual void Serialize( GenericWriter writer )
{
  ....
  
  if( ticks < now ) 
    d = TimeSpan.MaxValue; 
  else 
    d = TimeSpan.MaxValue;
  
  ....
}

V3004 A instrução 'then' é equivalente à instrução 'else'. Serialization.cs 383

public override void WriteDeltaTime( DateTime value )
{
  ....
  
  if( ticks < now ) 
    d = TimeSpan.MaxValue; 
  else 
    d = TimeSpan.MaxValue;
  
  ....
}

Eu usei “This” por um motivo, é muito provável que, entre outras coisas, isso não ocorra sem copiar e colar, esses fragmentos de código parecem muito dolorosos.

Aviso PVS-Studio: V3051 Uma conversão de tipo excessiva. O objeto já é do tipo 'Item'. Mobile.cs 11237

public Item Talisman
{
  get
  {
    return FindItemOnLayer( Layer.Talisman ) as Item;
  }
}
public Item FindItemOnLayer( Layer layer )
{
  ....
}

Este aviso analisador pode ser obtido se houver uso excessivo do como operador . Não há erro nesta seção do código, mas também não faz sentido converter o objeto em seu próprio tipo. Aviso do

PVS-Studio: V3148 O valor potencial 'nulo' da conversão de 'toSet' para um tipo de valor pode levar a NullReferenceException. Properties.cs 502

public static string ConstructFromString( .... )
{
  object toSet;
  bool isSerial = IsSerial( type );

  if ( isSerial ) // mutate into int32
    type = m_NumericTypes[4];

  ....
  else if ( value == null )
  {
    toSet = null;
  }
  ....

  if ( isSerial ) // mutate back
    toSet = (Serial)((Int32)toSet);

  constructed = toSet;
  return null;
}

Nesta seção do código, estamos interessados ​​no caso em que o valor da variável é nulo . Em seguida, a variável toSet também é nula . Além disso, se a variável isSerial == true , toSet é convertido em Int32 , o que levará a NRE .

Você pode corrigir esse código adicionando, por exemplo, 0 por padrão:

toSet = (Serial)((Int32)(toSet ?? 0));

Aviso do PVS-Studio: V3031 Uma verificação excessiva pode ser simplificada. O '||' O operador está cercado pelas expressões opostas 'pack == null' e 'pack! = null'. BODBuyGump.cs 64
public override void OnResponse(Server.Network.NetState sender, RelayInfo info)
{
  ....
  if ( (pack == null) ||
       ((pack != null) &&
        (!pack.CheckHold(
                m_From,
                item,
                true,
                true,
                0,
                item.PileWeight + item.TotalWeight)) ) )
  {
    pv.SayTo(m_From, 503204);
    m_From.SendGump(new BOBGump(m_From, m_Book, m_Page, null));
  }
  ....
}

Este código pode ser simplificado, como o analisador relata:

if ((pack == null) || ((pack != null) && (!pack.CheckHold(....))))

À esquerda e à direita do operador '||' são o oposto no significado da expressão. Aqui, o pacote de verificação ! = Null é redundante, porque antes disso o pacote de condições opostas == null é verificado e essas expressões são separadas pelo operador '||'. Esta linha pode ser reduzida da seguinte forma:

if (pack == null || !pack.CheckHold(....))

PVS-Studio Warning: V3080 Possível desreferência nula. Considere inspecionar 'vencedor'. CTF.cs 1302

private void Finish_Callback()
{
  ....
  CTFTeamInfo winner = ( teams.Count > 0 ? teams[0] : null );

  .... 

  m_Context.Finish( m_Context.Participants[winner.TeamID] as Participant );
}

Vamos dizer times.Count é 0 . Então vencedor = nulo. Além disso, no código, a propriedade winner.TeamID é acessada sem verificar nulo , o que resultará no acesso por uma referência nula.

PVS-Studio Warning: 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; StormsEye.cs 87

public static void Gain( Mobile from, Skill skill ) 
{
  ....
  if ( from.Player && 
     ( skills.Total / skills.Cap ) >= Utility.RandomDouble())
  ....
}

Este fragmento contém o código de operação de dividir as variáveis skills.Total e skills.Cap , que são do tipo int , e o resultado é depois convertido implicitamente para digitar duplo , como os relatórios do analisador.

PVS-Studio Warning: V3085 O nome do campo 'typeofObject' em um tipo aninhado é ambíguo. O tipo externo contém campo estático com nome idêntico. PropsGump.cs 744

private static Type typeofObject = typeof( object );
....
private class GroupComparer : IComparer
{
  ....
  private static Type typeofObject = typeof( Object );
  ....
}

A variável typeofObject foi criada nesta seção do código em uma classe aninhada . Seu problema é que na classe externa existe uma variável com o mesmo nome e podem ocorrer erros devido a isso. É melhor não permitir isso, a fim de reduzir a probabilidade de tais erros devido à desatenção.

PVS-Studio Warning: V3140 Os acessadores de propriedades usam diferentes campos de apoio. WallBanner.cs 77

private bool m_IsRewardItem;

[CommandProperty( AccessLevel.GameMaster )]
public bool IsRewardItem
{
  get{ return m_IsRewardItem; }
  set{ m_IsRewardItem = value; InvalidateProperties(); }
}

private bool m_East;

[CommandProperty( AccessLevel.GameMaster )]
public bool East
{
  get{ return m_East; }
  set{ m_IsRewardItem = value; InvalidateProperties(); }
}

E aqui há um erro que apareceu devido a copiar e colar. O método de acesso definido da propriedade East foi atribuir um valor para m_East , não m_IsRewardItem . Avisos do

PVS-Studio:

V3012 O operador '?:', Independentemente de sua expressão condicional, sempre retorna um e o mesmo valor: 0xe7f. TreasureChestLevel2.cs 52

V3012 O operador '?:', Independentemente de sua expressão condicional, sempre retorna um e o mesmo valor: 0xe77. TreasureChestLevel2.cs 57

private void SetChestAppearance()
{
  bool UseFirstItemId = Utility.RandomBool();

  switch( Utility.RandomList( 0, 1, 2, 3, 4, 5, 6, 7 ) )
  {
    ....
    case 6:// Keg
      this.ItemID = ( UseFirstItemId ? 0xe7f : 0xe7f );
      this.GumpID = 0x3e;
      break;

    case 7:// Barrel
      this.ItemID = ( UseFirstItemId ? 0xe77 : 0xe77 );
      this.GumpID = 0x3e;
      break;
  }
}

Algum tipo de ilusão de escolha :) Independentemente do valor de UseFirstItemId , this.ItemID será 0xe7f no primeiro caso ou 0xe77 no segundo.

Aviso PVS-Studio: V3066 Possível ordem incorreta de argumentos passada para o método 'OnSwing': 'defender' e 'atacante'. BaseWeapon.cs 1188

public virtual int AbsorbDamageAOS( Mobile attacker,
                                    Mobile defender,
                                    int damage )
{
  ....
  if ( weapon != null )
  {
    defender.FixedParticles(0x3779,
                            1,
                            15,
                            0x158B,
                            0x0,
                            0x3,
                            EffectLayer.Waist);
    weapon.OnSwing( defender, attacker );
  }
  ....
}

public virtual TimeSpan OnSwing( Mobile attacker, Mobile defender )
{
  return OnSwing( attacker, defender, 1.0 );
}

Parecia suspeito para o analisador que o método OnSwing () recebesse argumentos na ordem inversa. Talvez isso seja devido a um erro.

PVS-Studio Warning: V3092 Interseções de faixa são possíveis dentro de expressões condicionais. Exemplo: if (A> 0 && A <5) {...} else if (A> 3 && A <9) {...}. HouseFoundation.cs 1883

public static bool IsFixture( int itemID )
{
  ....
  else if( itemID >= 0x319C && itemID < 0x31B0 ) 
    return true;
  // ML doors
  else if( itemID == 0x2D46 ||
           itemID == 0x2D48 ||
           itemID == 0x2FE2 ||
           itemID == 0x2FE4 )
    return true;
  else if( itemID >= 0x2D63 && itemID < 0x2D70 )
    return true;
  else if( itemID >= 0x319C && itemID < 0x31AF ) 
    return true;
  ....
}

As faixas testadas sob condições se cruzam. Isso pareceu suspeito para o analisador. Mesmo que esse trecho de código funcione corretamente, ainda vale a pena corrigi-lo. Imagine uma situação em que precisássemos reescrever o corpo do último caso, para que, se a condição for verdadeira, o método retorne falso . Se itemID for igual a, digamos, 0x319C , o método ainda retornará true . Por sua vez, isso acarreta a perda de tempo para procurar erros.

Conclusão


O RunUO foi criado há muito tempo, muito trabalho foi feito, no entanto, usando o exemplo deste projeto, você pode ver os benefícios do uso de análise estática em projetos com histórico. O analisador de 543 mil linhas de código de projeto emitiu cerca de 500 avisos (sem contar o nível Baixo), a maioria dos quais não apareceu no artigo devido à sua uniformidade. Para se familiarizar com o resultado da análise em mais detalhes, sugiro o uso de uma licença gratuita para projetos de código aberto .



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Ekaterina Nikiforova. Verifique o RunUO pelo PVS-Studio Analyzer .

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


All Articles