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.
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.
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.
public Point3d (Point3d P)
{
X = P.X;
Y = P.Y;
X = P.Z; // <=
}Warning:
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:
private static void WebUpdate(....)
{
....
if (ver != version) // <=
{
....
}
else if (ver != version) // <=
{
....
}
}Warning:
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.
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:
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;
}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:
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.
public int CompareTo(object obj)
{
RenderableObject robj = obj as RenderableObject;
if(obj == null) // <=
return 1;
return this.m_renderPriority.CompareTo(robj.RenderPriority);
}Warning:
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.
public override void Render(DrawArgs drawArgs)
{
....
if(this.linePoints.Length > 1) // <=
{
....
if(this.linePoints != null) // <=
{
....
}
}
....
}Warnings:
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)
{
....
}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:
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:
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:
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
{
....
}
....
}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:
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.
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:
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";
}public override bool PerformSelectionAction(DrawArgs drawArgs)
{
....
if(icon.OnClickZoomAltitude != double.NaN ||
icon.OnClickZoomHeading != double.NaN ||
icon.OnClickZoomTilt != double.NaN)
{
....
}
....
}Warning:
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:
private static void addExtendedInformation(....)
{
....
if(toolBarImage.Length > 0 &&
!Path.IsPathRooted(toolBarImage))
Path.Combine(...., toolBarImage); // <=
....
}Warning:
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:
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:
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.
protected virtual void CreateElevatedMesh()
{
....
if (minimumElevation > maximumElevation)
{
// Compensate for negative vertical exaggeration
minimumElevation = maximumElevation;
maximumElevation = minimumElevation;
}
....
}Warning:
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.
public static bool SearchForAddress(....)
{
double num1;
long2 = num1 = 0;
long1 = num1 = num1; // <=
lat2 = num1 = num1; // <=
lat1 = num1;
....
}Warnings:
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.
private static void m_timer_Elapsed(....)
{
....
if (Elapsed != null)
Elapsed(sender, e);
}Warning:
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.
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:
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:
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:
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.
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:
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:
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:
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.
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.