Проверка RunUO анализатором PVS-Studio

• Введение
• Участки кода, привлекшие внимание при рассмотрении отчета анализатора
• Заключение

Данная статья посвящена проверке проекта RunUO при помощи статического анализатора PVS-Studio. RunUO - эмулятор серверного ПО для Ultima Online, игры, в свое время завоевавшей сердца многих фанатов жанра MMORPG.

Введение

RunUO – эмулятор серверного программного обеспечения для MMORPG Ultima Online. Цель этого проекта - создать стабильное программное обеспечение, которое будет способно конкурировать с официальными серверами EA Games. RunUO был создан в далеком 2002г, но он не теряет актуальность и активно используется и по сей день.

Цель проверки проекта – популяризация темы статического анализа. Мы проверяем различные проекты – игры (пример), библиотеки (пример), мессенджеры (пример), браузеры (пример) и многое другое (пример, пример, пример) чтобы привлечь внимание самой разнообразной аудитории. Этими статьями мы пытаемся обратить внимание на важность использования статического анализа при разработке. Статический анализ позволяет сделать код надежнее и безопаснее. Также, при его регулярном использовании можно найти и устранить ошибки на самых ранних этапах. Это экономит время и силы разработчиков, ведь никому не хочется тратить 50 часов на поиск ошибки, которую может найти анализатор.

Также мы помогаем open-source сообществу. Посредством статей с найденными ошибками мы делаем вклад в развитие open-source. Однако в статьях мы разбираем далеко не все предупреждения. Некоторые предупреждения показались нам слишком скучными для того, чтобы попасть в статью, некоторые оказались ложными срабатываниями и т.д. Поэтому мы готовы предоставить бесплатную лицензию для проектов с открытым исходным кодом. Тем более то, что нам показалось скучным для статьи, может показаться довольно интересным для разработчиков проверяемого проекта, ведь разработчикам проекта всё же виднее, какие проблемы наиболее критичны.

Участки кода, привлекшие внимание при рассмотрении отчета анализатора

Предупреждение PVS-Studio: V3010 The return value of function 'Intern' is required to be utilized. 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 );
}

Возвращаемое значение метода Intern() нигде не учитывается, на что указывает анализатор. Возможно, это ошибка или избыточный код.

Предупреждение PVS-Studio: V3017 A pattern was detected: (item is BasePotion) || ((item is BasePotion) && ...). The expression is excessive or contains a logical error. 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 )
  {
    ....
  }
}

Тут есть подвыражения, которые можно упростить. Выпишу их, чтобы было нагляднее:

if (item is BasePotion || ( item is BasePotion && Core.ML ))

Допустим, item is BasePotion = true, тогда условие будет истинным, несмотря на Core.ML. Если же item is BasePotion = false, то условие будет ложным, опять же несмотря на значение Core.ML. Такой код чаще всего является просто избыточным, однако бывают ситуации хуже, когда программист ошибся и написал не ту переменную во втором подвыражении.

Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'bPlayerOnly' and '!bPlayerOnly'. BaseCreature.cs 3005

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

Этот код является или избыточным, или ошибочным. Его проблема в том, что по разные стороны '||' стоят разные по смыслу подвыражения. Если мы сократим его вот так:

if ( m.Player || !bPlayerOnly )

то ровным счетом ничего не изменится.

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'deed is SmallBrickHouseDeed' to the left and to the right of the '||' operator. 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 )
      ....
}

Думаю, тут не стоит ничего объяснять, очередной или ошибочный, или избыточный код.

Предупреждение PVS-Studio: V3067 It is possible that 'else' block was forgotten or commented out, thus altering the program's operation logics. 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;

  ....
}

Достаточно редкое предупреждение. Анализатору показалось подозрительным форматирование кода после директивы #endregion. Если не вчитываться в код, выглядит, будто строчка

i.Movable = !locked;

выполнится в любом случае, вне зависимости от переменной i. Возможно, тут были забыты фигурные скобки... В общем, разработчикам стоит проверить этот код.

Предупреждение PVS-Studio: V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Earthquake.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
  {
    ....
  }
}

В этом коде, возможно, пропущены фигурные скобки. Такой вывод можно сделать из-за странного форматирования кода в теле if ( !m.Player ).

Предупреждение PVS-Studio: V3083 Unsafe invocation of event 'ServerStarted', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. EventSink.cs 921

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

В этом методе используется потенциально небезопасный вызов обработчика события RefreshStarted, на что и указывает анализатор.

Почему же он опасен? Представим такую ситуацию. У события ServerStarted есть только один подписчик. И в момент между проверкой на null и непосредственными вызовом обработчика события ServerStarted() в другом потоке была произведена отписка от этого события. Это приведет к возникновению исключения NullReferenceException.

