RunUO check by the PVS-Studio analyzer

• Introduction
• Most striking code fragments from the analyzer's report
• Conclusion

This article covers the check of the RunUO project using the static PVS-Studio analyzer. RunUO is the emulator of server software for Ultima Online, the game that once won the hearts of many MMORPG fans.

Introduction

RunUO is a server software emulator for MMORPG Ultima Online. The goal of this project is to create stable software that will be able to compete with the official servers of EA Games. RunUO was created back in 2002, but is relevant and actively used to this day.

The purpose of this project review is to popularize the topic of static analysis. We check various projects - games (example), libraries (example), messengers (example), browsers (example) and more (example, example, example) to catch an eye of the most diverse audience. With these articles, we try to draw attention to the importance of using static analysis in development process. Static analysis makes the code more reliable and safer. Also, with its regular use, you can find and fix bugs at the earliest stages. This saves time and efforts of developers, as no one wants to spend 50 hours seeking out an error that the analyzer can detect.

We also help the open-source community. By posting articles with found errors, we contribute to the development of open-source community. However, we don't break all warnings down in the articles. As for this article, some warnings seemed too ordinary to get into it, some appeared to be false positives and so on. Therefore, we're ready to provide with a free license for working with open source projects. Besides, what we considered uninteresting might seem quite intriguing for developers of open source project under the check. After all, project developers know best which problems are most critical.

Most striking code fragments from the analyzer's report

PVS-Studio warning: 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 );
}

The return value of the Intern() method isn't taken into account anywhere, as the analyzer points out. Maybe it's an error or redundant code.

PVS-Studio warning: 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 )
  {
    ....
  }
}

There are sub-expressions here that can be simplified. I'll cite them for clarity:

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

Suppose item is BasePotion = true, then the condition will be true despite Core.ML. But if item is BasePotion = false, the condition will be false, again despite the Core.ML value. In most cases, such code is simply redundant, but there are worse cases, when the developer made a mistake and wrote a wrong variable in the second sub-expression.

PVS-Studio warning: 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 )
  {
    ....
  }
  ....
}

This code is either redundant or erroneous. The problem is that there are different sub-expressions on different sides of '||'. If we narrow it down to this:

if ( m.Player || !bPlayerOnly )

nothing will change.

PVS-Studio warning: 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 )
      ....
}

I don't think there's anything to explain. It's another erroneous or redundant piece of code.

PVS-Studio warning: 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;

  ....
}

It's a quite rare warning. The analyzer found it suspicious to format the code after the #endregion directive in such a way. If we don't read the code carefully, it looks like the line

i.Movable = !locked;

will execute anyway regardless of the variable i. Perhaps, authors forgot to write curly brackets here... Well, code authors should revise this fragment.

PVS-Studio warning: 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
  {
    ....
  }
}

This code probably lacks curly brackets. We can conclude it because of strange code formatting in the if ( !m.Player ) body.

PVS-Studio warning: 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();
}

In this method, unsafe call of the RefreshStarted event handler is used, as the analyzer indicates.

Why is it dangerous? Imagine this situation. The ServerStarted event has only one subscriber. In the moment between checking for null and directly calling the ServerStarted() event handler, someone unsubscribed from the event in another thread. This will result in NullReferenceException.

The easiest way to prevent this situation is to ensure that the event is safely called with the '?.' operator:

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

PVS-Studio warning: 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;
  }
}

The analyzer warning relates to unsafe usage of the double checked locking pattern. As can be seen from the above code, double checked locking was applied to implement the singleton pattern. When attempting to get the Packet class instance and addressing the RemovePacket property, the getter checks the m_RemovePacket field for null. If the check is successful, we get into the body of the lock operator, where the field m_RemovePacket gets initialized. The plot thickens when the main thread has already initialized the m_RemovePacket variable through the constructor, but hasn't called the SetStatic() method yet. In theory, another thread can access the RemovePacket property at this very awkward moment. The check of m_RemovePacket for null will fail and the caller thread will get the reference to a half ready-to-use object. To solve this problem, we can create an intermediate variable of Packet class in the body of the lock operator, initialize the variable via the constructor and the SetStatic() method, and after assign it to the m_RemovePacket variable. This way, the body of the lock operator might look as follows:

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

It seems that the problem has been fixed and the code will work as expected. But not so fast.

Here's another thing: the analyzer offers to use the volatile keyword for a reason. In the release version of the program, the compiler might optimize and reorder calling lines of the SetStatic() method and assignment of the instance variable to the m_RemovePacket field (from the compiler's point of view, program semantics won't break). Here we get back to the point where we started - the m_RemovePacket variable might be uninitialized. We can't say exactly when this reordering may occur. We are even not sure if it happens at all, as the CLR version, the architecture of the used processor and other factors might affect it. It's still worth preventing this scenario. In this regard, one of the solutions (not the most productive) will be usage of the keyword volatile. The variable declared with the volatile modifier won't be object to displacements during compiler optimizations. The final code version might look as follows:

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

