Dusting the globe: analysis of NASA World Wind project




Sometimes it is useful to look back to see how helpful the analyzer was to old projects, and which errors can be avoided in good time, if the analyzer is regularly used. This time our choice was NASA World Wind project, which was being developed on C# until 2007.

Picture 3

Introduction

NASA World Wind is an interactive globe that allows the viewing of any place on Earth. This project uses the public photo base from the Landsat satellite and the relief modeling project Shuttle Radar Topography Mission. The first versions of the project were created in C#. Later the project continued its development in Java. The last C# version was 1.4. Although the C# version is long abandoned, this doesn't prevent us from testing the project, and evaluating the code quality from the NASA Ames Research Center developers.

Why have we tested an old project? Quite a long time ago we were asked to check a NASA project and finally, we stumbled upon this project. No, this check does not bring any benefit to the project. But we didn't set this goal this time. We just wanted to show the benefits PVS-Studio static code analyzer can bring to the development process, and to the company; NASA in this case.

By the way, this is not the first case of checking "historical" projects. Perhaps you may be interested in reading the following articles:

NASA World Wind Project demonstrates quite well the abilities of the PVS-Studio analyzer. You will see in the article that the developers seem to use a lot of Copy-Paste mechanisms. Many errors multiplied, and are often duplicated in the code. Some errors are quite illustrative in showing the principles of analyzer diagnostics work.

To do the analysis we used PVS-Studio analyzer version 6.06.

Error density

After the check, the analyzer issued 120 first level warnings, and 158 second level warnings. After examining the messages, I think that 122 fragments need revision and fixing. Thus, the percentage of false positives was 56%. This means that every second message indicates an error or really bad code which needs correction.

Now, let's evaluate the error density. On the whole, there are 474,240 lines of code (taking the comments into account) in 943 files. Among them we can find 122 troublesome fragments. As a result, we see that the analyzer finds 0.25 errors per 1000 lines of code. This shows high code quality.

Last line effect

public Point3d (Point3d P) 
{
   X = P.X;
   Y = P.Y;
   X = P.Z;  // <=
}

Warning:

  • V3008 The 'X' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 40, 38. Point3d.cs 40
Picture 1

Here we see a classic error in the copy constructor. During the assignment of three-dimensional object coordinates, instead of setting the variable Z, the value of X variable was rewritten twice. It is obvious that this error occurred as a result of the use of the "Copy-Paste method". The chance of making a mistake in the last line, if you copy the code, is much higher. Read more about this phenomenon in the article by Andrey Karpov "Last line effect". To fix this constructor, we need to change the variable in the last line.

public Point3d (Point3d P)
{
   X = P.X;
   Y = P.Y;
   Z = P.Z;  
}

This is not the only error in this project that proves this effect. There will be several more examples, proving it.

Several more suspicious fragments, detected by the V3008 diagnostic:

  • V3008 The 'this._imagePath' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 270, 263. ImageLayer.cs 270
  • V3008 The 'm_PolygonFill' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1623, 1618. ShapeFileLayer.cs 1623

A logic error or insidious typo?

private static void WebUpdate(....)
{
  ....
  if (ver != version)          // <=
   {
      ....
   }
   else if (ver != version)    // <=
   {
      ....
   }
}

Warning:

  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 2111, 2197. KMLImporter.cs 2111

The fragment performs automatic updates of the plugin in your project. When there is a mismatch between the versions, it downloads the appropriate plugin. If this is an internal plugin, the program refuses to update it automatically, and just issues a message about the new version. But the conditional operator, following the else statement, contains an expression that contradicts the condition of the else statement. This code is rather controversial, and only the developers of it, who know how the function should work, can say for sure where the error is hidden here. At this point I can only assume that else should belong to a completely different statement. Or, judging by the commentary next to the conditional operator, we can draw the conclusion that the else statement isn't in the right place, and the second if statement should have a different condition.

Copying error

public GpsSetup(....)
{
 ....
  if (m_gpsIcon!=null)
  {
   ....
   labelTitle.Text = "Set options for " +
                     m_gpsIcon.m_RenderInfo.sDescription;
  }
  else
  if (m_gpsTrackLine != null)
  {
   ....
   labelTitle.Text = "Set options for " + 
                     m_gpsIcon.m_RenderInfo.sDescription; // <=
  }
 ....
}

