Celestia is a three-dimensional space simulator. Simulation of the space allows exploring our universe in three dimensions. Celestia is available on Windows, Linux and macOS. The project is very small and PVS-Studio detected few defects in it. Despite this fact, we'd like to pay attention to it, as it's a popular educational project and it will be rather useful to somehow improve it. By the way, this program is used in popular films, series and programs for showing space. This fact, in turns, raises requirements to the code quality.
Introduction
The official website of the Celestia project provides its detailed description. The source code is available on GitHub. The analyzer checked 166 .cpp files, excluding libraries and tests. The project is small, but found defects are noteworthy.
To do the source code analysis we used the PVS-Studio static code analyzer. Both Celestia and PVS-Studio are cross-platform. We analyzed the project on the Windows platform. It was simple to build the project by getting dependencies using Vcpkg — Microsoft library manager. According to reviews, it is inferior to Conan's capacities, but this program was also quite convenient to use.
Analysis results
Warning 1
V501 There are identical sub-expressions to the left and to the right of the '<' operator: b.nAttributes < b.nAttributes cmodfix.cpp 378
bool operator<(const Mesh::VertexDescription& a,
const Mesh::VertexDescription& b)
{
if (a.stride < b.stride)
return true;
if (b.stride < a.stride)
return false;
if (a.nAttributes < b.nAttributes) // <=
return true;
if (b.nAttributes < b.nAttributes) // <=
return false;
for (uint32_t i = 0; i < a.nAttributes; i++)
{
if (a.attributes[i] < b.attributes[i])
return true;
else if (b.attributes[i] < a.attributes[i])
return false;
}
return false;
}How easy it is to make a mistake when copying code. We write about it in every review. Apparently, only static code analysis can help out in this situation.
The programmer copied the conditional expression and didn't fully edit it. The correct version is most likely as follows:
if (a.nAttributes < b.nAttributes)
return true;
if (b.nAttributes < a.nAttributes)
return false;An interesting research on this topic: "The Evil within the Comparison Functions".
Warning 2
V575 The 'memset' function processes '0' elements. Inspect the third argument. winmain.cpp 2235
static void BuildScriptsMenu(HMENU menuBar, const fs::path& scriptsDir)
{
....
MENUITEMINFO info;
memset(&info, sizeof(info), 0);
info.cbSize = sizeof(info);
info.fMask = MIIM_SUBMENU;
....
}The code author mixed up the second and third arguments of the memset function. Instead of filling the structure with zeros, it says to fill 0 bytes of memory.
Warning 3
V595 The 'destinations' pointer was utilized before it was verified against nullptr. Check lines: 48, 50. wintourguide.cpp 48
BOOL APIENTRY TourGuideProc(....)
{
....
const DestinationList* destinations = guide->appCore->getDestinations();
Destination* dest = (*destinations)[0];
guide->selectedDest = dest;
if (hwnd != NULL && destinations != NULL)
{
....
}
....
}The destinations pointer gets dereferenced two lines before it's compared with NULL. Such code can potentially lead to an error.
Warning 4
V702 Classes should always be derived from std::exception (and alike) as 'public' (no keyword was specified, so compiler defaults it to 'private'). fs.h 21
class filesystem_error : std::system_error
{
public:
filesystem_error(std::error_code ec, const char* msg) :
std::system_error(ec, msg)
{
}
}; // filesystem_errorThe analyzer has detected the class inherited from the std::exception class via the private modifier (set by default). Such inheritance is dangerous because the std::exception exception will not be caught due to non-public inheritance. As a result, exception handlers behave not as intended.
Warning 5
V713 The pointer 's' was utilized in the logical expression before it was verified against nullptr in the same logical expression. winmain.cpp 3031
static char* skipUntilQuote(char* s)
{
while (*s != '"' && s != '\0')
s++;
return s;
}In one piece of the conditional expression the programmer forgot to dereference the s pointer. It turned out to be a comparison of the pointer, not its value. And it doesn't make sense in this situation.
Warning 6
V773 The function was exited without releasing the 'vertexShader' pointer. A memory leak is possible. modelviewwidget.cpp 1517
GLShaderProgram*
ModelViewWidget::createShader(const ShaderKey& shaderKey)
{
....
auto* glShader = new GLShaderProgram();
auto* vertexShader = new GLVertexShader();
if (!vertexShader->compile(vertexShaderSource.toStdString()))
{
qWarning("Vertex shader error: %s", vertexShader->log().c_str());
std::cerr << vertexShaderSource.toStdString() << std::endl;
delete glShader;
return nullptr;
}
....
}The memory is released by the glShader pointer but isn't cleared by the vertexShader pointer when exiting the function.
A similar fragment below:
- V773 The function was exited without releasing the 'fragmentShader' pointer. A memory leak is possible. modelviewwidget.cpp 1526
Warning 7
V547 Expression '!inputFilename.empty()' is always true. makexindex.cpp 128
int main(int argc, char* argv[])
{
if (!parseCommandLine(argc, argv) || inputFilename.empty())
{
Usage();
return 1;
}
istream* inputFile = &cin;
if (!inputFilename.empty())
{
inputFile = new ifstream(inputFilename, ios::in);
if (!inputFile->good())
{
cerr << "Error opening input file " << inputFilename << '\n';
return 1;
}
}
....
}Repeated check of the file name presence. It's not a bug, but due to the fact that the inputFilename variable is already checked at the beginning of the function, the check below can be removed, making the code more compact.
Warning 8
V556 The values of different enum types are compared: switch(ENUM_TYPE_A) { case ENUM_TYPE_B:… }. render.cpp 7457
enum LabelAlignment
{
AlignCenter,
AlignLeft,
AlignRight
};
enum LabelVerticalAlignment
{
VerticalAlignCenter,
VerticalAlignBottom,
VerticalAlignTop,
};
struct Annotation
{
....
LabelVerticalAlignment valign : 3;
....
};
void Renderer::renderAnnotations(....)
{
....
switch (annotations[i].valign)
{
case AlignCenter:
vOffset = -font[fs]->getHeight() / 2;
break;
case VerticalAlignTop:
vOffset = -font[fs]->getHeight();
break;
case VerticalAlignBottom:
vOffset = 0;
break;
}
....
}Enumeration values are mixed up in the switch operator. Because of this, enumerations of different types are compared in one fragment: LabelVerticalAlignment and AlignCenter.
Warning 9
V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 2844, 2850. shadermanager.cpp 2850
GLVertexShader*
ShaderManager::buildParticleVertexShader(const ShaderProperties& props)
{
....
if (props.texUsage & ShaderProperties::PointSprite)
{
source << "uniform float pointScale;\n";
source << "attribute float pointSize;\n";
}
if (props.texUsage & ShaderProperties::PointSprite)
{
source << DeclareVarying("pointFade", Shader_Float);
}
....
}The analyzer has detected two identical conditional expressions in a row. Either an error has been made or two conditions can be combined into one, and thus make the code simpler.
Warning 10
V668 There is no sense in testing the 'dp' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. windatepicker.cpp 625
static LRESULT
DatePickerCreate(HWND hwnd, CREATESTRUCT& cs)
{
DatePicker* dp = new DatePicker(hwnd, cs);
if (dp == NULL)
return -1;
....
}The value of the pointer returned by the new operator is compared with null. If the operator was unable to allocate memory, then according to the C++ standard, an exception std::bad_alloc() is thrown. Then the check for null is pointless.
Three more similar checks:
- V668 There is no sense in testing the 'modes' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 2967
- V668 There is no sense in testing the 'dropTarget' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 3272
- V668 There is no sense in testing the 'appCore' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. winmain.cpp 3352
Warning 11
V624 The constant 3.14159265 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>. 3dstocmod.cpp 62
int main(int argc, char* argv[])
{
....
Model* newModel = GenerateModelNormals(*model,
float(smoothAngle * 3.14159265 / 180.0), weldVertices, weldTolerance);
....
}The diagnostic is optional but in this case it's better to use the ready-made constant for the Pi number from the standard library.
Conclusion
Recently the project has been developed by enthusiasts, but is still popular and in demand in the training programs. There are thousands of addons with different space objects on the Internet. Celestia was used in the film "The Day After Tomorrow" and the documentary series "Through the Wormhole with Morgan Freeman".
We're glad that by checking various projects with open source code we're not only promoting static code analysis methodology, but also contribute to development of open source projects. By the way, you can also use the PVS-Studio analyzer not only to test your own, but also third-party projects as an enthusiast. To do this, you can use one of the options of free licensing.
Use static code analyzers, make your projects more reliable and better!