In some cases, it's undesirable to use the volatile field due to some cost of accessing this field. Let's not dwell on this issue, noting simply that in this example, the atomic field writing is needed only once (when first accessing the property). However, volatile field declaration will lead to the fact that the compiler will atomically perform its each reading and writing, which might be non-optimal in terms of performance.

Therefore, let's consider another way to avoid this analyzer warning. We can use the Lazy<T> type for backing m_RemovePacket field instead of double checked locking. As a result, we'll get rid of potential non-optimizations from declaring the volatile field. In this case, the body of the getter can be replaced by the initializing method, which will be passed to the constructor of the Lazy<T> instance:

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

The initializing method will be called only once when first accessing the instance of the Lazy type. In doing so, the Lazy<T> type will ensure thread security in case of a simultaneous multi-thread access to a property. The thread security mode is controlled by the second parameter of the Lazy constructor.

PVS-Studio warning: 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;
    ....
  }
  ....
}

The targeted variable was checked for the IAxe type, but not for the Item type, as reported by the analyzer.

PVS-Studio warning: 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;
  }
  ....
}

Actually there is no error here, but writing in such a way is not the best practice. Due to m_LastMobile value assignment to Zero, the structure with the Serial() default constructor will be created, leading to m_Serial=0 initialization. Which is akin to calling new Serial(0). In fact, developers got lucky that serial is meant to be equal to 0. If another value had to be there, this would lead to an error.

PVS-Studio warning: 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 is the maximum possible value that can contain Int32. Therefore, whatever value the m_Serial variable had, it would be less or equal to 0x7FFFFFFF.

PVS-Studio warning: 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;
  }
  ....
}

The analyzer warns of a suspicious piece of code in which the true and false branches of the if operator fully match. Perhaps, TimeSpan.MinValue has to be in one of the branches. The same code was found in several other places:

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

I used "the same code" expression for reason. It seems to me that copypaste is at play here as well, these two fragments look suspiciously alike.

PVS-Studio warning: 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 )
{
  ....
}

This analyzer warning triggers when having redundant usage of the as operator. There is no error in this code fragment, but it also makes no sense to cast the object to its own type.

PVS-Studio warning: 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;
}

In this code section, let's pay attention to the scenario when the value variable is null. This way, null is assigned to the toSet variable. Further, if the variable isSerial == true, then toSet is cast to Int32, resulting in NRE.

We can fix this code by adding 0 by default:

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

PVS-Studio warning: 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));
  }
  ....
}

As the analyzer tells us, we can simplify this code:

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

On the left and right of the '||' operator, there are opposite expressions. Here the pack != null check is redundant, as before that the opposite condition is checked: pack == null, and these expressions are separated by the operator '||'. We can shorten this line as follows:

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

PVS-Studio warning: 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 );
}

Suppose teams.Count is 0. Then winner = null. Further in the code, the winner.TeamID property is accessed without a check for null, leading to access by null reference.

PVS-Studio warning: 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())
  ....
}

In this code fragment, the skills.Total variable is divided into skills.Cap (variables are of int type); the result is then implicitly converted into the double type, this is what the analyzer tells us about.

PVS-Studio warning: 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 );
  ....
}

In the above code, the typeofObject variable was created in the inner class. Its problem is that there is a variable with the same name in the outer class, and that can cause errors. It is better not to allow this in order to reduce the probability of such errors due to inattention.

PVS-Studio warning: 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(); }
}

Here we can immediately notice an error that appeared due to copypaste. The set access method of the East property was supposed to assign the value for m_East, not for m_IsRewardItem.

PVS-Studio warnings:

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

Here comes the illusion of choice :) Regardless of the UseFirstItemId value, this.ItemID will still be equal either to 0xe7f in the first case, or to 0xe77- in the second.

PVS-Studio warning: 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 );
}

The analyzer found it suspicious that the OnSwing() method was passed arguments in reverse order. This may be the result of a bug.

PVS-Studio warning: 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;
  ....
}

The ranges checked in the conditions above intersect. This seemed suspicious to the analyzer. Even if this piece of code works correctly, it's still worth tweaking it. Let's imagine the situation where we need to rewrite the body of the last if so that the method will return false if the condition is true. If itemID equals, let's say, 0x319C, the method will return true anyway. This, in turn, will result in a waste of time searching for the bug.

Conclusion

RunUO appeared quite long ago, a lot of work has been done. At the same time, using this project as an example we can fully realize the benefits of static analysis application on projects with a long history. The analyzer issued about 500 warnings for 543,000 lines of code (without the Low level), most of which didn't get into the article because of their similarity. You're welcome to check out the free license for open source projects to find out more about the analysis results.