Geekly Articles each Day
📜 ⬆️ ⬇️

Shake dust from the globe: check the project NASA World Wind

PVS-Studio and NASA World Wind Sometimes it is useful to look back and see how the analyzer could help in old projects, and which errors can be avoided in a timely manner if you use the analyzer regularly. This time, the choice fell on the NASA World Wind project, which until 2007 was developed in C #.

NASA World Wind is an interactive globe that allows you to see any place on Earth. For the project, the project uses the Landsat public image database and the Shuttle Radar Topography Mission relief modeling project. The first versions of the project were created in C #. Later the project continued its development in the Java language. The latest version released in C # is 1.4. Although the C # version has been abandoned for many years, it will not prevent us from checking the project and assessing the quality of the code developed by the NASA Ames Research Center .

Why did we check the old project? We have long been offered to check out some of the NASA projects and now we accidentally came across this project. Yes, this check will not bring any benefit to the project. But we did not set such a goal this time. We just wanted to once again demonstrate the benefits that the PVS-Studio static code analyzer can bring when developing, including NASA.
')

By the way, this is not the first time that “historical” projects have been checked. You may also be interested in the following articles:
The NASA World Wind project shows well the capabilities of the PVS-Studio analyzer. As it will be seen from the article, while writing the code, it seems, the Copy-Paste mechanisms were actively used. Many errors have multiplied and are often duplicated in the code. Some project errors are indicative and well illustrate the principles of analyzer diagnostics.

To analyze the project, we used the PVS-Studio analyzer version 6.06.

Error density


After checking the project, the analyzer issued 120 warnings of the first level and 158 warnings of the second level. After studying the messages, I believe that the code should be fixed in 122 places. Thus, the percentage of false positives was 56%. This means that every second message indicates an error or a clearly bad code that needs to be corrected.

Now let's calculate the error density. In total in the project, taking into account comments, 474240 lines of code in 943 files. Among them, 122 problem areas. We get that the analyzer finds an average of 0.25 errors per 1000 lines of code. This indicates the high quality of the code.

Last line effect


public Point3d (Point3d P) { X = PX; Y = PY; X = PZ; // <= }

A warning:
Unicorn Facepalm

Here we see the classic error in the copy constructor. When assigning three-dimensional coordinates of an object, instead of setting the variable Z , the value of the variable X was overwritten twice. It is quite obvious that this mistake was made as a result of using the “Copy-Paste methodology”. The chance to make a mistake in the last line when copying code is much higher. You can read more about this phenomenon in Andrei Karpov’s article “Last line effect”. To fix this constructor, you need to replace the variable in the last line.
 public Point3d (Point3d P) { X = PX; Y = PY; Z = PZ; }

This is not the only mistake in the project that confirms this effect. In the continuation of the article will meet some more examples proving it.

A few more suspicious places where V3008 diagnostics worked:

Logical error or insidious typo?


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

A warning:
The fragment is engaged in automatic updating of a plug-in in the project. If the version doesn’t match, he downloads the necessary plugin. If this is an internal plugin, the program refuses to update it automatically and simply gives a message about the new version. But the conditional statement located after else contains an expression that contradicts the condition for entering the else statement . The code is quite contradictory, and only developers who know exactly how the function should work can determine exactly what the error is. At the moment, I can only assume that else should not be related to this conditional operator. Or, judging by the commentary next to the conditional operator, it can be concluded that the else operator is in place, and the second if statement should have a different condition.

Copy failed


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

A warning:
In the given fragment the error connected with inaccurate copying of the code. In the last condition statement, the variable is confused. As a result, an error occurred with access to the null link. According to previous checks, the m_gpsIcon variable used in the last line is guaranteed to be null . If the condition m_gpsTrackLine! = Null works, the program will end with an error. I can assume that the correct code should look like this:
 if (m_gpsTrackLine != null) { .... labelTitle.Text = "Set options for " + m_gpsTrackLine.m_sDescription; }


Noncompliant 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:
Another example of an error arising from large code fragments created by copying code. The above function is intended for processing incoming commands and is a very long block of code consisting of conditional statements of the same type. The code will never come to re-check. There is nothing wrong with such an error, but it may well be that instead of a duplicate some other command should have been located. If the necessary command is not processed, then the code behaves in a completely different way than the programmer assumes.

The analyzer indicated at once two places in this huge tree of conditional operators. In the first place, a double line.StartsWith check ("checkBoxNoDelay =") is located nearby, and it could be noticed by scrutinizing the code, although considering the amount of code, this is a rather complicated process that would take a lot of time.

The second place is hidden from the eyes of developers much better. The first line.StartsWith check (“comboBoxAPRSInternetServer =”) is located in the middle of the tree, and the second check actually completes it, and there are about 100 lines of code between them. This case well demonstrates that sometimes static code analysis can be even more effective than a code review. Regular use of the analyzer allows you to detect such errors in the early stages and saves programmers from the painful debugging and reading of long functions.

Variable name error


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

A warning:
A typo in the variable name led to the potential use of a null reference. Instead of checking the object of the derived class robj , the base object obj is checked. In case it does not correspond to the RenderableObject type , the program will end with an error. The correction requires changing the name of the variable in the conditional statement to robj .

Access via null link


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

Warnings:
Two different diagnostics immediately indicate this code. The code violated the procedure for checking and accessing the link. First, calculate the number of elements within the object, and then check whether the object exists at all. Perhaps the this.linePoints object may never get a null value, but then the check in the internal condition is also not needed. It is logical to change the code as follows:
 if(this.linePoints != null && this.linePoints.Length > 1) { .... }


Always a false condition


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

A warning:
This error is similar to the one described in the previous section. Here, an object reference is assigned to a variable directly before conditional statements. Most likely, the reference to the object can not be zero, and verification in the internal condition is not required. If this is not the case, the check must be transferred to an external conditional statement. It is already working with the properties of the currentSurfaceImage object. If the link is zero, the error will occur before the check is performed.

Similar places:

Wandering Brace


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

A warning:
The analyzer detected a mismatch between the formatting and the logic of the code. On closer examination, it turned out that the closing bracket was forgotten in the internal if . The missing parenthesis was found after completion of the block of the second operator else . As a result, the project was able to compile, despite the inaccurate code. The compiler can only report if else ceases to belong to the conditional operator. In this case, simply shifting the else to another if statement . Since the closing parenthesis was in the wrong place, the work of two conditional statements was broken, as well as the formatting of else statements . I can guess what the code should look like after correcting the position of the bracket 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; .... } .... }

