PVS-Studio analyzer RunUO check

Picture 1

This article is devoted to checking the RunUO project using the PVS-Studio static analyzer. RunUO is a server software emulator for Ultima Online, a 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 it does not lose relevance and is actively used to this day.

The purpose of the project verification is to popularize the topic of static analysis. We check various projects - games ( example ), libraries ( example ), messengers ( example ), browsers ( example ) and much more ( example , example , example) to attract the attention of a diverse audience. With these articles, we are trying to draw attention to the importance of using static analysis in development. Static analysis makes the code more reliable and safer. Also, with its regular use, you can find and eliminate errors at the earliest stages. This saves developers time and effort, because no one wants to spend 50 hours searching for the error that the analyzer can find.

We also help the open-source community. Through articles with errors found, we contribute to the development of open-source. However, in the articles we do not analyze all warnings. Some warnings seemed too boring for us to get into the article, some turned out to be false positives, etc. Therefore, we are ready to provide a free license for open source projects. Moreover, what seemed to us boring for the article may seem quite interesting for the developers of the project being tested, because the developers of the project are still more aware of which problems are most critical.

Pieces of code that caught attention when examining the analyzer 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 is not taken into account anywhere, as indicated by the analyzer. Perhaps this is a bug 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 subexpressions that can be simplified. I will write them out so that it is more clear:
if (item is BasePotion || ( item is BasePotion && Core.ML ))

Suppose, item is BasePotion = to true , then the condition is true, despite the Core.ML . If the item is BasePotion = to false , then the condition is false, again regardless of the value Core.ML . Such code is most often simply redundant, however, there are worse situations when the programmer made a mistake and wrote the wrong variable in the second subexpression.

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. His problem is that on both sides of '||' are different in the sense of subexpression. If we cut it like this:

if ( m.Player || !bPlayerOnly )

then 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 think itโ€™s not worthwhile to explain anything here, the next or wrong, or redundant 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;

  ....
}

Quite a rare warning. The analyzer found the code formatting suspicious after the #endregion directive to be suspicious. If you donโ€™t read the code, it looks like a line

i.Movable = !locked;

will be executed in any case, regardless of the variable i . Perhaps curly braces were forgotten here ... In general, developers should check this code.

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

Curly braces may be missing in this code. This conclusion can be made due to the strange formatting of the code in the body of if (! M.Player) .

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

This method uses a potentially unsafe call to the RefreshStarted event handler , as the analyzer indicates.

Why is it dangerous? Imagine this situation. The ServerStarted event has only one subscriber. And at the moment between checking for null and directly calling the ServerStarted () event handler in another thread, an unsubscription was made from this event. This will throw a NullReferenceException .

The easiest way to prevent this situation is to provide a safe event call using 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 is associated with the unsafe use of the double checked locking pattern. As you can see from the code above, a double-check lock was applied to implement the generative pattern - Loners. When we try to get an instance of the Packet class by accessing the RemovePacket property , the getter checks the m_RemovePacket field for equality to zero. If the check passes, then we get into the body of the lock statement , where the m_RemovePacket field is initialized . An interesting situation arises at the moment when the main thread has already initialized the m_RemovePacket variable through the constructor, but has not yet called the methodSetStatic (). Theoretically, another thread can access the RemovePacket property at the very inconvenient moment. Checking m_RemovePacket for equality to zero will no longer fail, and the calling thread will receive a link to an object that is not ready to use. To solve this problem, you can create an intermediate variable of the Packet class in the body of the lock statement , initialize it through a constructor call and the SetStatic () method , and then assign it to the m_RemovePacket variable . In this case, the body of the lock statement might look like this:

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

The problem seems to be fixed and the code will work as expected. But this is not so.

There is one more point: the analyzer does not just suggest using the volatile keyword . In the release version of the program, the compiler can optimize and reorder the lines of the call to the SetStatic () method and assign the instance variable to the m_RemovePacket field (from the point of view of the compiler, the semantics of the program will not be violated). And we again return to the same point where we started - the possibility of obtaining the un-initialized variable m_RemovePacket. It is impossible to say exactly when this reordering can occur, and whether it will happen at all: it can be affected by the CLR version, the architecture of the processor used, etc. Itโ€™s still worth protecting from such a scenario, and one of the solutions (however, not the most productive) will be to use the volatile keyword . A variable that will be declared with the volatile modifier will not be subject to permutations during optimization by the compiler. A finally fixed code may look like this:

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

