Our website uses cookies to enhance your browsing experience.
Bugs in code are nothing new. Today, we're exploring not just some bugs, but cosmic bugs—literally! What does a NASA project have to hide? Get your tinfoil hats ready and let's go!
WorldWind Java is an open-source platform developed by NASA for visualizing geospatial data in 3D. This Java-based project provides developers with the tools necessary to create interactive applications, displaying satellite imagery, digital elevation models, geographic annotations, and other spatial layers. WorldWind Java is used for everything from developing scientific and application-specific solutions in geographic information systems and environmental modeling to supporting educational purposes and data visualization.
In this article, we'll see what bugs PVS-Studio static analyzer has detected in the project source code.
Note. We checked the same project in C# back in 2016. You can read the article here.
As usual, let's start with the classic—developers are trying to dereference nothing.
Fragment 1
....
this.trackLayers.remove(track);
this.wwd.getModel().getLayers().remove(layer); <=
if (this.wwd != null) { <=
this.wwd.redraw();
}
....The PVS-Studio warning: V6060 The 'this.wwd' reference was utilized before it was verified against null. TrackController.java 229
Indeed, it's the classic. Developers check the wwd property for null after it has been used. So, they might have thought that this property could be null and handled properly, just not entirely.
A similar thing happens in the next fragment.
Fragment 2
public ProjectionTransverseMercator(Angle centralMeridian)
{
super(makeProjectionLimits(centralMeridian, DEFAULT_WIDTH)); <=
if (centralMeridian == null) <=
{
....
}
this.centralMeridian = centralMeridian;
}The PVS-Studio warning: V6060 The 'centralMeridian' reference was utilized before it was verified against null. ProjectionTransverseMercator.java 72
The situation here is a bit more interesting: the object is used inside the mysterious makeProjectionLimits method. Let's take a peek:
protected static Sector makeProjectionLimits(
Angle centralMeridian,
Angle width)
{
double minLon = centralMeridian.degrees - width.degrees;
....
}The very first operation dereferences the centralMeridian object. So, it's worth checking centralMeridian for null before calling this method.
Typebusters
An intriguing headline, don't you think? Take a look at the following snippet:
Fragment 3
protected short outlineStipplePattern;
....
public BasicShapeAttributes() {
....
this.outlineWidth = 1;
this.outlineStippleFactor = 0;
this.outlineStipplePattern = (short) 0xF0F0; <=
this.imageSource = null;
this.imageScale = 1;
....
}
....The PVS-Studio warning: V6124 Converting an integer literal to the type with a smaller value range will result in overflow. BasicShapeAttributes.java 105
A fixed number of bytes is allocated for a certain integer type. If the value exceeds that allocated range, then extra bits will be... just cut off. However, the code still compiles and works, but the value will be no longer what we've expected.
In this snippet, the devs are attempting to assign 61680 to the short type, where the maximum value is 32767. This leads to an overflow.
This project has quite a few similar warnings. Here's another example.
Fragment 4
....
final int BAND_Y = 0, BAND_ALPHA = 1;
final int ALPHA_OPAQUE = (short) 0xFFFF; <=
final int ALPHA_TRANSLUCENT = (short) 0;
....The PVS-Studio warning: V6124 Converting an integer literal to the type with a smaller value range will result in overflow. ImageUtil.java 2085
After executing this code line, we get the -1 value in the ALPHA_OPAQUE variable. Now let's tweak the code by changing the cast to int.
....
final int BAND_Y = 0, BAND_ALPHA = 1;
final int ALPHA_OPAQUE = (int) 0xFFFF; <=
final int ALPHA_TRANSLUCENT = (short) 0;
....In this code fragment, we get the 65535 value. Maybe the developers wanted to pull off a clever type-casting trick, but I think they ended up outsmarting themselves.
Fragment 5
....
protected static final int UTM_MAX_LATITUDE = 84;
....
double northLat = UTM_MAX_LATITUDE / 2;
....The PVS-Studio warning: V6094 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. UTMGraticuleLayer.java 459
Here's another type-casting error: the developers divide one integer by another and write the result into a variable with the double type. So, we assume that the division result will be a floating-point number. In fact, the division result will also be the integer, and then it will be cast to the double type.
To prevent the fractional part from suddenly disappearing, the developers just need to explicitly cast it to the double type:
....
protected static final int UTM_MAX_LATITUDE = 84;
....
double northLat = (double)UTM_MAX_LATITUDE / 2;
....So, what could've gone wrong when using conditional statements? For example, you might run into a classic Hobson's choice.
Fragment 6
....
final double CENTER_START = this.useMidZoom ? 0.2 : 0.0;
final double CENTER_STOP = this.useMidZoom ? 0.8 : 0.8; <=
....The PVS-Studio warning: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value '0.8'. KMLOrbitViewController.java 287
The urge to read too much into things and figure out the reason behind them is strong, but I don't think it's necessary here. Interestingly enough, the developers copied this code fragment into another method, and the analyzer issued the same warning there.
Fragment 7
if (Southern_Hemisphere != 0)
{
Easting = -(rho * Math.sin(dlam) - Polar_False_Easting);
Northing = rho * Math.cos(dlam) + Polar_False_Northing;
} else
Easting = rho * Math.sin(dlam) + Polar_False_Easting;
Northing = -rho * Math.cos(dlam) + Polar_False_Northing;
....The PVS-Studio warning: V6040 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. PolarCoordConverter.java 349
Wanna see Python code in Java? Here's your order! In the else branch of this conditional statement, the developers forgot to put curly brackets. They assumed that everything would operate smoothly. However, the second else line in the branch that assigns the Northing variable will be executed regardless of the condition result—so this variable value may be incorrect.
Fragment 8
....
if (altitude == null || altitude == 0)
{
double t = sector.getDeltaLonRadians() >
sector.getDeltaLonRadians() ?
sector.getDeltaLonRadians() :
sector.getDeltaLonRadians(); <=
....
}
....The PVS-Studio warning: V6012 The '?:' operator, regardless of its conditional expression, always returns one and the same value 'sector.getDeltaLonRadians()'. GazetteerPanel.java 328
This fragment is more tangled than the previous one. A call to the same method is compared to itself, and the if and else branches just call the sector.getDeltaLonRadians method again.
It's quite simple here, though. An object of the Sector type has the getDeltaLonRadians and getDeltaLatRadians methods that return latitude and longitude deltas in radians, respectively. The developers just mixed up the method names, which makes the ternary operator completely meaningless.
A fix could look like this:
....
if (altitude == null || altitude == 0)
{
double t = sector.getDeltaLonRadians() >
sector.getDeltaLatRadians() ?
sector.getDeltaLonRadians() :
sector.getDeltaLatRadians();
....
}
....The project contains more than one such an error!
Fragment 9
....
if (MGRS.getLatitude().degrees != 0 ||
MGRS.getLatitude().degrees != 0)
{
lat = MGRS.getLatitude();
lon = MGRS.getLongitude();
}
else
return null;
....The PVS-Studio warning: V6001 There are identical sub-expressions 'MGRS.getLatitude().degrees != 0' to the left and to the right of the '||' operator. GoToCoordinatePanel.java 185
Yeah, the developers mixed up latitude and longitude again—so, the same condition is repeated twice, as the analyzer pointed out.
Fragment 10
....
if (this.currentResourceTile != null)
{
if (this.currentResourceTile.getLevelNumber() == 0 &&
this.forceLevelZeroLoads &&
!this.currentResourceTile.isTextureInMemory(
dc.getTextureCache()) && <=
!this.currentResourceTile.isTextureInMemory(
dc.getTextureCache() <=
)
)
this.forceTextureLoad(this.currentResourceTile);
....The PVS-Studio warning: V6001 There are identical sub-expressions to the left and to the right of the '&&' operator. TiledImageLayer.java 478
The same condition appears on both sides of the && operator. Maybe the developers would like to use another method like in the previous code fragment, but I couldn't find it.
However, I found another similar fragment.
Fragment 11
....
if (this.currentResourceTile != null)
{
if (this.currentResourceTile.getLevelNumber() == 0 &&
this.forceLevelZeroLoads &&
!this.currentResourceTile.isTextureInMemory(
dc.getTextureCache()) && <=
!this.currentResourceTile.isTextureInMemory(
dc.getTextureCache() <=
)
)
this.forceTextureLoad(this.currentResourceTile);
....The PVS-Studio warning: V6001 There are identical sub-expressions to the left and to the right of the '&&' operator. MercatorTiledImageLayer.java 399
Have you ever experienced déjà vu? Yep, this is the same exact code fragment that the developers copied to another part of the project, specifically to another implementation of the AbstractLayer interface.
In addition to the condition issue that the analyzer has detected, we can see that there's something wrong with the OOP design of the application when a rather large code fragment is simply copied from one implementation to another.
However, is this the last snippet where the developers have copied something?
Oh my, you have no idea what consequences may arise after pressing everyone's favorite combo—Ctrl+C and Ctrl+V...
Fragment 12
if (line.startsWith("LINE "))
....
else if (line.startsWith("EDGEVIS "))
this.edgeVis = getStringValue(line);
else if (line.startsWith("COLRTABLE "))
this.colorTable = getIntegerValues(line);
else if (line.startsWith("COLRTABLE "))
this.colorTable = getIntegerValues(line);
else if (line.startsWith("SCALEMODE "))
this.scale = getScaleValue(line);The PVS-Studio warning: V6003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. GeoSymAttributeConverter.java 151
Yes, in this chain of the if statements, two conditions repeat each other. Maybe the developers copied extra lines while adding a few other conditions and forgot to delete them—or they meant to write something completely different.
Fragment 13
....
switch (control) {
case NORTH:
azimuth = Angle.ZERO;
break;
....
case NORTH_LEADER:
azimuth = Angle.ZERO;
break;
default:
break;
}
....The PVS-Studio warning: V6067 Two or more case-branches perform the same actions. MeasureTool.java 1601
In this switch-case, the same code is executed in two cases. That's possible, but the developers should've combined two conditions to avoid repeating the code:
....
switch (control) {
case NORTH:
case NORTH_LEADER:
azimuth = Angle.ZERO;
break;
....
default:
break;
}
....However, it's also possible that the NORTH_LEADER behavior should've differed from the NORTH behavior.
Fragment 14
public final URL getUrl()
{
return url;
}
....
public final URL getURL()
{
return this.url;
}The PVS-Studio warning: V6032 It is odd that the body of method 'getUrl' is fully equivalent to the body of another method 'getURL'. URLRetriever.java 123
Did I ever tell you what the definition of insanity is? Insanity is doing the exact same thing over and over again, expecting things to change.
Vaas Montenegro, Far Cry 3
Yeah, these methods are like two peas in a pod. Yet there's a slight difference: the second one accesses url via this, and the URL in its name is written in all capital letters. That's it!
But here's the real head-scratcher: they're unused. Not only the developers have written an existing method twice, but they also don't use it anywhere else :)
Fragment 15
public void beginPolyline()
{
this.lastIndex = -1;
}
public void endPolyline()
{
this.lastIndex = -1;
}The PVS-Studio warning: V6032 It is odd that the body of method 'beginPolyline' is fully equivalent to the body of another method 'endPolyline'. PolylineTessellator.java 57
The first method writes the -1 value to lastIndex. The second method also writes the -1 value to lastIndex. So, here's a question for fellow experts: why are there two methods here?
I don't have an answer for that. It's likely that each of these methods would do something other than assignment. But hey, this code commit is approaching its 10th anniversary this October. Happy birthday, a little commit :)
Fragment 16
public void removePropertyChangeListener(
java.beans.PropertyChangeListener listener)
{
if (listener == null)
{
String msg = Logging.getMessage(
"nullValue.ListenerIsNull"
);
Logging.logger().severe(msg);
throw new IllegalArgumentException(msg);
}
this.getChangeSupport()
.addPropertyChangeListener(listener);
}Take a closer look at this method—seriously, I mean it. I even asked Mister Cody Fantastic to hide everything below to avoid spoilers.
Got it? Now look at this method:
public void addPropertyChangeListener(
java.beans.PropertyChangeListener listener)
{
if (listener == null)
{
String msg = Logging.getMessage(
"nullValue.ListenerIsNull"
);
Logging.logger().severe(msg);
throw new IllegalArgumentException(msg);
}
this.getChangeSupport()
.addPropertyChangeListener(listener);
}Now, please answer the question: how are they different?
If you answered, "They're not," you're almost right—only the names are different. One method, judging by its name, adds ChangeListener, and the other removes it. However, the method logic is identical. The analyzer reported the same thing:
The PVS-Studio warning: V6032 It is odd that the body of method 'addPropertyChangeListener' is fully equivalent to the body of another method 'removePropertyChangeListener'. KMLRoot.java 1290
Fragment 17
public double getLegLength()
{
return this.arrowLength;
}We'll skip the mental gymnastics this time and jump straight to another getter from which this one has been copied.
public double getArrowLength()
{
return this.arrowLength;
}The PVS-Studio warning: V6091 Suspicious getter implementation. The 'legLength' field should probably be returned instead. AttackByFirePosition.java 169
That's right, the developers forgot to change the object field that this getter is supposed to return, which causes arrowLength to be returned instead of legLength.
Fragment 18
....
switch (imageType)
{
case IMAGE_TYPE_ALPHA_RGB:
rgbColor = 0xFF000000 + rgbColor;
break;
//case IMAGE_TYPE_GRAY:
// break;
//case IMAGE_TYPE_RGB:
// break;
case IMAGE_TYPE_GRAY_ALPHA:
rgbColor = (rgbColor << 8) + 0xFF;
break;
case IMAGE_TYPE_RGB_ALPHA:
rgbColor = (rgbColor << 8) + 0xFF;
break;
}
....The PVS-Studio warning: V6002 The switch statement does not cover all values of the 'RPFImageType' enum: IMAGE_TYPE_RGB, IMAGE_TYPE_GRAY. NITFSImageSegment.java 281
The analyzer has reported that this switch-case doesn't cover all possible values of the imageType enumeration—and that's true. Moreover, the developers have commented on two cases that are uncovered.
Yes, they're idle, but they have originally been there. What's going on here is a mystery. There might have been some handling of such cases.
Of the whole project, this code fragment is probably the biggest mystery to me.
Fragment 19
protected String askForDatasetName(
String suggestedName)
{
String datasetName = suggestedName;
for (; ; )
{
Object o = JOptionPane.showInputDialog(....);
if (!(o instanceof String)) // user canceled the input
{
Thread.interrupted();
String msg = Logging.getMessage(....);
Logging.logger().info(msg);
throw new WWRuntimeException(msg);
}
return WWIO.replaceIllegalFileNameCharacters((String) o);
}
}The PVS-Studio warning: V6037 An unconditional 'return' within a loop. DataInstaller.java 273
There are the infinite for(; ; ; ) loop—it's similar to while(True), but it exits after the first iteration. Yet this return has no condition. There should probably be a condition here.
It's also unclear what exactly went wrong here. Using an infinite loop is problematic enough, but in this case, we don't even know why it has been created in the first place.
Now let's look at the code fragments where something is wrong with the collections. Like this one, for example.
Fragment 20
....
int[] size = new int[] {desiredSize, desiredSize};
....
if (dLon.radians > dLat.radians)
{
size[0] = desiredSize; <=
size[1] = minSize;
}
else
{
size[0] = minSize;
size[1] = desiredSize; <=
}
....The PVS-Studio warnings:
V6026 This value is already assigned to the 'size[1]' variable. ExportImageOrElevations.java 415
V6026 This value is already assigned to the 'size[0]' variable. ExportImageOrElevations.java 409
The code authors probably overlooked the fact that both array elements already have desiredSize values—and assigned that value again. Yet they picked the element index based on the condition. In fact, they could remove one line from each branch and the code would work the same way.
Fragment 21
public void releaseNameCopyBuffer(char[] buf)
{
if (buf != null) {
if (buf != _nameCopyBuffer) { <=
throw new IllegalArgumentException(....);
}
}
....
}The PVS-Studio warning: V6013 Arrays 'buf' and '_nameCopyBuffer' are compared by reference. Possibly an equality comparison was intended. IOContext.java 234
The devs are comparing two arrays here, but they do so incorrectly. The thing is, when comparing arrays, developers should've used the Arrays.equals method because the != operator compares object references, not the actual contents . It's likely the developers meant to compare the actual contents.
Let's see how things went wrong when passing arguments.
Fragment 22
private final void _skipUtf8_3(int c)
throws IOException, JsonParseException
{
if (_inputPtr >= _inputEnd) {
loadMoreGuaranteed();
}
//c &= 0x0F;
c = (int) _inputBuffer[_inputPtr++]; <=
....
}The PVS-Studio warning: V6023 Parameter 'c' is always rewritten in method body before being used. Utf8StreamParser.java 1585
So, the code authors take the c integer argument and then immediately override it with another value. Why? A samurai has no goal, only a path...
There was a line before the argument reassignment that could've fixed the whole mess... if it were where the analyzer pointed to.
Plus, there are multiple methods in the same fragment, and they're all called within the same switch-case.
switch (codes[c]) {
case 1:
_decodeEscaped();
break;
case 2:
_skipUtf8_2(c);
break;
case 3:
_skipUtf8_3(c);
break;
case 4:
_skipUtf8_4(c);
break;
default:
....
}Interestingly enough, the passed parameter is unused in each of these methods! For example, here's the _skipUtf8_2 method:
private final void _skipUtf8_2(int c)
throws IOException, JsonParseException
{
if (_inputPtr >= _inputEnd) {
loadMoreGuaranteed();
}
c = (int) _inputBuffer[_inputPtr++];
if ((c & 0xC0) != 0x080) {
_reportInvalidOther(c & 0xFF, _inputPtr);
}
}The c argument is exactly the same as in the method above. Yet this isn't the limit either. This is what the skipUtf8_4 method hides:
private final void _skipUtf8_4(int c)
throws IOException, JsonParseException
{
if (_inputPtr >= _inputEnd) {
loadMoreGuaranteed();
}
int d = (int) _inputBuffer[_inputPtr++];
if ((d & 0xC0) != 0x080) {
_reportInvalidOther(d & 0xFF, _inputPtr);
}
if (_inputPtr >= _inputEnd) {
loadMoreGuaranteed();
}
if ((d & 0xC0) != 0x080) {
_reportInvalidOther(d & 0xFF, _inputPtr);
}
if (_inputPtr >= _inputEnd) {
loadMoreGuaranteed();
}
d = (int) _inputBuffer[_inputPtr++];
if ((d & 0xC0) != 0x080) {
_reportInvalidOther(d & 0xFF, _inputPtr);
}
}That's right, the c argument isn't used here, but some local d variable is evaluated instead.
So, none of the methods that are called in switch-case require this argument. Looks like the devs passed it there for some reason, but it's not obvious why.
Fragment 23
public static List<Layer> getLayersRemoved(
LayerList oldList, LayerList newList)
{
return getListDifference(newList, oldList);
}Nothing bad seems to happen: we take two parameters, then call another method, passing these parameters, though... Now, let's take a look at the signature of the method being called here:
public static List<Layer> getListDifference(
LayerList oldList, LayerList newList)Yes, the code authors mixed up the argument order. The oldList should be passed first, followed by the newList. However, the order in this method call is incorrect, which breaks the program logic. The analyzer reported the same thing.
The PVS-Studio warning: V6029 Possible incorrect order of arguments passed to method: 'newList', 'oldList'. LayerList.java 143
These are the errors I found in the WorldWind Java project. While reading the text, you might have thought I was teasing the NASA developers, but this isn't the case. NASA undoubtedly employs great engineers. All I've done is highlight the mistakes they've made. Happens to the best of us!
I'll report any detected errors to the developers via Issue on GitHub, which will hopefully help improve the project.
You can try the PVS-Studio static code analyzer on your project using this link.
See ya!
0
0
0
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
16
0
0