A 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 correct the error, you need to change the code as follows:
 double x2=vertex2.X; double y2=vertex2.Y;

In addition to typing errors during assignment, another oddity is noticed. Why assign a fixed value in each iteration of the loop? Inside the loop, the value does not change and it is much more logical to make this assignment once before it starts.

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

A warning:
The fDistance parameter entering the function is overwritten immediately before use. The actual value of the variable is lost, and the value of the property of the m_fTotalDistance class is used instead . There is no variable anywhere else in the function, so changing fDistance does not make sense. Judging by the other function fragments, we can assume that the variables are interchanged here, and in fact this fragment should look 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"; }


Invalid NaN check


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

A warning:
According to the documentation, you cannot compare two double.NaN values ​​with the! = Operator. The result of this comparison will always be true . For a correct check, use the double.IsNaN () method. Accordingly, the code will take the following form:
 if(!double.IsNaN(icon.OnClickZoomAltitude) || !double.IsNaN(icon.OnClickZoomHeading) || !double.IsNaN(icon.OnClickZoomTilt)) ....

The analyzer found many places using such incorrect checks:

Ignoring the result of a function


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

A warning:
Calling the Path.Combine function without processing the result does not make sense. In this case, the function serves to form the path to the object based on the absolute path to the executable file and the relative path to the image. A lack of value processing indicates an obvious error. The Path.Combine function is used in many places in the program. Based on this, we can conclude that the code should be corrected as follows:
 toolBarImage=Path.Combine(...., toolBarImage);

The error was copied to other places of the project:

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

A warning:
The code apparently checks to see if the cursor is over the ToolBar at the moment, or not. From the code you can see that there is no handling of the MenuAnchor.Left element. It is impossible to know exactly what the developers meant when writing this fragment. Probably, its processing was not required, but I consider - it is necessary to pay attention to this fragment.

Meaningless assignment


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

A warning:
Here is just a redundant code. The presence of the string maximumElevation = minimumElevation does not make sense, since at the time of assignment both variables have the same values ​​as a result of the previous operation. You can, of course, assume that the developers wanted to change the values ​​of variables, but they did it incorrectly. This is a dubious assumption, since both the comment and the fact that the maximumElevation variable is no longer used, prove the opposite.

Multiple assignment


 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 found in the project. There is no danger of this fragment, but assigning a value three times by the same change looks strange. In my opinion, it would be clearer to initialize the variable num1 immediately when it was declared, and afterwards to assign the value num1 separately to each variable.

Insecure call to event handler


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

A warning:
In a multi-threaded application, such an event call is not secure. A situation may well occur when between checking for null and directly calling the handler there will be an unsubscribe from the event in another thread. If it was the only handler, then it would lead to the use of a null reference. To safely call an event, it is recommended to use a temporary variable, then the event will be triggered correctly in any case. Other ways to fix this error can be found on the diagnostics page of V3083 .

By the way, this is a very insidious kind of error, since problems will arise extremely rarely, and it will be almost impossible to reproduce them again!

:


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

:
. . , .

:

& &&


 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:
, . , , & . &&. , , , , bytedata[2] , bytedata[3] , .


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

:
, , , . , , . as , . , , . , , . :


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

:
. , . , . , , — . , . , - , . , , , .

Conclusion


, , . , — , . , , PVS-Studio , . : .


If you want to share this article with an English-speaking audience, then please use the link to the translation: Alexander Chibisov. Dusting the globe: analysis of NASA World Wind project .

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

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

More articles:


All Articles