Sometimes using a volatile field may be undesirable due to the overhead of accessing such a field. We will not dwell on this topic in detail, simply noting that in the example under consideration, an atomic field record is needed only once (when the property is first accessed), however, declaring the field as volatile will cause the compiler to do its atomic reading and writing that may not be optimal in terms of performance.

Therefore, another approach to correcting this analyzer warning to avoid additional overhead from declaring a volatile field is to use the Lazy <T> type for backing the m_RemovePacket fieldinstead of double check locking. In this case, the getter body can be replaced by the initializer 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 initializer method will be called once, when the Lazy type is accessed for the first time, and streaming security in case of accessing the property from several threads at the same time will be guaranteed by the Lazy <T> type (the thread safety 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 belonging to type IAxe , but no one checked for belonging to Item , as the analyzer reports.

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

As such, there is no mistake here, however, writing this is not very good. Due to the assignment of m_LastMobile to the value of an uninitialized Zero , a structure with the default constructor Serial () will be created , which will lead to the initialization of m_Serial = 0 . And this, in turn, is equivalent to calling new Serial (0) . In fact, the developers are lucky that serial should be equal to 0 , if there should have been any other value, 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 Int32 can contain . Therefore, whatever value the m_Serial variable has , it will in any case be less than 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 about a suspicious piece of code in which the true and false branches of the if statement completely coincide. Perhaps one of the branches should have TimeSpan.MinValue. The same code was found in several 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 โ€œThisโ€ for a reason, it is very likely that, among other things, it could not do without copy-paste, these code fragments look very painful.

Warning 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 )
{
  ....
}

This analyzer warning can be obtained if there is excessive use of the as operator . There is no error in this section of the code, 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 section of code, we are interested in the case when the variable value is null . Then the toSet variable is also null . Further, if the variable isSerial == true , then toSet is cast to Int32 , which will lead to NRE .

You can fix this code by adding, for example, 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));
  }
  ....
}

This code can be simplified, as the analyzer reports:

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

To the left and right of the operator '||' there are opposite in meaning expressions. Here the check pack! = Null is redundant, because before that the opposite condition pack == null is checked , and these expressions are separated by the operator '||'. This line can be shortened 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 );
}

Lets say teams.Count is 0 . Then winner = null. And further in the code, the winner.TeamID property is accessed without checking for null , which will result in access by a 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())
  ....
}

This code fragment contains the operation of dividing the variables skills.Total and skills.Cap , which are of type int , and the result is then implicitly converted to type double , as the analyzer reports.

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

The variable typeofObject was created in this section of code in a nested class . Its problem is that in the outer class there is a variable with the same name, and errors can occur due to this. It is better not to allow this in order to reduce the likelihood 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(); }
}

And here there is a mistake that appeared due to copy-paste. The set accessor of the East property was to assign a value for m_East , not 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;
  }
}

Some kind of illusion of choice :) Regardless of the value of UseFirstItemId , this.ItemID will be either 0xe7f in the first case, or 0xe77 in the second.

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

It seemed suspicious to the analyzer that the OnSwing () method was passed arguments in the reverse order. Perhaps this is due to an error.

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

Ranges tested under conditions intersect. This seemed suspicious to the analyzer. Even if this piece of code works correctly, itโ€™s still worth fixing it. Imagine a situation where we needed to rewrite the body of the last if so that, if the condition is true, the method returns false . If itemID is equal to, say, 0x319C , the method will still return true . This, in turn, will entail the loss of time to search for errors.

Conclusion


RunUO was created a long time ago, a lot of work has been done, but with the example of this project you can see the benefits of using static analysis on projects with a history. The analyzer for 543 thousand lines of project code issued about 500 warnings (not counting the Low level ), most of which were not included in the article because of their uniformity. To get acquainted with the result of the analysis in more detail, I suggest using a free license for open source projects .



If you want to share this article with an English-speaking audience, then please use the link to the translation: Ekaterina Nikiforova. RunUO Check by the PVS-Studio Analyzer .

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


All Articles