# RunUO check by the PVS\-Studio analyzer

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\. 

![0711_runuo/image1.png](https://import.viva64.com/docx/blog/0711_runuo/image1.png)

## Introduction

[RunUO](https://github.com/runuo/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](https://pvs-studio.com/en/blog/posts/csharp/0704/)\), libraries \([example](https://pvs-studio.com/en/blog/posts/csharp/0683/)\), messengers \([example](https://pvs-studio.com/en/blog/posts/cpp/0434/)\), browsers \([example](https://pvs-studio.com/en/blog/posts/cpp/0262/)\) and more \([example](https://pvs-studio.com/en/blog/posts/java/0697/), [example](https://pvs-studio.com/en/blog/posts/cpp/0674/), [example](https://pvs-studio.com/en/blog/posts/cpp/0083/)\) 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](https://pvs-studio.com/en/blog/posts/cpp/0221/) 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](https://pvs-studio.com/en/blog/posts/0614/) 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

```cpp
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

```cpp
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:

```cpp
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

```cpp
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:

```cpp
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

```cpp
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

```cpp
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 

```cpp
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

```cpp
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

```cpp
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:

```cpp
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

```cpp
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:

```cpp
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:

```cpp
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:

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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:

```cpp
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

```cpp
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: 

```cpp
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:

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

**PVS\-Studio** **warning:** V3080 Possible null dereference\. Consider inspecting 'winner'\. CTF\.cs 1302

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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

```cpp
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](https://pvs-studio.com/en/docs/manual/0007/)\), 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](https://pvs-studio.com/en/blog/posts/0614/) to find out more about the analysis results\.