Warning:

  • V3080 Possible null dereference. Consider inspecting 'm_gpsIcon'. GpsTrackerPlugin.SourceSetup.cs 68

In the fragment given above there is an error that resulted from incorrect code copying. In the last conditional statement the variable was confused. As a result, there was an error with the access by null reference. According to the previous checks, the variable m_gpsIcon, used in the last string, will surely be null. If the condition m_gpsTrackLine!=null is true, the program will terminate. I can suggest that the correct code should look as follows:

if (m_gpsTrackLine != null)
{
  ....
  labelTitle.Text = "Set options for " + 
                     m_gpsTrackLine.m_sDescription;
}

Unexecutable condition

public bool LoadSettings(....)
{
 ....
 if (bSet)
 {
  while(true)
   {
     line = sr.ReadLine();
     if (line==null || line.StartsWith("END UI CONTROLS"))
        break;
     ....
     if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
        ....
     else
     ....
     if (line.StartsWith("checkBoxNoDelay="))            // <= 
        ....
     else
     if (line.StartsWith("checkBoxNoDelay="))            // <=
        ....
     ....
     else
     if (line.StartsWith("comboBoxAPRSInternetServer=")) // <=
     ....
   }
  ....
 }
 ....
}

Warnings:

  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4503, 4607. GPSTracker.cs 4503
  • V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 4527, 4530. GPSTracker.cs 4527

Another example of an error which occurs because of code fragments that are created by copying the code. This function is a very long code block, consisting of identical conditional operators, which is meant for the handling of incoming commands. The code will never get to the second check. There is nothing wrong about this error, but some other command should probably have been used instead of a duplicate. Thus, if the necessary command isn't processed, the code will not behave in the way the programmer expects it to.

The analyzer pointed to two fragments in this huge tree of conditional statements. In the first fragment the double check line.StartsWith("checkBoxNoDelay=") is located right near, and it could be noticed with careful examination of the code, although, having such an amount of code, it is a very complex process and would take a lot of time.

The second place is hidden from the eyes of developers much better. The first check line.StartsWith("comboBoxAPRSInternetServer=") is located in the middle of the tree, and the second check actually finishes it, but there are about 100 code lines between them. This case demonstrates quite well, that sometimes static analysis can be much more efficient than code review. Regular usage of the analyzer allows the detection of errors at early stages, and avoidance of painful debugging and reading of long functions.

An error in the variable name

public int CompareTo(object obj)
{
  RenderableObject robj = obj as RenderableObject;
  if(obj == null)   // <=
    return 1;
  return this.m_renderPriority.CompareTo(robj.RenderPriority);
}

Warning:

  • V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'robj'. RenderableObject.cs 199

A typo in variable name led to the potential use of a null reference. Instead of checking an object of the derived class robj, the base object obj was checked. If it does not match the type RenderableObject, the program terminates. To fix it we need to change the variable name to robj in the expression of a conditional statement.

Access by null reference

public override void Render(DrawArgs drawArgs)
{
  ....
  if(this.linePoints.Length > 1)     // <=  
   {
     ....
     if(this.linePoints != null)     // <=
      {
        ....
      }
   }
  ....
}

Warnings:

  • V3022 Expression 'this.linePoints != null' is always true. PathLine.cs 346
  • V3095 The 'this.linePoints' object was used before it was verified against null. Check lines: 332, 346. PathLine.cs 332

Two different diagnostics pointed to this code. We see that the precedence of actions for checks and access by reference is violated. First, the code evaluates the number of objects inside an object, and then it checks if the object exists at all. Perhaps the object this.linePoints may never get the null value, but then the check in the inner condition isn't needed either. It is logical to change the code as follows:

if(this.linePoints != null && this.linePoints.Length > 1)  
{
  ....
}

A condition that is always false

private bool checkSurfaceImageChange()
{
  ....
  SurfaceImage currentSurfaceImage = 
  m_ParentWorldSurfaceRenderer.SurfaceImages[i] as SurfaceImage;
                    
  if(currentSurfaceImage.LastUpdate > m_LastUpdate || 
      currentSurfaceImage.Opacity != 
      currentSurfaceImage.ParentRenderable.Opacity)
     {
      if(currentSurfaceImage == null ||               // <=
         currentSurfaceImage.ImageTexture == null || 
         currentSurfaceImage.ImageTexture.Disposed || 
         !currentSurfaceImage.Enabled || ....)
         {
           continue;
         }
         else
         {
           return true;
         }
      }
  ....
}

