Over the course of its history, humanity has been making enormous efforts to study the night sky. By now, we have mapped almost the entire area of it. We have observed hundreds of thousands of asteroids, comets, planets and stars, nebulas and galaxies. To see all these wonders yourself, you don't even have to leave home and buy a telescope - you can simply install Stellarium, a virtual planetarium, on your computer and explore the night sky while stretching out comfortably on your sofa... But is it that comfortable? Let's check the code of Stellarium for bugs to find it out.
According to the Wikipedia page, Stellarium is an open-source free-software planetarium, licensed under the terms of the GNU General Public License version 2, available for Linux, Windows, and macOS. A port Stellarium called Stellarium Mobile is available for Android, iOS, and Symbian as a paid version, being developed by Noctua Software. All versions use OpenGL to render a realistic projection of the night sky in real time.
Stellarium was created by the French programmer Fabien Chéreau, who launched the project in the summer of 2001 (17 years ago). Currently, Stellarium is being maintained and developed by Alexander Wolf, Georg Zotti, Marcos Cardinot, Guillaume Chéreau, Bogdan Marinov, Timothy Reaves, Ferdinand Majerech, and Jörg Müller. A number of other developers have contributed to the development of Stellarium, especially Robert Spearman, Johannes Gajdosik, Matthew Gates, Nigel Kerr, and Johan Meuris, the latter of whom is responsible for the artwork.
The project was analyzed with the static code analyzer PVS-Studio. This is a tool to detect bugs and potential vulnerabilities in programs written in C, C++, and C# (Java support is coming soon!). It supports Windows, Linux, and macOS and is designed for developers who care about improving the code's quality.
It was quite simple to do the analysis. First, I downloaded Stellarium's source code from GitHub and then installed all software packages required to build the project. Since it is built with Qt Creator, I used the compiler-launch tracking mechanism, a special feature of the Standalone version of PVS-Studio. It can open analysis reports as well.
New readers and Stellarium users are probably wondering why I mentioned a unicorn in the title and what it has to do with code analysis. The answer is, I am one of the developers of PVS-Studio, and the unicorn is our dear playful mascot. Now, up we go!
Figure 1. Going up!
I hope you will learn something new from this article, while the authors of Stellarium will fix some of the bugs and, therefore, make the project better.
Get yourself a coffee and a croissant and sit back: we are getting to the most interesting part of our articles - overview of the bugs reported by the analyzer!
To make it more entertaining, I recommend that in every case (starting with this one) you try to find the bug yourself first and only then read the analyzer warning and my comments:
void QZipReaderPrivate::scanFiles()
{
....
// find EndOfDirectory header
int i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
const int pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
break;
++i;
}
....
}PVS-Studio diagnostic message: V654 The condition 'start_of_directory == - 1' of loop is always true. qzip.cpp 617
Found it? If yes, kudos to you!
The problem is in the condition of the while loop. This condition is always true as the start_of_directory variable does not change inside the loop body. It doesn't look like the loop is going to run forever because it has a return and break in it, but it still looks suspicious.
I think that the programmer forgot to add the assignment start_of_directory = pos in the signature check. If so, the break statement is not necessary either. The code could be rewritten as follows:
int i = 0;
int start_of_directory = -1;
EndOfDirectory eod;
while (start_of_directory == -1) {
const int pos = device->size()
- int(sizeof(EndOfDirectory)) - i;
if (pos < 0 || i > 65535) {
qWarning() << "QZip: EndOfDirectory not found";
return;
}
device->seek(pos);
device->read((char *)&eod, sizeof(EndOfDirectory));
if (readUInt(eod.signature) == 0x06054b50)
start_of_directory = pos;
++i;
}However, I'm not sure this is exactly what it is supposed to look like. The authors should check this part for themselves and make the necessary improvements.
Here's another strange condition:
class StelProjectorCylinder : public StelProjector
{
public:
....
protected:
....
virtual bool
intersectViewportDiscontinuityInternal(const Vec3d& capN,
double capD) const
{
static const SphericalCap cap1(1,0,0);
static const SphericalCap cap2(-1,0,0);
static const SphericalCap cap3(0,0,-1);
SphericalCap cap(capN, capD);
return cap.intersects(cap1)
&& cap.intersects(cap2)
&& cap.intersects(cap2);
}
};PVS-Studio diagnostic message: V501 There are identical sub-expressions 'cap.intersects(cap2)' to the left and to the right of the '&&' operator. StelProjectorClasses.hpp 175
As you have probably guessed already, the bug is in the last line of the function: the programmer made a typo that makes the function ignore the actual value of cap3 when returning.
This error pattern is extremely common: nearly every project we checked had typos in variable names of the form name1, name2, and the like. This usually happens when using the copy-paste technique.
The snippet above is an example of another common error pattern, which we even did a little research on. My colleague Andrey Karpov called it "the last line effect". If you haven't heard about it yet, I do recommend opening the article in a new tab of your browser to read later. Let's move on.
void BottomStelBar::updateText(bool updatePos)
{
....
updatePos = true;
....
if (location->text() != newLocation || updatePos)
{
updatePos = true;
....
}
....
if (fov->text() != str)
{
updatePos = true;
....
}
....
if (fps->text() != str)
{
updatePos = true;
....
}
if (updatePos)
{
....
}
}PVS-Studio diagnostic messages:
The value of the updatePos parameter is always overwritten before it can be used. That is, the function will always return with the same result no matter what value is passed to it.
It doesn't look right, does it? Whenever the updatePos parameter is used, it has the value true, which means the conditions if (location->text() != newLocation || updatePos) and if (updatePos) will be always true.
Another snippet:
void LandscapeMgr::onTargetLocationChanged(StelLocation loc)
{
....
if (pl && flagEnvironmentAutoEnabling)
{
QSettings* conf = StelApp::getInstance().getSettings();
setFlagAtmosphere(pl->hasAtmosphere()
& conf->value("landscape/flag_atmosphere", true).toBool());
setFlagFog(pl->hasAtmosphere()
& conf->value("landscape/flag_fog", true).toBool());
setFlagLandscape(true);
}
....
}PVS-Studio diagnostic messages:
The analyzer has detected a suspicious expression in the arguments of the functions setFlagAtmosphere and setFlagFog. Indeed, both operands of the bitwise operator & are values of type bool. What should be used instead of & is the && operator, and here's why.
True, the result of that expression will always be correct. Before the bitwise AND is executed, both operands will be promoted to type int. In C++, such cast is unambiguous: false converts into 0 and true converts into 1. That's why it evaluates to the same result as it would with the && operator.
However, there's one subtle yet important difference. For && operations, the so called lazy evaluation is used. If the left operand's value is false, then the right operand is not evaluated at all because the logical AND will evaluate to false anyway. This is done for the sake of saving computational resources and allowing programmers to write complex structures. For example, you can check a pointer for null and, if it's found to be non-null, dereference it to do an additional check, like this: if (ptr && ptr->foo()).
This lazy evaluation strategy is not applied to operations with the bitwise &. The expressions conf->value("...", true).toBool() will be evaluated every time no matter the value of pl->hasAtmosphere().
In rare cases, this can be a deliberate trick used, for instance, when evaluation of the right operand has certain "side effects" which the programmer wants to save for later use. That's not a good thing to do either because it makes the code harder to read and maintain. Besides, the evaluation order of the operands in the & operation is not defined, so using such "tricks" may end up with undefined behavior.
If you need to save the side effects, do that in a separate line and store the result to some separate variable. Those who will be maintaining the code later will be thankful for that :)
Figure 2. Peering into the night sky.
Moving on to the next section.
This section is about managing dynamic memory, and we'll start with the following snippet:
/************ Basic Edge Operations ****************/
/* __gl_meshMakeEdge creates one edge,
* two vertices, and a loop (face).
* The loop consists of the two new half-edges.
*/
GLUEShalfEdge* __gl_meshMakeEdge(GLUESmesh* mesh)
{
GLUESvertex* newVertex1 = allocVertex();
GLUESvertex* newVertex2 = allocVertex();
GLUESface* newFace = allocFace();
GLUEShalfEdge* e;
/* if any one is null then all get freed */
if ( newVertex1 == NULL
|| newVertex2 == NULL
|| newFace == NULL)
{
if (newVertex1 != NULL)
{
memFree(newVertex1);
}
if (newVertex2 != NULL)
{
memFree(newVertex2);
}
if (newFace != NULL)
{
memFree(newFace);
}
return NULL;
}
e = MakeEdge(&mesh->eHead);
if (e == NULL)
{
return NULL;
}
MakeVertex(newVertex1, e, &mesh->vHead);
MakeVertex(newVertex2, e->Sym, &mesh->vHead);
MakeFace(newFace, e, &mesh->fHead);
return e;
}PVS-Studio diagnostic messages:
The function allocates memory for three structures and passes it to the pointers newVertex1, newVertex2 (remember what I told you about variable names?), and newFace. If one of them turns out to be null, all memory reserved in the function is freed and the function returns NULL.
But what if memory is successfully allocated for all three structures but the MakeEdge(&mesh->eHead) function returns NULL? In that case, execution will reach the second return statement.
Since the pointers newVertex1, newVertex2, and newFace are local variables, they will cease to exist after the function returns. However, the memory that was previously allocated for them will not be freed. It will remain reserved, yet you will no longer be able to access it.
Such defects are called "memory leaks". The typical scenario involving them is this: when running for a long time, the program starts to consume more and more memory and may even use up all of it.
Note that the third return is OK in this example. The functions MakeVertex and MakeFace pass the allocated addresses to other data structures, thus delegating to them the responsibility for releasing that memory.
The next defect was found in a method more than 90 lines long. I abridged it for you and kept only the flawed lines.
void AstroCalcDialog::drawAngularDistanceGraph()
{
....
QVector<double> xs, ys;
....
}Only one line is left. Hint: this is the only time the objects xs and ys are mentioned.
PVS-Studio diagnostic messages:
The vectors xs and ys are created but never used. It turns out that every time the drawAngularDistanceGraph method is called, an empty container is created and deleted, which is totally redundant. I think this declaration is a trace of previous refactoring. It's not an error, of course, but it's still better to get rid of redundant code.
Here's one more example with a little editing by me:
void SatellitesDialog::updateSatelliteData()
{
....
// set default
buttonColor = QColor(0.4, 0.4, 0.4);
....
}To find the defect, you'll have to look at the prototypes of the constructors of the Qcolor class:
PVS-Studio diagnostic messages:
The Qcolor class doesn't have any constructors taking a value of type double as an argument, so the arguments will be implicitly cast to int. As a result, the fields r, g, b of the buttonColor object will all have the value 0.
If the programmer wanted to form an object from values of type double, they should have used a different constructor.
For example, it could be a constructor taking Qrgb as an argument:
buttonColor = QColor(QColor::fromRgbF(0.4, 0.4, 0.4));There's also another way to do that. In Qt, RGB-colors are represented by real numbers from the range [0.0, 1.0] or integers from the range [0, 255].
So, the programmer could cast the real numbers to integers as follows:
buttonColor = QColor((int)(255 * 0.4),
(int)(255 * 0.4),
(int)(255 * 0.4));or simply:
buttonColor = QColor(102, 102, 102);Starting to get bored? Don't worry: there are more interesting things ahead.
Figure 3. Unicorn in outer space. View from Stellarium. Click on the image to enlarge.
I kept a few cool examples for this last section :) Here's one of them.
HipsTile* HipsSurvey::getTile(int order, int pix)
{
....
if (order == orderMin && !allsky.isNull())
{
int nbw = sqrt(12 * 1 << (2 * order));
int x = (pix % nbw) * allsky.width() / nbw;
int y = (pix / nbw) * allsky.width() / nbw;
int s = allsky.width() / nbw;
QImage image = allsky.copy(x, y, s, s);
....
}
....
}PVS-Studio diagnostic message: V634 The priority of the '*' operation is higher than that of the '<<' operation. It's possible that parentheses should be used in the expression. StelHips.cpp 271
What about this one? Found it? Let's examine the (12 * 1 << (2 * order)) expression. The analyzer reminds us that the '*' operation has higher precedence than the bit shifting operation '<<' does. It's easy to see that multiplying 12 by 1 makes no sense and there's no need to enclose 2 * order in parentheses.
What the programmer must have really meant is this:
int nbw = sqrt(12 * (1 << 2 * order));Now the value 12 is multiplied by the correct number.
Note. There's one more thing I'd like to point out: if the value of the right operand of '<<' is greater than or equal to the number of bits of the left operand, the result is not defined. Since numeric literals are by default of type int, which is 32 bits long, the value of the order parameter must not exceed 15. Otherwise, the program may end up with undefined behavior.
Moving on. The code below is quite intricate, but I'm sure you are skilled enough to spot the bug :)
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
if (inSignDomain == sdBoth)
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
}
else if (inSignDomain == sdNegative)
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
else
{
foundRange = false;
return QCPRange();
}
}
else if (inSignDomain == sdPositive)
{
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
else
{
foundRange = false;
return QCPRange();
}
}
foundRange = false;
return QCPRange();
}PVS-Studio diagnostic message: V779 Unreachable code detected. It is possible that an error is present. qcustomplot.cpp 19512.
The point is that each if...else branch has a return statement - that's why execution will never reach the last two lines.
Technically speaking, this code will execute correctly. It's just that the presence of unreachable code is in itself a signal of some problem. In this case, it indicates that the method is not structured properly, which makes the code much harder to understand.
This function needs refactoring to make it neater. For example:
/* inherits documentation from base class */
QCPRange QCPStatisticalBox::
getKeyRange(bool& foundRange, SignDomain inSignDomain) const
{
foundRange = true;
switch (inSignDomain)
{
case sdBoth:
{
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
break;
}
case sdNegative:
{
if (mKey + mWidth * 0.5 < 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey < 0)
return QCPRange(mKey - mWidth * 0.5, mKey);
break;
}
case sdPositive: {
if (mKey - mWidth * 0.5 > 0)
return QCPRange(mKey - mWidth * 0.5, mKey + mWidth * 0.5);
else if (mKey > 0)
return QCPRange(mKey, mKey + mWidth * 0.5);
break;
}
}
foundRange = false;
return QCPRange();
}The last bug is my favorite one in this project. The snippet in question is short and straightforward:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
Plane(v1, v2, v3, SPolygon::CCW);
}Noticed anything odd? Not everyone can :)
PVS-Studio diagnostic message: V603 The object was created but it is not being used. If you wish to call constructor, 'this->Plane::Plane(....)' should be used. Plane.cpp 29
The programmer relied on several of the object's fields to be initialized in the nested constructor, but what happens instead is this. When calling the Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3) constructor, an unnamed temporary object is created and deleted right away inside it, while the fields remain uninitialized.
To make the code work properly, the developers should use a safe and handy feature of C++11 - a delegating constructor:
Plane::Plane(Vec3f& v1, Vec3f& v2, Vec3f& v3)
: Plane(v1, v2, v3, SPolygon::CCW)
{
distance = 0.0f;
sDistance = 0.0f;
}But if your compiler doesn't support the new language version, you may write it like this:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
this->Plane::Plane(v1, v2, v3, SPolygon::CCW);
}Or like this:
Plane::Plane(Vec3f &v1, Vec3f &v2, Vec3f &v3)
: distance(0.0f), sDistance(0.0f)
{
new (this) Plane(v1, v2, v3, SPolygon::CCW);
}Note that the last two solutions are pretty dangerous. Be careful and make sure you understand how exactly they work.
So, what can I say about the quality of Stellarium's code? To be honest, there weren't many bugs. What's more, I haven't found a single error that depends on undefined behavior. For an open-source project, the code is very high-quality, and I take my hat off to them. Good job, guys! It was a pleasure reviewing your project.
As for the planetarium itself, I do use it quite often. Sadly, because I live in a city, I rarely get an opportunity to marvel at clear sky at night, but Stellarium can take me to any spot on our planet without my bothering to stand up from the sofa. So, yes, it's comfortable indeed!
I especially like the "Constellation art" mode. It's really breathtaking to watch huge figures float across the sky in a mysterious dance!
Figure 4. Mysterious dance. View from Stellarium. Click on the image to enlarge.
We Earth dwellers tend to make mistakes, and there's nothing shameful about overlooking some bugs in programs. For this, code analysis tools such as PVS-Studio are being developed. If you, too, live on Earth, welcome to download and try PVS-Studio.
I hope you enjoyed reading this article and learned something cool and useful. And I also hope that the authors of Stellarium will get the bugs fixed soon. I wish them good luck with that!
Subscribe to our channels to follow the news of the programming world!
