Sony C#/.NET component set analysis

Some of you may know that we have recently released version 6.00 of our analyzer, that now has C# support. The ability to scan C# projects increases the number of open-source projects we can analyze. This article is about one such check. This time it is a project, developed by Sony Computer Entertainment (SCEI).

What have we checked?

Sony Computer Entertainmentis a video game company. Being a branch of Sony Corporation, it specializes in video games and game consoles. This company develops video games, hardware and software for PlayStation consoles.

Authoring Tools Framework (ATF) is a set of C#/.NET components for making tools on Windows®. ATF has been used by most Sony Computer Entertainment first party game studios to make custom tools. This component set is used by such studios as Naughty Dog, Guerrilla Games and Quantic Dream. Tools developed with these program components were used during the creation of such well know games as 'The Last of Us' and 'Killzone'. ATF is an open-source project which is available at GitHub repository.

Analysis tool

To perform the source code analysis, we used PVS-Studio static code analyzer. This tool scans projects written in C/C++/C#. Every diagnostic message has a detailed description in the documentation with examples of incorrect code and possible ways to fix the bugs. Quite a number of the diagnostic descriptions have a link to corresponding sections of error base, where you can see information about the bugs that were found in real projects with the help of these diagnostics.

You can download the analyzer here and run it on your (or somebody's) code.

Examples of errors

public static void DefaultGiveFeedback(IComDataObject data, 
                                       GiveFeedbackEventArgs e)
{
  ....
  if (setDefaultDropDesc && (DropImageType)e.Effect != currentType)
  {
    if (e.Effect != DragDropEffects.None)
    {
      SetDropDescription(data, 
        (DropImageType)e.Effect, e.Effect.ToString(), null);
    }
    else
    {
      SetDropDescription(data, 
        (DropImageType)e.Effect, e.Effect.ToString(), null);
    }
    ....
  }
}

Analyzer warning: V3004 The 'then' statement is equivalent to the 'else' statement. Atf.Gui.WinForms.vs2010 DropDescriptionHelper.cs 199

As you see in the code, the same method with similar arguments will be called, in spite of the 'e.Effect != DragDropEffects.None' being true or not. It's hard to suggest any ways to fix this code fragment, without being a developer of this code, but I think it is clear that this fragment needs a more thorough revision. What exactly should be fixed, is a question which should be directed to the author of this code.

Let's look at the following code fragment:

public ProgressCompleteEventArgs(Exception progressError, 
            object progressResult, 
            bool cancelled)
{
  ProgressError = ProgressError;
  ProgressResult = progressResult;
  Cancelled = cancelled;
}

Analyzer warning: V3005 The 'ProgressError' variable is assigned to itself. Atf.Gui.Wpf.vs2010 StatusService.cs 24

It was supposed that during the method call, the properties would get values, passed as arguments; at the same time the names of properties and parameters differ only in the first letter case. As a result - 'ProgressError' property is assigned to itself instead of being given the 'progressError' parameter.

Quite interesting here, is the fact that it's not the only case where capital and small letters get confused. Several of the projects we've checked have the same issues. We suspect that we'll find a new error pattern typical for C# programs soon. There is a tendency to initialize properties in a method, where the names of parameters differ from the names of the initialized properties by just one letter. As a result, we have errors like this. The next code fragment is probably not erroneous, but it looks rather strange, to say the least.

public double Left { get; set; }
public double Top  { get; set; }

public void ApplyLayout(XmlReader reader)
{
  ....
  FloatingWindow window = new FloatingWindow(
                                this, reader.ReadSubtree());
  ....
  window.Left = window.Left;
  window.Top = window.Top;
  ....
}

Analyzer warning:

In the analyzer warnings you can see that the 'window' object properties 'Left' and 'Top' are assigned to themselves. In some cases this variant is perfectly appropriate, for example when the property access method has special logic. But there is no additional logic for these properties, and so it is unclear why the code is written this way.

Next example:

private static void OnBoundPasswordChanged(DependencyObject d,
                      DependencyPropertyChangedEventArgs e)
{
    PasswordBox box = d as PasswordBox;

    if (d == null || !GetBindPassword(d))
    {
        return;
    }

    // avoid recursive updating by ignoring the box's changed event
    box.PasswordChanged -= HandlePasswordChanged;
    ....
}

Analyzer warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'd', 'box'. Atf.Gui.Wpf.vs2010 PasswordBoxBehavior.cs 38

We have already seen quite a number of errors of this type in the C# projects that we checked. By casting an object to a compatible type using 'as operator' the programmer gets a new object, but further in the code the source object is compared to 'null'. This code can work correctly, if you are sure that the 'd' object will always be compatible with the 'PasswordBox' type. But it is not so (for now or if there are more changes in the program); you can easily get 'NullReferenceException' in code that used to work correctly. So in any case this code needs to be reviewed.

In the following example, conversely, the programmer clearly tried to make the code as secure as possible, although it's not quite clear what for.

public Rect Extent
{
    get { return _extent; }
    set
    {
        if (value.Top    < -1.7976931348623157E+308  || 
            value.Top    >  1.7976931348623157E+308  || 
            value.Left   < -1.7976931348623157E+308  ||
            value.Left   >  1.7976931348623157E+308  || 
            value.Width  >  1.7976931348623157E+308  || 
            value.Height >  1.7976931348623157E+308)
        {
            throw new ArgumentOutOfRangeException("value");
        }
        _extent = value;
        ReIndex();
    }
}

Analyzer warning: V3022 Expression is always false. Atf.Gui.Wpf.vs2010 PriorityQuadTree.cs 575

This condition will always be false. Let's look into the code and see why.

This is an implementation of the property that has 'Rect' type, hence 'value' also has 'Rect' type. 'Top', 'Left', 'Width', 'Height' are properties of this type, that have 'double' type. This code checks if these property values exceed the range of values, that the 'double' type takes. We also see that 'magic numbers' are used here for comparison, not constants, defined in the 'double' type. That's why this condition will always be false, since the 'double' type values are always within the value range.

Apparently, the programmer wanted to secure the program from a non-standard implementation of 'double' type in the compiler. Nevertheless, it looks rather strange, so it was reasonable for the analyzer to issue a warning, suggesting that the programmer double-check the code.

Let's go on.

public DispatcherOperationStatus Status { get; }
public enum DispatcherOperationStatus
{
  Pending,
  Aborted,
  Completed,
  Executing
}
public object EndInvoke(IAsyncResult result)
{
  DispatcherAsyncResultAdapter res = 
    result as DispatcherAsyncResultAdapter;
  if (res == null)
    throw new InvalidCastException();

  while (res.Operation.Status != DispatcherOperationStatus.Completed
         || res.Operation.Status == DispatcherOperationStatus.Aborted)
  {
    Thread.Sleep(50);
  }

  return res.Operation.Result;
}

Analyzer warning: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. Atf.Gui.Wpf.vs2010 SynchronizeInvoke.cs 74

The condition of the 'while' loop is redundant, it could be simplified by removing the second subexpression. Then the loop can be simplified in the following way:

while (res.Operation.Status != DispatcherOperationStatus.Completed)
  Thread.Sleep(50);

Next example, quite an interesring one:

private Vec3F ProjectToArcball(Point point)
{
  float x = (float)point.X / (m_width / 2);    // Scale so bounds map
                                               // to [0,0] - [2,2]
  float y = (float)point.Y / (m_height / 2);

  x = x - 1;                           // Translate 0,0 to the center
  y = 1 - y;                           // Flip so +Y is up
  if (x < -1)
    x = -1;
  else if (x > 1)
    x = 1;
  if (y < -1)
    y = -1;
  else if (y > 1)
    y = 1;
  ....
}

Analyzer warning:

This is one of those cases, where it's very hard for a third-party developer to say for sure if there is an error in this code or not. On the one hand - integer division with implicit casting to a real type looks strange. On the other hand, sometimes it can be done deliberately, regardless of the precision loss.

It's difficult to say what was meant here. Perhaps the programmer didn't want to lose the precision of the code, but it will still occur as a result of 'm_width / 2' operation. In this case we should rewrite the code in the following way:

float x = point.X / ((float)m_width / 2); 

On the other hand, there is a possibility that an integer number was meant to be written to 'x', as further on we see operations of comparison with integer values. But in this case, there was no need to do explicit casting to 'float' type.

float x = point.X / (m_width / 2);

Our analyzer continues developing and obtaining new diagnostics. The next error was found with the help of our new diagnostic. But as this diagnostic wasn't in the release version of the analyzer, there will be no link to the documentation, but I hope the idea is clear:

public static QuatF Slerp(QuatF q1, QuatF q2, float t)
{
  double dot = q2.X * q1.X + q2.Y * q1.Y + q2.Z * q1.Z + q2.W * q1.W;

  if (dot < 0)
    q1.X = -q1.X; q1.Y = -q1.Y; q1.Z = -q1.Z; q1.W = -q1.W;

  ....
}

Analyzer 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. Atf.Core.vs2010 QuatF.cs 282

You can see that a sum of several products is evaluated and the result is written to the 'dot' variable. After that if the 'dot' value is negative, there is inversion of all values of this operation. More precisely, the inversion was meant to be here, judging by the code formatting. In reality, only 'X' property of 'q1' will be inverted, all other properties will be inverted regardless of the value of 'dot' variable. Solution of this problem is curly brackets:

if (dot < 0)
{
  q1.X = -q1.X; 
  q1.Y = -q1.Y; 
  q1.Z = -q1.Z; 
  q1.W = -q1.W;
}

Let's go on.

public float X;
public float Y;

public float Z;
public void Set(Matrix4F m)
{
  ....
  ww = -0.5 * (m.M22 + m.M33);
  if (ww >= 0)
  {
    if (ww >= EPS2)
    {
      double wwSqrt = Math.Sqrt(ww);
      X = (float)wwSqrt;
      ww = 0.5 / wwSqrt;
      Y = (float)(m.M21 * ww);
      Z = (float)(m.M31 * ww);
      return;
    }
  }
  else
  {
    X = 0;
    Y = 0;
    Z = 1;
    return;
  }

  X = 0;
  ww = 0.5 * (1.0f - m.M33);
  if (ww >= EPS2)
  {
    double wwSqrt = Math.Sqrt(ww);
    Y = (float)wwSqrt;                   //<=
    Z = (float)(m.M32 / (2.0 * wwSqrt)); //<=
  }

  Y = 0; //<=
  Z = 1; //<=
}

Analyzer warning:

We have intentionally provided an additional code fragment so that the error is more evident. 'Y' and 'Z' are instance fields. Depending on the conditions, some values are written to these fields and then method execution is terminated. But in the body of the last 'if' operator, the programmer forgot to write the 'return' operator, so the fields will be assigned not those values, as it was supposed. This being the case, the correct code could look like this:

X = 0;
ww = 0.5 * (1.0f - m.M33);
if (ww >= EPS2)
{
  double wwSqrt = Math.Sqrt(ww);
  Y = (float)wwSqrt;                   
  Z = (float)(m.M32 / (2.0 * wwSqrt)); 
  return;
}

Y = 0; 
Z = 1; 

Perhaps this is enough. These fragments seemed the most interesting to us, that's why we've brought them here. There were more bugs found, but we haven't provided low severity level examples here, instead opting to show the mid to high severity level examples.

Conclusion:

As you see, no one is immune to failure, it's quite easy to assign an object to itself, or miss some operator due to carelessness. At times, such errors are hard to detect visually in big projects, moreover most of them will not immediately show up - some of them will shoot you in the foot half a year later. To avoid such misfortune, it's a good idea to use an analyzer that is able to detect bugs during the early phases of development, decreasing the development costs, keeping you sane, and your legs safe.