Warning:

  • V3063 A part of conditional expression is always false: currentSurfaceImage == null. SurfaceTile.cs 1069

This error is similar to the one described in the previous section. Here, the reference to the object is assigned to the variable just before the conditional operators. Most likely, the reference to the object cannot be null, and a check in the internal condition is not necessary. If it's not so, the check should be moved to the outer conditional statement. There we already see that the properties of the object currentSurfaceImage are already processed. If the reference is a null one, the bug will appear before the check is done.

Similar fragments:

  • V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.APRS.cs 87
  • V3063 A part of conditional expression is always true: iWildIndex==-1. GPSTrackerPlugin.NMEA.cs 169
  • V3063 A part of conditional expression is always false: newvalue == null. SchemaTypes.cs 860

Wandering parenthesis

public void threadStartFile()
{
 ....
  if (File.Exists(sFileName))
   {
    ....
    if (gpsSource.bTrackAtOnce!=true && ....)
    {
      if (!gpsSource.bWaypoints)
      {
         m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
         ....                    
                            // <=
      gpsSource.sFileNameSession=sFileName;
      ....
      }
      else
      {
      ....
        }
      }
      else
      {
       ....                             
      }
   }                        // <=
 ....
}

Warning:

  • V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. GPSTrackerPlugin.File.cs 314

The analyzer detected a mismatch between the formatting and the code logic. Doing a more thorough examination, we realized that in the inner if, the programmer forgot a closing bracket. The missing bracket was found after the second else statement block. As a result, the project was compiled, in spite of this sloppy code. The compiler may report only if else ceases to belong to the conditional statement. In this case there was a shift of else to a different if statement. As the closing bracket was in the wrong place, the work of two conditional statements was disrupted along with the formatting of else statements. I can suppose the way the code should be written after the correction of the bracket placement and formatting errors.

public void threadStartFile()
{
 ....
 if (File.Exists(sFileName))
  {
   ....
   if (gpsSource.bTrackAtOnce!=true && ....)
   {
     if (!gpsSource.bWaypoints)
     {
        m_GpsTracker.m_gpsTrackerPlugin.pluginShowFixInfo("");
        ....                    
     }
     gpsSource.sFileNameSession=sFileName;
     ....
   }
   else
   {
     ....
   }
 }
 else
 {
   ....                             
 }
 ....
}

Unused parameter

public bool Diagonal(CPoint2D vertex1, CPoint2D vertex2)
{
 ....
 for (int i= 0; i<nNumOfVertices; i++) 
  {
    ....
    //Diagonal line:
    double x1=vertex1.X;
    double y1=vertex1.Y;
    double x2=vertex1.X;
    double y2=vertex1.Y;
    ....
  }
 ....
}

Warning:

  • V3065 Parameter 'vertex2' is not utilized inside method's body. CPolygon.cs 227

The function receives the coordinates of two points of the line. But as a result of typos, the values of both points are taken only from the variable vertex1. To fix this, we need to change the code as follows:

double x2=vertex2.X;
double y2=vertex2.Y;

Besides the typo in the assignment, there was one more oddity. Why should we assign a fixed value in every loop iteration? The value doesn't change inside the loop, and it's much more logical to do the assignment once before its beginning.

Overwritten function parameter

void ShowInfo(.... , float fDistance )
{
  ....
  if (m_fTotalDistance>=0F)
   {
     string sUnit=(m_fTotalDistance>=1F)?"km":"m";
     fDistance = (m_fTotalDistance < 1F) ? 
                 (m_fTotalDistance * 1000F) : 
                  m_fTotalDistance;
     sInfo += "Track Distance: " +
              Convert.ToString(
               decimal.Round(
                Convert.ToDecimal(fDistance),3)) +
              sUnit + 
              "\n";
   }
  ....
}

Warning:

  • V3061 Parameter 'fDistance' is always rewritten in method body before being used. GPSTrackerPlugin.WorldWind.cs 1667

The fDistance parameter, coming to the function, gets rewritten right before it is used. The real value of the variable gets lost, and instead of it we have the property value of m_fTotalDistance class used. There is no point in changing the value fDistance, because this variable isn't used anywhere else in the function. Judging by other function fragments, we can suppose that the variables are swapped and the fragment should be written like this:

