Checking the source code for a set of C # /. NET components from Sony


    As some of you remember, we recently released a version of the analyzer that supports C # code verification . With the advent of the possibility of analyzing verification of projects written in C #, a new scope for creativity opens up. An analysis of one of these projects developed by Sony Computer Entertainment (SCEI) will be discussed in this article.


    What was checked?


    Sony Computer Entertainment is a video game company, a division of Sony Corporation that specializes in video games and game consoles. This company develops video games, hardware and software for PlayStation consoles.



    The Authoring Tools Framework (ATF) is a set of C # /. NET components for developing tools for Windows. ATF is used by most of SCEI's leading video game development studios for custom tools. This set of components is used by such studios as Naughty Dog, Guerrilla Games, Quantic Dream. In particular, tools developed using these software components were used to create such famous games as The Last of Us and Killzone. ATF is an open source project that is available in the repository on GitHub .

    Analysis tool


    To analyze the source code, the PVS-Studio static analyzer was used . This tool can check source code written in C / C ++ / C #. Each diagnostic message is described in detail in the documentation, which gives examples of incorrect code and ways to fix it. Many descriptions of diagnostics have a link to the corresponding section of the error database , which contains information about what errors in real projects could be detected using certain diagnostics.



    You can download and try the analyzer on your (or someone else's) project using the link .

    Error Examples


    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 can see from the code, regardless of the truth of the condition 'e.Effect! = DragDropEffects.None', the same method will be called with the same arguments. Not being a programmer who wrote this code, it is sometimes difficult to say how to fix this place correctly, but I think no one doubts that something needs to be fixed. What exactly is up to the author of the code to decide.

    Consider the following code snippet:
    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 understood that when the method is called, the properties will be assigned the values ​​passed as arguments, about this the names of the properties and parameters differ only in the first letter case. As a result, the 'ProgressError' property is written to itself instead of getting the value of the 'progressError' parameter.

    Interestingly, this is far from an isolated case of confusion between uppercase and lowercase letters. In some proven projects, errors of exactly the same type were encountered. There is a suspicion that soon we will reveal a new typical error pattern inherent in programs written in C #. There is a tendency to initialize properties in a method whose parameter names differ from the names of initialized properties only in the first letter case. As a result, errors like this one appear. In the following code example, if it is not erroneous, it looks at least strange:
    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 Warnings:
    • V3005 The 'window. Left' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 706
    • V3005 The 'window.Top' variable is assigned to itself. Atf.Gui.Wpf.vs2010 DockPanel.cs 707

    From the analyzer’s code and warnings, it can be seen that the 'Left' and 'Top' properties of the 'window' object are written to themselves. For some cases, this option is acceptable, for example, when special logic is 'hung' on the access method of a property. However, there is no additional logic for these properties, so the reasons for writing such code remain unclear.

    The following 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

    This type of error has also been found many times in proven C # projects. As a result of casting the object to a compatible type using the 'as' operator, a new object is obtained, however, in the code, the original object is checked for equality of 'null'. This code can work quite correctly if it is known for certain that the object 'd' will always be compatible with the type 'PasswordBox'. But if this is not the case (now, or in the future, something will change in the program), then you can easily get a 'NullReferenceException' on code that used to work fine.

    In the following example, the programmer, on the contrary, tried to secure himself in all available ways, only it is a little incomprehensible why:
    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. If it’s not clear, let's figure out why.

    This is an implementation of a property of type 'Rect', therefore, 'value' is also of type 'Rect'. 'Top', 'Left', 'Width', 'Height' - properties of this type, having the type 'double'. This code checks to see if the values ​​of these properties are outside the range of values ​​that the type 'double' can take. Moreover, for comparison, 'magic numbers' are used, and not the constants defined in the 'double' type. Since values ​​of type 'double' are always in this range, this condition will always be false.

    Apparently the programmer wanted to protect his program from a non-standard implementation of type 'double' in the compiler. Nevertheless, it looks strange, so the analyzer quite reasonably issued a warning, asking the programmer to double-check the code.

    Move 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 for the execution of the 'while' loop is redundant, it could be simplified by removing the second subexpression. Then the cycle could be simplified to the following form:
    
    while (res.Operation.Status != DispatcherOperationStatus.Completed)
      Thread.Sleep(50);

    The following interesting example:
    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 Warnings:
    • V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double) (X) / Y ;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 216
    • V3041 The expression was implicitly cast from 'int' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double) (X) / Y ;. Atf.Gui.OpenGL.vs2010 ArcBallCameraController.cs 217

    This is again one of those cases when it is very difficult for a third-party developer to say for sure whether the code contains an error or not. On the one hand, performing integer division followed by implicit casting to a real type looks strange. On the other hand, sometimes they do it deliberately, neglecting the lost accuracy.

    What was meant here is hard to say. Perhaps they didn’t want to lose accuracy at all, but as a result of the operation 'm_width / 2' this loss will nevertheless occur. Then the code would need to be fixed to the following:
    float x = point.X / ((float)m_width / 2); 

    On the other hand, they probably wanted to write an integer in 'x', since further comparisons are performed with integer values. In this case, it was not necessary to explicitly cast the code to the type 'float':
    float x = point.X / (m_width / 2);

    Our analyzer grows and develops, respectively, updated with new interesting diagnostics. The following error was just found by one of these new diagnostics. Since this diagnosis is not yet included in the release at the time of writing, I cannot provide a link to the documentation. I hope everything is clear here:
    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

    The code shows that the sum of several products is calculated and the result of this operation is written to the variable 'dot', after which, if the value of 'dot' is negative, the inversion of all the values ​​involved in the operation is performed. More precisely, inversion was implied, judging by the formatting of the code. In fact, in this case only the property 'X' of the object 'q1' will be inverted, and all other properties will be inverted regardless of the value of the variable 'dot'. The solution is braces:
    if (dot < 0)
    {
      q1.X = -q1.X; 
      q1.Y = -q1.Y; 
      q1.Z = -q1.Z; 
      q1.W = -q1.W;
    }

    Move 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 Warnings:
    • V3008 The 'Y' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 221, 217. Atf.Core.vs2010 QuatF.cs 221
    • V3008 The 'Z' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 222, 218. Atf.Core.vs2010 QuatF.cs 222

    I specifically provided an additional piece of code to make the error more visual. 'Y' and 'Z' are instance fields. Depending on some conditions, these or those values ​​are written in these fields, after which the method stops. But in the body of the last 'if' statement they forgot to write the 'return' statement, because of which the fields will not be assigned the values ​​that were implied. Then the correct code might 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; 

    This, perhaps, dwell. These places seemed to be the most interesting, so I decided to write them out and make them out. Other errors were found, in particular - I did not consider warnings with a low level of importance at all, from the warnings of medium and high levels I wrote out only a few.

    Conclusion




    As you can see, no one is safe from errors, and by inattentiveness it is easy to assign an object to yourself or skip some operator. In a large amount of code, such errors are often difficult to detect visually, and not all of them manifest themselves immediately - some of them will shoot your foot a couple of years later. To prevent this, a static analyzer is needed to detect errors at an early stage, reduce the cost of developing a project, protect your nerves and keep your legs intact.


    If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Sony C # /. NET component set analysis .

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

    Also popular now: