# Celestia: bugs' adventures in space 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\. ![0673_Celestia/image1.png](https://import.viva64.com/docx/blog/0673_Celestia/image1.png) ## Introduction The official website of the Celestia project provides its detailed description\. The source code is available on [GitHub](https://github.com/CelestiaProject/Celestia)\. 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](https://pvs-studio.com/en/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](https://github.com/microsoft/vcpkg) \- Microsoft library manager\. According to reviews, it is inferior to [Conan's](https://conan.io/) capacities, but this program was also quite convenient to use\. ## Analysis results **Warning 1** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions to the left and to the right of the '<' operator: b\.nAttributes < b\.nAttributes cmodfix\.cpp 378 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/blog/posts/cpp/0509/)"\. **Warning 2** [V575](https://pvs-studio.com/en/docs/warnings/v575/) The 'memset' function processes '0' elements\. Inspect the third argument\. winmain\.cpp 2235 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v595/) The 'destinations' pointer was utilized before it was verified against nullptr\. Check lines: 48, 50\. wintourguide\.cpp 48 ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp class filesystem_error : std::system_error { public: filesystem_error(std::error_code ec, const char* msg) : std::system_error(ec, msg) { } }; // filesystem_error ``` The 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](https://pvs-studio.com/en/docs/warnings/v713/) The pointer 's' was utilized in the logical expression before it was verified against nullptr in the same logical expression\. winmain\.cpp 3031 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v773/) The function was exited without releasing the 'vertexShader' pointer\. A memory leak is possible\. modelviewwidget\.cpp 1517 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v547/) Expression '\!inputFilename\.empty\(\)' is always true\. makexindex\.cpp 128 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v556/) The values of different enum types are compared: switch\(ENUM\_TYPE\_A\) \{ case ENUM\_TYPE\_B: \.\.\. \}\. render\.cpp 7457 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v581/) The conditional expressions of the 'if' statements situated alongside each other are identical\. Check lines: 2844, 2850\. shadermanager\.cpp 2850 ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v624/) The constant 3\.14159265 is being utilized\. The resulting value could be inaccurate\. Consider using the M\_PI constant from \. 3dstocmod\.cpp 62 ```cpp 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](https://en.wikipedia.org/wiki/The_Day_After_Tomorrow)" and the documentary series "[Through the Wormhole with Morgan Freeman](https://en.wikipedia.org/wiki/Through_the_Wormhole)"\. We're glad that by checking [various](https://pvs-studio.com/en/blog/inspections/) 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](https://pvs-studio.com/en/blog/posts/0614/) of free licensing\. Use static code analyzers, make your projects more reliable and better\!