if (fDistance>=0F)
{
 string sUnit=(fDistance>=1F)?"km":"m";
 m_fTotalDistance = (fDistance < 1F) ? 
                    (fDistance * 1000F) : fDistance;
 sInfo += "Track Distance: " + 
          Convert.ToString(
           decimal.Round(
            Convert.ToDecimal(m_fTotalDistance),3)) +
          sUnit + 
          "\n";
}

Incorrect check of NaN

public override bool PerformSelectionAction(DrawArgs drawArgs)
{
  ....
  if(icon.OnClickZoomAltitude != double.NaN || 
     icon.OnClickZoomHeading != double.NaN || 
     icon.OnClickZoomTilt != double.NaN)
     {
       ....
     }
  ....
}

Warning:

  • V3076 Comparison of 'icon.OnClickZoomAltitude' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389

According to the documentation, we cannot compare two values of double.NaN by means of != operator. The result of this comparison will always be true. We should use double.IsNaN() method to do a correct check. So, the code will be as follows:

if(!double.IsNaN(icon.OnClickZoomAltitude) || 
   !double.IsNaN(icon.OnClickZoomHeading) || 
   !double.IsNaN(icon.OnClickZoomTilt)) ....

The analyzer has detected many places where such incorrect checks were used:

  • V3076 Comparison of 'icon.OnClickZoomHeading' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
  • V3076 Comparison of 'icon.OnClickZoomTilt' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. Icon.cs 389
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 642
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 645
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 650
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 677
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 681
  • V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 886
  • V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 894
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1038
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1041
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1046
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1073
  • V3076 Comparison of 'm_ShapeTileArgs.ScaleMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1077
  • V3076 Comparison of 'm_ScalarFilterMin' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1259
  • V3076 Comparison of 'm_ScalarFilterMax' with 'double.NaN' is meaningless. Use 'double.IsNaN()' method instead. ShapeFileLayer.cs 1267

Ignoring the function result

private static void addExtendedInformation(....)
{
 ....
 if(toolBarImage.Length > 0 && 
    !Path.IsPathRooted(toolBarImage))
      Path.Combine(...., toolBarImage);    // <=
 ....
}

Warning:

  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 943

The call of the Path.Combine function without handling the result makes no sense. In this case the function serves for forming the path to the object based on the absolute path to the executable file and relative path to the image. The absence of value handling denotes a real error. The Path.Combine function is used in many places within the program. Thus, we can assume that the code should be fixed in the following way:

toolBarImage=Path.Combine(...., toolBarImage);

The error was copied and got in other places of the project:

  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1361
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1566
  • V3010 The return value of function 'Combine' is required to be utilized. ConfigurationLoader.cs 1687
  • V3010 The return value of function 'Replace' is required to be utilized. WorldWind.cs 2455

Missing Enum value

public enum MenuAnchor
{
  Top,
  Bottom,
  Left,
  Right
}

public bool OnMouseMove(MouseEventArgs e)
{
  ....
  if(this._visibleState == VisibleState.Visible)
   {
     ....
     switch(m_anchor)
     {
       case MenuAnchor.Top: ....
       case MenuAnchor.Bottom: ....    
       case MenuAnchor.Right: ....
     }
   }
  ....
}

Warning:

  • V3002 The switch statement does not cover all values of the 'MenuAnchor' enum: Left. Menu.cs 1681

Apparently, the code checks if the cursor is placed above the ToolBar at this moment or not. We can see in the code that the handling of the MenuAnchor.Left element is missing. It's impossible to understand what the project developers meant by writing such code. Perhaps, its handling wasn't necessary at all, but to my mind - this fragment is worth reconsidering.

Meaningless assignment

protected virtual void CreateElevatedMesh()
{
  ....
  if (minimumElevation > maximumElevation)
  {
    // Compensate for negative vertical exaggeration
    minimumElevation = maximumElevation;
    maximumElevation = minimumElevation;
  }
  ....
}

Warning:

  • V3037 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 625, 624. QuadTile.cs 625

The code is just redundant here. The presence of the string maximumElevation = minimumElevation doesn't make sense, because at the time of assignment, both variables have the same value in the result of the previous operation. Of course, we can assume that the developers wanted to change the values of the variables, but they did so incorrectly. This is a questionable assumption because both the comment and the fact that the variable maximumElevation is no longer used, prove the opposite.

