Celestia: приключения багов в космосе

    Picture 1

    Celestia — трехмерный космический симулятор. Симуляция космоса позволяет исследовать нашу вселенную в трех измерениях. Celestia доступна на Windows, Linux и macOS. Проект очень маленький и в нём, с помощью PVS-Studio, обнаруживается совсем небольшое количество дефектов. Однако нам очень хочется уделить ему внимание, так как это популярный образовательный проект, который полезно улучшить. Кстати, программа используется в популярных фильмах, сериалах и передачах для представления космоса. Что тоже повышает требования к качеству кода.

    Введение


    На официальном сайте проекта Celestia можно найти его подробное описание. Исходный код проекта размещён на GitHub. Исключив библиотеки и тесты, анализатор проверил 166 .cpp файлов. Проект маленький, но найденные дефекты достаточно интересные.

    Для анализа кода использовался статический анализатор кода PVS-Studio. И Celestia, и PVS-Studio являются кроссплатформенными. Для анализа была выбрана платформа Windows. Проект было легко собрать, подтянув зависимости с помощью Vcpkg — менеджера библиотек Microsoft. По отзывам он уступает возможностям Conan, но этой программой тоже было удобно пользоваться.

    Результаты анализа


    Предупреждение 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;
    }

    Как же легко ошибиться при копировании кода. Пишем об этом в каждом обзоре. Видимо, только статический анализ кода способен выручить в этой ситуации.

    Программист скопировал условное выражение и не до конца его отредактировал. Правильный вариант, скорее всего, такой:

    if (a.nAttributes < b.nAttributes)
      return true;
    if (b.nAttributes < a.nAttributes)
      return false;

    Интересное исследование на эту тему: "Зло живёт в функциях сравнения".

    Предупреждение 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;
      ....
    }

    Автор кода перепутал второй и третий аргументы функции memset. Вместо заполнения структуры нулями, указано заполнять 0 байт памяти.

    Предупреждение 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)
      {
        ....
      }
      ....
    }

    Указатель destinations разыменовывается на две строки выше, чем сравнивается с NULL. Такой код может потенциально привести к ошибке.

    Предупреждение 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_error

    Анализатор обнаружил класс, унаследованный от класса std::exception через модификатор private (заданный по умолчанию). Данное наследование опасно тем, что при непубличном наследовании, при попытке поймать исключение std::exception, оно будет пропущено. Как результат, обработчики исключений ведут себя не так, как задумывалось.

    Предупреждение 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;
    }

    В одном месте условного выражения забыли разыменовать указатель s. Получилось сравнение указателя, а не значения по нему. А это не имеет смысла в данной ситуации.

    Предупреждение 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;
      }
      ....
    }

    При выходе из функции освобождается память по указателю glShader, но не очищается по указателю vertexShader.

    Такое же место ниже по коду:

    • V773 The function was exited without releasing the 'fragmentShader' pointer. A memory leak is possible. modelviewwidget.cpp 1526

    Предупреждение 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;
        }
      }
      ....
    }

    Повторная проверка наличия имени файла. Не является ошибкой, но из-за того, что проверка переменной inputFilename уже есть в начале функции, ниже проверку можно убрать, что сделает код более компактным.

    Предупреждение 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;
      }
      ....
    }

    В операторе switch, перепутали значения перечислений. Из-за этого в одном месте сравниваются перечисления разных типов: LabelVerticalAlignment и AlignCenter.

    Предупреждение 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);
      }
      ....
    }

    Анализатор обнаружил два одинаковых условных выражения подряд. Тут либо допущена ошибка, либо два условия можно объединить в одно, и тем самым упростить код.

    Предупреждение 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;
      ....
    }

    Значение указателя, возвращённого оператором new, сравнивается с нулём. Если оператор не смог выделить память, то согласно стандарту языка Си++, генерируется исключение std::bad_alloc(). Таким образом, проверять указатель на равенство нулю не имеет смысла.

    Ещё три аналогичных проверки:

    • 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

    Предупреждение 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);
      ....
    }

    Диагностика рекомендательная, но здесь действительно лучше воспользоваться готовой константой для числа Pi из стандартной библиотеки.

    Заключение


    В последнее время проект развивается силами энтузиастов, но по-прежнему популярен и востребован в учебных программах. В интернете есть тысячи дополнений с разными космическими объектами. Celestia использовалась в фильме "The Day After Tomorrow" и документальном сериале "Through the Wormhole with Morgan Freeman".

    Мы рады, что проверяя многие проекты с открытым исходным кодом, мы не только популяризуем методологию статического анализа кода, но и вносим вклад в развитие мира открытых проектов. Вы, кстати, тоже можете использовать анализатор PVS-Studio не только для проверки своих собственных, но и сторонних проектов в качестве энтузиаста. Для этого можно воспользоваться одним из вариантов бесплатного лицензирования.

    Используйте статические анализаторы кода, делайте свои проекты надёжней и качественней!



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Celestia: Bugs' Adventures in Space.
    PVS-Studio
    533,77
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Похожие публикации

    Комментарии 11

      +7
      Однажды гулял в Celestia между галактиками и реально (хоть и не на долго) испугался, когда понял, что не помню как вернуться в родной Млечный путь
        0
        Серьезная заявка на навигатора! Я от Денеба смог вернуться с пятого раза.
        +1
        V624 The constant 3.14159265 is being utilized. The resulting value could be inaccurate. Consider using the M_PI constant from <math.h>


        Вот только в стандарте С и С++ нет такой константы.
        stackoverflow.com/questions/1727881/how-to-use-the-pi-constant-in-c

        Есть много нестандартных расширений в разных компиляторах, но я бы не стал рекомендовать это вот так, без кучи оговорок.
          +2
          Вообще совет использования math.h для С++ выглядит странно. Есть же cmath!
            +1
            Ну я бы сказал что
            #include <cmath>
            и
            M_PI
            всё равно лучше чем вручную вбивать значение. Рано или поздно ошибётесь
              –1
              Это точно лучше, если M_PI будет по стандарту в этом заголовочном файле.
              Дело то в том, что никто не гарантирует, что на вашей платформе с вашим компилятором эта константа есть.
              Может быть, нужно хитрый дефайн определить, может быть вообще нету ее там.
              Так что давать такие советы как V624 нужно очень осторожно.
            +1
            К предупреждению 1:
            Правильный вариант, скорее всего, такой:
            if (a.nAttributes < b.nAttributes)
              return true;
            if (b.nAttributes > a.nAttributes)
              return false;

            Всё же правильный вариант, скорее всего, такой:
            if (a.nAttributes < b.nAttributes)
              return true;
            if (b.nAttributes < a.nAttributes)
              return false;

            А на первый предложенный вариант анализатор тоже должен бы сработать. Попробуйте проверить.
              +1
              У Вас закешировалась страница. Эта опечатка была исправлена в 9:33.
                0
                Про кэш — возможно, обновлял только комментарии, а не страницу целиком.
                Но вопрос про анализатор: среагирует ли на такую опечатку, определив return false как unreachable, либо каким-нибудь иным образом?
                  +1
                  Да, это достаточно простой случай для детектирования.
              0
              del

              Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

              Самое читаемое