Самый простой способ не допустить эту ситуацию – обеспечить безопасный вызов события с помощью оператора '?.':

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

Предупреждение PVS-Studio: V3054 Potentially unsafe double-checked locking. Use volatile variable(s) or synchronization primitives to avoid this. 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;
  }
}

Предупреждение анализатора связано с небезопасным использованием шаблона "блокировки с двойной проверкой" (double checked locking). Как видно из приведённого выше кода, блокировка с двойной проверкой была применена для реализации порождающего шаблона - Одиночки. Когда мы пытаемся получить экземпляр класса Packet, обращаясь к свойству RemovePacket, геттер осуществляет проверку поля m_RemovePacket на равенство нулю. Если проверка проходит, то мы попадаем в тело оператора lock, где и происходит инициализация поля m_RemovePacket. Интересная ситуация возникает в тот момент, когда главный поток уже инициализировал переменную m_RemovePacket через конструктор, но ещё не вызвал метод SetStatic(). Теоретически другой поток может обратиться к свойству RemovePacket именно в тот самый неудобный момент. Проверка m_RemovePacket на равенство нулю уже не пройдёт, и вызывающий поток получит ссылку на неполностью готовый к использованию объект. Для решения этой проблемы в теле оператора lock можно создавать промежуточную переменную класса Packet, инициализировать её через вызов конструктора и метод SetStatic(), а уже после присваивать её переменной m_RemovePacket. В этом случае тело оператора lock может выглядеть следующим образом:

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

Кажется, что проблема исправлена, и код будет работать как ожидается. Но это не так.

Остался ещё один момент: анализатор не просто так предлагает использовать ключевое слово volatile. В релизной версии программы компилятор может выполнить оптимизацию и переупорядочить строки вызова метода SetStatic() и присваивание переменной instance полю m_RemovePacket (с точки зрения компилятора семантика программы не будет нарушена). И мы опять возвращаемся к тому же, с чего и начинали - возможности получения недоинициализированной переменной m_RemovePacket. Нельзя точно сказать, когда может произойти это переупорядочивание, и случится ли оно вообще: на это может повлиять версия CLR, архитектура используемого процессора и т.п. Обезопаситься от подобного сценария всё же стоит, и одним из решений (однако не самым производительным) будет использование ключевого слова volatile. Переменная, которая будет объявлена с модификатором volatile, не будет подвержена перестановкам при проведении оптимизации компилятором. Окончательно исправленный код может выглядеть следующим образом:

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

Иногда использование volatile поля может быть нежелательно из-за накладных расходов на обращение к такому полю. Не будем подробно останавливаться сейчас на этой теме, отметив просто, что в рассматриваемом примере атомарная запись поля нужна лишь один раз (при первом обращении к свойству), однако объявление поля как volatile приведёт к тому, что каждое его чтение и запись компилятор будет делать атомарно, что может оказаться неоптимальным с точки зрения производительности.

Поэтому, другим подходом к исправлению данного предупреждения анализатора, позволяющим избежать дополнительных накладных затрат от объявления volatile поля, будет использование типа Lazy<T> для backing поля m_RemovePacket вместо блокировки с двойной проверкой. В этом случае тело геттера может быть заменено методом-инициализатором, который будет передаваться в конструктор экземпляра 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;
  }
}

Метод-инициализатор будет вызван один раз, при первом обращении к экземпляру типа Lazy, причём потоковая безопасность в случае одновременного обращения к свойству из нескольких потоков будет гарантироваться самим типом Lazy<T> (режим потокобезопасности управляется вторым параметром конструктора Lazy).

Предупреждение PVS-Studio: V3131 The expression 'targeted' is checked for compatibility with the type 'IAxe', but is casted to the 'Item' type. 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;
    ....
  }
  ....
}

Переменная targeted была проверена на принадлежность к типу IAxe, но на принадлежность к Item ее никто не проверял, о чем и сообщает анализатор.

Предупреждение PVS-Studio: V3070 Uninitialized variable 'Zero' is used when initializing the 'm_LastMobile' variable. 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;
  }
  ....
}

Как таковой ошибки тут нет, однако так писать не очень хорошо. Из-за присвоения m_LastMobile значения неинициализированной Zero, будет создана структура с конструктором по умолчанию Serial(), что приведет к инициализации m_Serial=0. А это, в свою очередь, равносильно вызову new Serial(0). На самом деле, разработчикам повезло, что serial и должен быть равным 0, если б там должно было находиться какое-либо другое значение, это привело бы к ошибке.

Предупреждение PVS-Studio: V3063 A part of conditional expression is always true if it is evaluated: m_Serial <= 0x7FFFFFFF. Serial.cs 83

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