Repeated assignment

public static bool SearchForAddress(....)
{
  double num1;
  long2 = num1 = 0;
  long1 = num1 = num1;  // <=
  lat2 = num1 = num1;   // <=
  lat1 = num1;
  ....
}

Warnings:

  • V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2011
  • V3005 The 'num1' variable is assigned to itself. PlaceFinder.cs 2012

Again, the effect of Copy-Paste, which is often seen in the project. This fragment isn't dangerous at all, but the assignment of the same variable three times looks a bit strange. In my opinion, it would be easier to initialize the variable num1 directly during its declaration. Then we can assign the num1 value separately to each variable.

Unsafe call of the event handler

private static void m_timer_Elapsed(....)
{
  ....
  if (Elapsed != null)
     Elapsed(sender, e);
}

Warning:

  • V3083 Unsafe invocation of event 'Elapsed', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TimeKeeper.cs 78

Such an event call in a multithreaded application isn't safe. It may easily happen so, that between the verification against null and the handler call there will be an unsubscription from the event in another thread. If this is the only handler, it will lead to the usage of a null reference. It is recommended to use a temporary variable to do a safe call. Then the event will be called correctly in any case. You may find other ways to correct this error on the diagnostic page V3038.

By the way this is a very treacherous type of error, because the problems will occur quite rarely, but it's almost impossible to reproduce them.

I'll show other unsafe calls as a list.

  • V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 65
  • V3083 Unsafe invocation of event 'Close', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. InternalWebBrowser.cs 73
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 672
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 691
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WavingFlagLayer.cs 1105
  • V3083 Unsafe invocation of event, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. TreeNodeWidget.cs 325
  • V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 721
  • V3083 Unsafe invocation of event 'OnResizeEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. FormWidget.cs 1656
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 351
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 362
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PictureBox.cs 590
  • V3083 Unsafe invocation of event 'OnMouseEnterEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 282
  • V3083 Unsafe invocation of event 'OnMouseLeaveEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 293
  • V3083 Unsafe invocation of event 'OnMouseUpEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWind.Widgets.PictureBox.cs 511
  • V3083 Unsafe invocation of event 'OnVisibleChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. WorldWindow.Widgets.Form.cs 184
  • V3083 Unsafe invocation of event 'StatusChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ScriptPlayer.cs 57
  • V3083 Unsafe invocation of event 'Navigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 608
  • V3083 Unsafe invocation of event 'ReadyStateChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 578
  • V3083 Unsafe invocation of event 'UpdateUI', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 568
  • V3083 Unsafe invocation of event 'HtmlKeyPress', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 587
  • V3083 Unsafe invocation of event 'HtmlEvent', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 600
  • V3083 Unsafe invocation of event 'BeforeNavigate', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 635
  • V3083 Unsafe invocation of event 'BeforeShortcut', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 626
  • V3083 Unsafe invocation of event 'BeforePaste', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 644
  • V3083 Unsafe invocation of event 'ContentChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. HtmlEditor.cs 615

Unsafe object for locking

private int APRSParse(....)
{
  int iRet=-1;
  try
  {
   lock("ExternDllAccess")
     {
       //Call the APRS DLL
       iRet=APRSParseLinePosStat(sString, 
                                 ref aprsPosition, 
                                 ref aprsStatus);
     }
  }
  catch(Exception)
  {
    iRet=-1;
  }
 return iRet;
}

Warning:

  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.APRS.cs 256

Using a text string as an object for blocking isn't a safe thing to do. You may get access to such objects from any place of the program. As a result you may have a deadlock, because in both cases we'll get the reference to the same object in the memory during the string analysis.

Here are several more spots, detected by the analyzer:

  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 226
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 244
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 370
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 416
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 448
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.File.cs 723
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 339
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 394
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 468
  • V3090 Unsafe locking on an object of type 'String'. GPSTrackerPlugin.WorldWind.cs 538
  • V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 3853
  • V3090 Unsafe locking on an object of type 'String'. GPSTracker.cs 6787
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 73
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Globals.cs 222
  • V3090 Unsafe locking on a type. All instances of a type will have the same 'Type' object. JHU_Log.cs 145

Use of & instead of &&