0x7FFFFFFF является максимально возможным значением, которое может вместить в себя Int32. Следовательно, какое бы значение не было у переменной m_Serial, оно в любом случае будет меньше или равно 0x7FFFFFFF.

Предупреждение PVS-Studio: V3004 The 'then' statement is equivalent to the 'else' statement. 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;
  }
  ....
}

Анализатор предупреждает о подозрительном участке кода, в котором истинная и ложная ветви оператора if полностью совпадают. Возможно, в одной из веток должно быть TimeSpan.MinValue. Этот же код встречался еще в нескольких местах:

V3004 The 'then' statement is equivalent to the 'else' statement. Item.cs 2103

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

V3004 The 'then' statement is equivalent to the 'else' statement. Serialization.cs 383

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

"Этот же" я использовала не просто так, очень вероятно, что тут, ко всему прочему, не обошлось и без копипасты, больно похоже выглядят эти фрагменты кода.

Предупреждение PVS-Studio: V3051 An excessive type cast. The object is already of the 'Item' type. Mobile.cs 11237

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

Это предупреждение анализатора можно получить при наличии избыточного использования оператора as. В этом участке кода ошибки нет, но также нет и смысла приводить объект к своему же типу.

Предупреждение PVS-Studio: V3148 Casting potential 'null' value of 'toSet' to a value type can lead to 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;
}

В этом участке кода нас интересует тот случай, когда переменная value равна null. Тогда переменной toSet тоже присваивается null. Далее, если переменная isSerial == true, то toSet приводится к Int32, что приведет к NRE.

Можно исправить этот код, добавив, к примеру, 0 по умолчанию:

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

Предупреждение PVS-Studio: V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'pack == null' and '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));
  }
  ....
}

Этот код можно упростить, о чем и сообщает анализатор:

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

Слева и справа от оператора '||' стоят противоположные по смыслу выражения. Тут проверка pack != null является избыточной, так как перед этим проверяется противоположное ему условие pack == null, причём эти выражения разделены оператором '||'. Эту строчку можно сократить следующим образом:

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

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'winner'. 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 );
}

Допустим, teams.Count равно 0. Тогда winner = null. И далее в коде происходит обращение к свойству winner.TeamID без проверки на null, что приведет к доступу по нулевой ссылке.

Предупреждение PVS-Studio: V3041 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: 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())
  ....
}

В этом фрагменте кода присутствует операция деления переменных skills.Total и skills.Cap, имеющих тип int, а результат потом неявно преобразуется к типу double, о чем и сообщает анализатор.

Предупреждение PVS-Studio: V3085 The name of 'typeofObject' field in a nested type is ambiguous. The outer type contains static field with identical name. PropsGump.cs 744

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

В этом участке кода во вложенном классе была создана переменная typeofObject. Проблема ее в том, что во внешнем классе имеется переменная с тем же именем, и из-за этого могут возникнуть ошибки. Лучше такого не допускать, дабы уменьшить вероятность таких ошибок из-за невнимательности.

Предупреждение PVS-Studio: V3140 Property accessors use different backing fields. 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(); }
}

А тут налицо ошибка, появившаяся из-за копипасты. Метод доступа set свойства East должен был присваивать значение для m_East, a не m_IsRewardItem.

Предупреждения PVS-Studio:

V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 0xe7f. TreasureChestLevel2.cs 52

V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: 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;
  }
}

Какая-то иллюзия выбора :) Вне зависимости от значения UseFirstItemId, this.ItemID будет равно или 0xe7f в первом случае, или 0xe77 во втором.

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'OnSwing' method: 'defender' and 'attacker'. 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 );
}

Анализатору показалось подозрительным то что методу OnSwing() передали аргументы в обратном порядке. Возможно, это является следствием ошибки.

Предупреждение PVS-Studio: V3092 Range intersections are possible within conditional expressions. Example: 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;
  ....
}

Диапазоны, проверяемые в условиях, пересекаются. Это показалось подозрительным анализатору. Даже если этот фрагмент кода работает корректно, все равно стоит его поправить. Представим ситуацию, когда нам понадобилось переписать тело последнего if так, чтобы при истинности условия метод возвращал false. При itemID равном, допустим, 0x319C, метод все равно возвратит true. Это, в свою очередь, повлечет за собой потери времени на поиск ошибки.

Заключение

RunUO был создан довольно давно, была проделана огромная работа, однако на примере этого проекта можно убедиться в пользе применения статического анализа на проектах "с историей". Анализатор на 543 тысячи строк кода проекта выдал около 500 предупреждений (не считая уровня Low), большая часть которых в статью не попала по причине их однотипности. Чтобы ознакомиться с результатом анализа поподробнее, предлагаю воспользоваться бесплатной лицензией для open source проектов.