public static String GetDocumentSource(....)
{
 ....
 int iSize = 2048;
 byte[] bytedata = new byte[2048];
 int iBOMLength = 0;

 while (true)
 {
  iSize = memstream.Read(bytedata, 0, bytedata.Length);
  if (iSize > 0)
   {
    if (!IsUnicodeDetermined)
     {
        ....
        if ((bytedata[0] == 0xEF) & 
            (bytedata[1] == 0xBB) & 
            (bytedata[2] == 0xBF)) //UTF8
            {
              IsUTF8 = true;
              IsBOMPresent = true;
            }

        if (!IsUTF16LE & !IsUTF16BE & !IsUTF8)
          {
            if ((bytedata[1] == 0) & 
                (bytedata[3] == 0) & 
                (bytedata[5] == 0) & 
                (bytedata[7] == 0))
                {
                  IsUTF16LE = true; //best guess
                }
          }
    ....
    }
  }
  ....
}

Warnings:

  • V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 280
  • V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. utils.cs 291

It's hard to say, whether this code leads to an error or not. But it looks dangerous, because the & operator evaluates both operands before the execution. We should use && operator instead. If it turns out that the program, for example, has read just one symbol, there will still be access to the elements bytedata[2], bytedata[3] and so on, which can lead to unexpected and unpleasant consequences.

Redundant code

protected IWidgetCollection m_ChildWidgets = 
                     new WidgetCollection();

public interface IWidgetCollection
{
  ....
  IWidget this[int index] {get;set;}
  ....
}

public void Render(DrawArgs drawArgs)
{
 ....
 for(int index = m_ChildWidgets.Count-1; index>=0; index--)
  {
   IWidget currentChildWidget = 
            m_ChildWidgets[index] as IWidget; // <=
   ....
  }
 ....
}       

Warning:

  • V3051 An excessive type cast. The object is already of the 'IWidget' type. PanelWidget.cs 749

This, of course, is not an error, but clearly, the code is redundant. Casting an object to the type that the object already is, won't do any good. If we remove as, nothing will change. In case the code does not match, the code won't be compiled. This warning was issued too many times, so I think that the redundant code should be fixed in many places.

Other detected places:

  • V3051 An excessive type cast. The object is already of the 'IWidget' type. FormWidget.cs 1174
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 80
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 219
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 244
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 269
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 294
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 313
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 337
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 362
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 387
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. RootWidget.cs 412
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 24
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 148
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 167
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 186
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 204
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 222
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWind.Widgets.RootWidget.cs 246
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. WorldWindow.Widgets.Form.cs 429
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_FormWidget.cs 1132
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 80
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 215
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 240
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 265
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 290
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 315
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 340
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 365
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_RootWidget.cs 390
  • V3051 An excessive type cast. The object is already of the 'IWidget' type. JHU_PanelWidget.cs 744

Strange code

public void LoadUrl(String url)
{
  ....
  if (!isCreated)
   {
    Debug.WriteLine("Doc not created" + 
                    iLoadAttempts.ToString());
    if (iLoadAttempts < 2)
     {
       this.bLoadUrlWhenReady = true;
       return;
     }
     else
     {
       throw new HtmlEditorException("Document not created");
     }
   }

  if (!isCreated) return; // <=
  ....
}

Warning:

  • V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless HtmlEditor.cs 1480

Here is an example of a fairly strange code fragment. There is definitely a bug here, but where exactly it is, is hard to say. The code tries to load a page and in the case of a failure starts checking. If the number of attempts to download is less than two, then it moves to the next try, if not - it issues a warning. However, after it yields an error, there is a forced exit from the function. Perhaps it's some sort of a precautionary measure, or it may just be extra code. Unfortunately, only the developers can say for sure what's wrong with this fragment.

Conclusion

As we have stated many times before, active copying of code leads to frequent errors, and it's very hard to avoid it. However, it's convenient to copy the code, and there is no way to write code without it. Fortunately, as you can see, PVS-Studio analyzer can help prevent many bugs related to copying of code and typos. I suggest downloading it and trying it on your project.



Use PVS-Studio to search for bugs in C, C++, and C# code

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;


Do you make errors in the code?

Check your code
with PVS-Studio

Static code analysis
for C, C++, and C#

goto PVS-Studio;
On our website we use a cookie to collect information of a technical nature.
If you do not agree, please leave the site. Learn More →
Do not show again