Плохой код пакета для создания 2D-анимаций Toonz

    На днях стало известно о том, что Digital Video, создатели проекта TOONZ, и японский издатель DWANGO подписали соглашение о приобретении компанией DWANGO проекта Toonz, программного обеспечения для создания 2D анимации.

    По условиям соглашения, подписанного между сторонами, будет открыт общий доступ к OpenToonz, проекту, разработанному компанией Toonz. Он так же будет включать некоторые элементы, разработанные Studio Ghibli, которые в свою очередь являются активными пользователями этих программ. С их помощью, например, Studio Ghibli создавали «Ходячий замок Хоула», «Унесенных призраками», «Рыбку Поньо», а также множество других картин. В их числе так же мультфильм «Футурама», который вдохновил меня на написание этой разоблачающей статьи про исходный код OpenToonz.

    Введение


    OpenToonz — это программное обеспечение для создания 2D анимации. Основой является проект «Toonz», который разработала итальянская компания Digital Video. Адаптировав эту программу, Studio Ghibli успешно использует ее уже много лет. Кроме мультипликационных картин проект также был задействован и в компьютерных играх — Discworld 2 и Claw.

    Стоит отметить, что цена пакета до настоящего момента составляла $10000, но качество кода оставляет желать лучшего. Этот проект — находка для любого статического анализатора. Размер исходного кода OpenToonz составляет примерно 1/10 ядра FreeBSD, в котором было найдено более 40 серьёзных ошибок с помощью анализатора PVS-Studio, но здесь их имеется гораздо больше!

    OpenToonz проверялся в Visual Studio 2013 с помощью статического анализатора PVS-Studio версии 6.03, который активно развивается и поддерживает языки C/C++/C#, и различные сборочные системы. Ещё во время компиляции проекта я насторожился, когда количество предупреждений компилятора начало быстро расти. В конце сборки их количество составило 1211 штук! Т.е. контролю за качеством исходного кода почти не уделяли внимание. Более того, некоторые предупреждения компилятора отключались с помощью директивы #pragma warning, но даже тут были допущены ошибки, про которые я расскажу ниже. Эта статья отличается тем, что в проекте найдено много самых настоящих ошибок, которые обычно допускают новички, только начинающие изучать C/C++. Описание найденных предупреждений анализатора я начну с ошибок использования памяти и указателей.

    Неправильная работа с памятью




    V611 The memory was allocated using 'new' operator but was released using the 'free' function. Consider inspecting operation logics behind the 'row' variable. motionblurfx.cpp 288
    template <class T>
    void doDirectionalBlur(....)
    {
      T *row, *buffer;
      ....
      row = new T[lx + 2 * brad + 2]; // <=
      if (!row)
        return;
      memset(row, 0, (lx + 2 * brad + 2) * sizeof(T));
      ....
      free(row);                      // <=
      r->unlock();
    }

    Здесь анализатор обнаружил, что динамическая память выделяется и освобождается несовместимыми между собой способами. После вызова оператора new [] необходимо освобождать память с помощью оператора delete []. Именно с двумя квадратными скобками! Я акцентирую внимание на этом не просто так — смотрите следующий пример.

    V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] uPrime;'. tstroke.cpp 3353
    double *reparameterize3D(....)
    {
      double *uPrime = new double[size]; // <=
    
      for (int i = 0; i < size; i++) {
        uPrime[i] = NewtonRaphsonRootFind3D(....);
        if (!_finite(uPrime[i])) {
          delete uPrime;                 // <=
          return 0;
        }
      }
      ....
    }

    В языке C++ операторы new/delete и new[]/delete[] используются в паре друг с другом. Использование разных операторов для выделения и освобождения динамической памяти является ошибкой. В приведённом выше коде память, занимаемая массивом uPrime, не будет корректна освобождена.

    К сожалению, такое место не единственное. Ещё 20 мест я выписал в файл OpenToonz_V611.txt.

    V554 Incorrect use of auto_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. screensavermaker.cpp 29
    void makeScreenSaver(....)
    {
      ....
      std::auto_ptr<char> swf(new char[swfSize]);
      ....
    }

    Перед нами — альтернативный вариант рассмотренной ошибки, но здесь оператор delete «спрятан» внутрь умного указателя std::auto_ptr. Это точно так же приводит к неопределенному поведению программы.

    Чтобы исправить ошибку, необходимо указать, что должен использоваться оператор delete [].

    Корректный вариант кода:
    std::unique_ptr<char[]> swf(new char[swfSize]);
    

    V599 The destructor was not declared as a virtual one, although the 'TTileSet' class contains virtual functions. cellselection.cpp 891
    void redo() const
    {
      insertLevelAndFrameIfNeeded();
      TTileSet *tiles;  // <=
      bool isLevelCreated;
      pasteRasterImageInCellWithoutUndo(...., &tiles, ....);
      delete tiles;     // <=
      TApp::instance()->getCurrentXsheet()->notifyXsheetChanged();
    }

    Теперь поговорим об утечках памяти и неполном разрушении объектов. В этом примере не полностью будут разрушаться объекты, унаследованные от класса TTileSet.

    Описание класса TTileSet:
    class DVAPI TTileSet
    {
      ....
    protected:
      TDimension m_srcImageSize;
    
      typedef std::vector<Tile *> Tiles;
      Tiles m_tiles;
    
    public:
      TTileSet(const TDimension &dim) : m_srcImageSize(dim)
      {
      }
      ~TTileSet();      // <=
      ....
      virtual void add(const TRasterP &ras, TRect rect) = 0;
      ....
      virtual TTileSet *clone() const = 0;
    };

    Класс содержит чистые виртуальные функции и является абстрактным. Создавать объекты этого класса нельзя, поэтому он используется только производными классами. Таким образом, из-за отсутствия виртуального деструктора у TTileSet (деструктор есть, но он не помечен как виртуальный) все производные классы не будут очищаться полностью.

    В исходном коде OpenToonz я нашёл несколько классов, которые наследуются от TTileSet:
    class DVAPI TTileSetCM32 : public TTileSet
    class DVAPI TTileSetCM32 : public TTileSet
    class DVAPI TTileSetFullColor : public TTileSet
    class DVAPI Tile : public TTileSet::Tile

    Каждый объект этих классов (или унаследованных от них классов) будет разрушаться не полностью. Формально это приведёт к неопределённому поведению программы; на практике это, скорее всего, приведёт к утечке памяти и иных ресурсов.

    Разработчикам необходимо проверить ещё два похожих места:
    • V599 The virtual destructor is not present, although the 'MessageParser' class contains virtual functions. tipcsrv.cpp 91
    • V599 The virtual destructor is not present, although the 'ColumnToCurveMapper' class contains virtual functions. functionselection.cpp 278

    Опасное использование указателей




    V503 This is a nonsensical comparison: pointer < 0. styleselection.cpp 104
    bool pasteStylesDataWithoutUndo(....)
    {
      ....
      if (palette->getStylePage(styleId) < 0) { // <=
        // styleId non e' utilizzato: uso quello
        // (cut/paste utilizzato per spostare stili)
        palette->setStyle(styleId, style);
      } else {
        // styleId e' gia' utilizzato. ne devo prendere un altro
        styleId = palette->getFirstUnpagedStyle();
        if (styleId >= 0)
          palette->setStyle(styleId, style);
        else
          styleId = palette->addStyle(style);
      }
      ....
    }

    Функция getStylePage() возвращает указатель на некую страницу: TPalette::Page*. Такое сравнение с нулём не имеет смысла. Сделав поиск использования функции getStylePage(), я обнаружил, что во всех остальных случаях, результат этой функции проверяется только на равенство и неравенство нулю, а в этом месте допустили ошибку.

    V522 Dereferencing of the null pointer 'region' might take place. Check the logical condition. palettecmd.cpp 102
    bool isStyleUsed(const TVectorImageP vi, int styleId)
    {
      ....
      TRegion *region = vi->getRegion(i);
      if (region || region->getStyle() != styleId)
        return true;
      ....
    }

    Скорее всего, в этом фрагменте кода перепутали операторы '&&' и '||'. Иначе, если указатель region будет нулевым, то выполнится его разыменование.

    V614 Potentially uninitialized pointer 'socket' used. Consider checking the first actual argument of the 'connect' function. tmsgcore.cpp 36
    void TMsgCore::OnNewConnection() //server side
    {
      QTcpSocket *socket;
      if (m_tcpServer)                                 // <=
        socket = m_tcpServer->nextPendingConnection(); // <=
      assert(socket);
    
      bool ret = connect(socket, ....);                // <=
      ret = ret && connect(socket, ....);              // <=
      assert(ret);
      m_sockets.insert(socket);
    }

    Анализатор обнаружил потенциальное использование неинициализированного указателя socket. Если переменная m_tcpServer будет равна false, то указатель не будет инициализирован. В таком неинициализированном виде он может быть передан в функцию connect().

    V595 The 'batchesTask' pointer was utilized before it was verified against nullptr. Check lines: 1064, 1066. batches.cpp 1064
    void BatchesController::update()
    {
      ....
      TFarmTask *batchesTask = getTask(batchesTaskId);   // <=
      TFarmTask farmTask = *batchesTask;                 // <=
    
      if (batchesTask) {                                 // <=
        QString batchesTaskParentId = batchesTask->m_parentId;
        m_controller->queryTaskInfo(farmTaskId, farmTask);
        int chunkSize = batchesTask->m_chunkSize;
        *batchesTask = farmTask;
        batchesTask->m_chunkSize = chunkSize;
        batchesTask->m_id = batchesTaskId;
        batchesTask->m_parentId = batchesTaskParentId;
      }
      ....
    }

    В коде присутствует много мест, где потенциально может произойти разыменование нулевого указателя. Обычно соответствующая проверка присутствует, но одно или несколько мест использования указателя всё равно остаются небезопасными. Например, в этом фрагменте кода есть проверка batchesTask, но до проверки уже будет выполнено одно разыменование указателя.

    Ещё 29 похожих мест я выписал в файле OpenToonz_V595.txt.

    Ошибки при работе со строками




    V530 The return value of function 'toUpper' is required to be utilized. sceneviewerevents.cpp 847
    void SceneViewer::keyPressEvent(QKeyEvent *event)
    {
      ....
      QString text = event->text();
      if ((event->modifiers() & Qt::ShiftModifier))
        text.toUpper();
      ....
    }

    Метод toUpper() не изменяет строку 'text'. В документации он описан как QString QString::toUpper() const, т.е. является константным методом.

    Корректный вариант кода:
    QString text = event->text();
      if ((event->modifiers() & Qt::ShiftModifier))
        text = text.toUpper();

    В коде присутствуют ещё три функции, возвращаемое значение которых не используется. Все эти места требуют исправления:
    • V530 The return value of function 'left' is required to be utilized. tfarmserver.cpp 569
    • V530 The return value of function 'ftell' is required to be utilized. tiio_bmp.cpp 804
    • V530 The return value of function 'accumulate' is required to be utilized. bendertool.cpp 374

    V614 Uninitialized iterator 'it1' used. fxcommand.cpp 2096
    QString DeleteLinksUndo::getHistoryString()
    {
      ....
      std::list<TFxP>::const_iterator it1; // <=
      std::list<TFx *>::const_iterator ft;
      for (ft = m_terminalFxs.begin(); ft != ....end(); ++ft) {
        if (ft != m_terminalFxs.begin())
          str += QString(",  ");
        str += QString("%1- -Xsheet")
              .arg(QString::fromStdWString((*it1)->getName())); // <=
      }
      ....
    }

    В операциях со строками используется неинициализированный итератор it1. Скорее всего, в одном месте его забыли заменить на итератор ft.

    V642 Saving the '_wcsicmp' function result inside the 'char' type variable is inappropriate. The significant bits could be lost breaking the program's logic. tfilepath.cpp 328
    bool TFilePath::operator<(const TFilePath &fp) const
    {
      ....
      char differ;
      differ = _wcsicmp(iName.c_str(), jName.c_str());
      if (differ != 0)
        return differ < 0 ? true : false;
      ....
    }

    Функция _wcsicmp возвращает следующие значения типа int:
    • < 0 — string1 less then string2;
    • 0 — string1 identical to string2;
    • > 0 — string1 greater than string2.

    Обратите внимание. '> 0'означает любые числа, а вовсе не 1. Этими числами могут быть: 2, 3, 100, 256, 1024, 5555, и так далее. Результат функции _wcsicmp может не поместиться в переменную типа char, из-за чего оператор сравнения вернёт неожиданный результат.

    V643 Unusual pointer arithmetic: "\\" + v[i]. The value of the 'char' type is being added to the string pointer. tstream.cpp 31
    string escape(string v)
    {
      int i = 0;
      for (;;) {
        i = v.find_first_of("\\\'\"", i);
        if (i == (int)string::npos)
          break;
        string h = "\\" + v[i]; // <=
        v.insert(i, "\\");
        i = i + 2;
      }
      return v;
    }

    Анализатор обнаружил ошибку, связанную с прибавлением к строковому литералу символьной константы. Ожидалось, что к строке прибавится символ, но к указателю на строку прибавляется числовое значение, что приводит к выходу за границу строкового литерала и получению неожиданного результата.

    Чтобы было понятней, вот чему эквивалентен этот код:
    const char *p1 = "\\";
    const int delta = v[i];
    const char *p2 = *p1 + delta;
    string h = p2;

    Корректный вариант кода:
    string h = string("\\") + v[i];

    V655 The strings were concatenated but are not utilized. Consider inspecting the 'alias + "]"' expression. plasticdeformerfx.cpp 150
    string PlasticDeformerFx::getAlias(....) const
    {
      std::string alias(getFxType());
      alias += "[";
      ....
      if (sd)
        alias += ", "+toString(sd, meshColumnObj->paramsTime(frame));
    
      alias + "]"; // <=
    
      return alias;
    }

    Анализатор обнаружил выражение, результат которого не используется. Скорее всего, в этой функции случайно написали оператор '+', вместо '+='. В результате этого к строке alias не прибавляется квадратная скобка в конце, как планировал программист.

    Неправильные исключения




    V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw domain_error(FOO); pluginhost.cpp 1486
    void Loader::doLoad(const QString &file)
    {
      ....
      int ret = pi->ini_(host);
      if (ret) {
        delete host;
        std::domain_error("failed initialized: error on ....");
      }
      ....
    }

    В функции случайно забыто ключевое слово throw. Последствие — данный код не генерирует исключение в случае ошибочной ситуации. Исправленный вариант кода:
    throw std::domain_error("failed initialized: error on ....");

    V746 Type slicing. An exception should be caught by reference rather than by value. iocommand.cpp 1620
    bool IoCmd::saveLevel(....)
    {
      ....
      try {
        sl->save(fp, TFilePath(), overwritePalette);
      } catch (TSystemException se) { // <=
        QApplication::restoreOverrideCursor();
        MsgBox(WARNING, QString::fromStdWString(se.getMessage()));
        return false;
      } catch (...) {
        ....
      }
      ....
    }

    Анализатор обнаружил потенциальную ошибку, связанную с перехватом исключения по значению. Это значит, что с помощью конструктора копирования будет сконструирован новый объект se типа TSystemException. При этом будет потеряна часть информации об исключении, которая хранилась в классах, унаследованных от TSystemException.

    Аналогичные подозрительные места:
    • V746 Type slicing. An exception should be caught by reference rather than by value. iocommand.cpp 2650
    • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 522
    • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 537
    • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 635
    • V746 Type slicing. An exception should be caught by reference rather than by value. tlevel_io.cpp 130
    • V746 Type slicing. An exception should be caught by reference rather than by value. calligraph.cpp 161
    • V746 Type slicing. An exception should be caught by reference rather than by value. calligraph.cpp 165
    • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 210
    • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 214
    • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 218
    • V746 Type slicing. An exception should be caught by reference rather than by value. scriptbinding_level.cpp 221

    Неверные условия




    V547 Expression '(int) startOutPoints.size() % 2 != 2' is always true. rasterselection.cpp 852
    TStroke getIntersectedStroke(TStroke &stroke, TRectD bbox)
    {
      ....
      for (t = 0; t < (int)outPoints.size(); t++)
        addPointToVector(...., (int)startOutPoints.size() % 2 != 2);
      ....
    }

    Интересная ошибка. Скорее всего, здесь планировали определить, является ли значение size() чётным или нечётным. Поэтому остаток от деления на 2 должен сравниваться с нулём.

    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. igs_motion_wind_pixel.cpp 127
    void rgb_to_lightness_(
      const double re, const double gr, const double bl, double &li)
    {
      li=((re < gr) ? ((gr < bl) ? bl : gr) : ((re < bl) ? bl : re) +
                                (gr < re)
                              ? ((bl < gr) ? bl : gr)
                              : ((bl < re) ? bl : re)) / 2.0;
    }

    В этом фрагменте кода допустили ошибку, связанную с приоритетом тернарного оператор ':?'. Его приоритет ниже, чем у оператора сложения. В результате чего, если условие (re < gr) ложно, то дальнейшие вычисления выполняются неправильно: вещественные переменные начинают складываться с логическими.

    Никогда не используйте совместно несколько тернарных операторов — это кратчайший путь добавить ошибку в код.

    V590 Consider inspecting the 'state == (- 3) || state != 0' expression. The expression is excessive or contains a misprint. psdutils.cpp 174
    int psdUnzipWithoutPrediction(....)
    {
      ....
      do {
        state = inflate(&stream, Z_PARTIAL_FLUSH);
        if (state == Z_STREAM_END)
          break;
        if (state == Z_DATA_ERROR || state != Z_OK) // <=
          break;
      } while (stream.avail_out > 0);
      ....
    }

    Условие, которое помечено стрелкой, не зависит от результата подвыражения «state == Z_DATA_ERROR». В этом легко убедиться, если построить таблицу истинности всего условного выражения.

    Copy-paste программирование




    V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 1448, 1454. tcenterlineskeletonizer.cpp 1448
    inline void Event::processVertexEvent()
    {
      ....
      if (newLeftNode->m_concave) {        // <=
        newLeftNode->m_notOpposites = m_generator->m_notOpposites;
        append<vector<ContourEdge *>, vector<ContourEdge *>::....
    
        newLeftNode->m_notOpposites.push_back(newRightNode->m_edge);
        newLeftNode->m_notOpposites.push_back(newRightNode->....);
      } else if (newLeftNode->m_concave) { // <=
        newRightNode->m_notOpposites = m_generator->m_notOpposites;
        append<vector<ContourEdge *>, vector<ContourEdge *>::....
    
        newRightNode->m_notOpposites.push_back(newLeftNode->m_edge);
        newRightNode->m_notOpposites.push_back(newLeftNode->....);
      }
      ....
    }

    Здесь перепутаны переменные newLeftNode и newRightNode в условиях. В результате такой ошибки ветвь else никогда не выполняется. Скорее всего, одно из условий должно быть таким: if (newRightNode->m_concave).

    V501 There are identical sub-expressions to the left and to the right of the '||' operator: m_cutLx || m_cutLx canvassizepopup.cpp 271
    bool m_cutLx, m_cutLy;
    
    void PeggingWidget::on00()
    {
     ....
     m_11->setIcon(...).rotate(m_cutLx || m_cutLx ? -90 : 90),....));
     ....
    }

    В коде присутствуют две логические переменные: m_cutLx и m_cutLy, которые отличаются всего на одну последнюю букву. В приведённом примере используют одну и ту же переменную m_cutLx. Скорее всего, в одной из них допустили опечатку.

    V501 There are identical sub-expressions 'parentTask->m_status == Aborted' to the left and to the right of the '||' operator. tfarmcontroller.cpp 1857
    void FarmController::taskSubmissionError(....)
    {
      ....
      if (parentTask->m_status == Aborted || // <=
          parentTask->m_status == Aborted) { // <=
          parentTask->m_completionDate = task->m_completionDate;
          if (parentTask->m_toBeDeleted)
            m_tasks.erase(itParent);
      }
      ....
    }

    Анализатор обнаружил два одинаковых сравнения с константой Aborted. Выполнив поиск по файлу, в строке 2028 этого файла я обнаружил идентичный блок кода, но с таким условием:
    if (parentTask->m_status == Completed ||
        parentTask->m_status == Aborted) {

    Скорее всего, в этом месте условное выражение должно быть аналогичным.

    V501 There are identical sub-expressions 'cornerCoords.y > upperBound' to the left and to the right of the '||' operator. tellipticbrush.cpp 1020
    template <typename T>
    void tellipticbrush::OutlineBuilder::addMiterSideCaps(....)
    {
      ....
      if (cornerCoords == TConsts::napd ||
        cornerCoords.x < lowerBound || cornerCoords.y > upperBound ||
        cornerCoords.y < lowerBound || cornerCoords.y > upperBound) {
        ....
      }
      ....
    }

    Тут программист допустил маленькую опечатку, использовав y вместо x.

    Ещё шесть ошибок, допущенных в следствие copy-paste программирования, я не буду описывать, а приведу списком. Эти места надо обязательно проверить разработчикам:
    • V501 There are identical sub-expressions 's.m_repoStatus == «modified»' to the left and to the right of the '||' operator. svnupdatedialog.cpp 210
    • V501 There are identical sub-expressions 'm_lineEdit->hasFocus()' to the left and to the right of the '||' operator. framenavigator.cpp 44
    • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 750, 825. tpalette.cpp 750
    • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 123, 126. igs_density.cpp 123
    • V523 The 'then' statement is equivalent to the 'else' statement. typetool.cpp 813
    • V583 The '?:' operator, regardless of its conditional expression, always returns one and the same value: Comma. tgrammar.cpp 731

    Разные ошибки




    V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 20, 205. tspectrum.h 205
    #ifdef WIN32
    #pragma warning(disable : 4251)
    #endif
    ....
    #ifdef WIN32
    #pragma warning(default : 4251)
    #endif

    Вот так отключают предупреждения компилятор, на которые всё-таки обратили внимание в этом проекте. Ошибка заключается в том, что директива #pragma warning(default: X) не включает предупреждение, а устанавливает его в состояние ПО УМОЛЧАНИЮ, которое может быть не таким, как ожидает программист.

    Исправленный вариант кода:
    #ifdef WIN32
    #pragma warning(push)
    #pragma warning(disable : 4251)
    #endif
    ....
    #ifdef WIN32
    #pragma warning(pop)
    #endif

    V546 Member of a class is initialized by itself: 'm_subId(m_subId)'. tfarmcontroller.cpp 572
    class TaskId
    {
      int m_id;
      int m_subId;
    
    public:
      TaskId(int id, int subId = -1) : m_id(id), m_subId(m_subId){};

    Интересная ошибка в списке инициализации класса. Поле m_subld инициализируется само собой, хотя, скорее всего, хотели написать m_subId(subId).

    V557 Array overrun is possible. The '9' index is pointing beyond array bound. tconvolve.cpp 123
    template <class PIXOUT>
    void doConvolve_cm32_row_9_i(....)
    {
      TPixel32 val[9];                                  // <=
      ....
      for (int i = 0; i < 9; ++i) {                     // <= OK
        ....
        else if (tone == 0)
          val[i] = inks[ink];
        else
          val[i] = blend(....);
      }
    
      pixout->r = (typename PIXOUT::Channel)((
        val[1].r * w1 + val[2].r * w2 + val[3].r * w3 +
        val[4].r * w4 + val[5].r * w5 + val[6].r * w6 +
        val[7].r * w7 + val[8].r * w8 + val[9].r * w9 + // <= ERR
        (1 << 15)) >> 16);
      pixout->g = (typename PIXOUT::Channel)((
        val[1].g * w1 + val[2].g * w2 + val[3].g * w3 +
        val[4].g * w4 + val[5].g * w5 + val[6].g * w6 +
        val[7].g * w7 + val[8].g * w8 + val[9].g * w9 + // <= ERR
        (1 << 15)) >> 16);
      pixout->b = (typename PIXOUT::Channel)((
        val[1].b * w1 + val[2].b * w2 + val[3].b * w3 +
        val[4].b * w4 + val[5].b * w5 + val[6].b * w6 +
        val[7].b * w7 + val[8].b * w8 + val[9].b * w9 + // <= ERR
        (1 << 15)) >> 16);
      pixout->m = (typename PIXOUT::Channel)((
        val[1].m * w1 + val[2].m * w2 + val[3].m * w3 +
        val[4].m * w4 + val[5].m * w5 + val[6].m * w6 +
        val[7].m * w7 + val[8].m * w8 + val[9].m * w9 + // <= ERR
        (1 << 15)) >> 16);
      ....
    }

    Большой фрагмент кода, в котором кто-то обращается к массиву val, состоящем из 9 элементов, по индексу от 1 до 9. Хотя рядом присутствует цикл, в котором выполняется правильное обращение к массиву по индексу от 0 до 8.

    V556 The values of different enum types are compared: m_action != EDIT_SEGMENT. Types: Action, CursorType. controlpointeditortool.cpp 257
    enum Action { NONE,
                  RECT_SELECTION,
                  CP_MOVEMENT,
                  SEGMENT_MOVEMENT,
                  IN_SPEED_MOVEMENT,
                  OUT_SPEED_MOVEMENT };
    
    enum CursorType { NORMAL,
                      ADD,
                      EDIT_SPEED,
                      EDIT_SEGMENT,
                      NO_ACTIVE };
    
    void ControlPointEditorTool::drawMovingSegment()
    {
      int beforeIndex = m_moveSegmentLimitation.first;
      int nextIndex = m_moveSegmentLimitation.second;
      if (m_action != EDIT_SEGMENT || // <=
          beforeIndex == -1 ||
          nextIndex == -1 ||
          !m_moveControlPointEditorStroke.getStroke())
        return;
      ....
    }

    Анализатор обнаружил сравнение enum значений, имеющие разные типы. С помощью поиска по коду я также обнаружил, что поле класса m_action инициализируется правильным типом, а в этом месте сравнивается с константой другого типа.

    Заключение




    Как я уже упомянул, проект OpenToonz — находка для любого статического анализатора кода: в нем очень много серьёзных ошибок для такого небольшого проекта. В статью не просто не вошли все сообщения анализатора, в неё не вошли даже многие интересные и серьёзные предупреждения из-за их большого количества. Конечно же, я отпишу о проверке на форум разработчиков. Возможно, им будет интересно повышение качества кода.

    Намерение открыть код своего программного продукта Universal Scene Description (USD) заявляла и компания Pixar. С нетерпением буду ждать этого события.

    Предлагаю всем желающим попробовать PVS-Studio на своих проектах C/C++/C#. Анализатор работает в среде Windows и поддерживает различные сборочные системы.


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. Toonz code leaves much to be desired.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio, версия 2015. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

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

      +5
      Картинки шикарны!
        +3
        Вы же из одной команды :)
        Неужели между собой статьи для блога до публикации не обсуждаете, включая и картинки?
        Или это КДПВ такой?
          0
          Мне жаль, если коллеги никогда не восхищались Вашей работой.
            +2
            Я не про сам факт восхищения, а про то, что он сделан публично. Восхищение — это прекрасно, особенно, если оно заслуженно (а картинки и вправду шикарны). Но в комментариях под статьёй в блоге смотрится как-то странно, что ли. От коллеги, а не от постороннего человека.
            У меня потому и возник вопрос — это специальное публичное восхищение или просто не было обсуждения статьи и, соответственно, возможности лично высказать всю гамму чувств?
            Не хочу обидеть, простое любопытство. Всегда было интересно, как у разных компаний пишутся статьи для блога на Хабре.
        +7
        Поразительно, как такой код вообще мог работать и не выстреливать через раз!
          +3
          Так toonz падает постоянно. Все аниматоры приучены сохранять проект после каждого нарисованного кадра.
            0
            Приведу пример про ошибки с непарностью new / delete с [] и без: «неопределённое поведение» в стандарте объявлено всегда, а на практике компиляторы (я даже подозреваю, _все_ компиляторы) оптимизируют код для типов без деструкторов так, что разницы нет. Т.е. в данном случае мы имеем ситуацию «неопределённое поведение в реальном мире: всё работает без проблем», не знаю уж, к счастью или к несчастью. Нехорошая ошибка в коде есть. Теоретически (и практически, если удастся найти компилятор, который не делает вышеобозначенную оптимизацию) всё плохо. Всё становится действительно плохо, когда кому-то захочется отмодифицировать код, заменив тип элемента без деструктора на тип с деструктором. В этот момент этот программист и наступит на заботливо разложенные грабли (и, вероятно, разберётся и пофиксает). Но, пока этого не произошло, «все работает ЗАМЕЧАТЕЛЬНО!» :(.
            0
            Не понятно зачем использовать std::string/QString.
              +6
              Там наверняка еще какой-нибудь MyString есть, with blackjack and hookers ^^
                +1
                QString очень удобная штука, в разы удобней std::string. В Qt'шном коде многие его используют, в том числе и я.
                А std::string нужен для совместимости с другими библиотеками, либо для функций из std.
                  0
                  +1. Отсутствие полноценной поддержки Юникода в стандартной библиотеке C++ делает использование QString очень целесообразным в Qt-проектах.
                  0
                  В Qt проектах нет смысла использовать std::string.
                    0
                    Более того, к 6 версии готовится QStringView! Кто следит за развитием стандарта сразу понял что это, практически аналог готовящегося std::string_view, но только с поддержкой QString, QStringRef и иже с ними(например QLatin1String и compile-time вариант макрос QStringLiteral, да, их много, но они все нужны сейчас)…
                    https://codereview.qt-project.org/#/c/143763/

                    Если есть народ который хорошо разбирается в этом подключайтесь к ревью.
                      0
                      Да и я о том же.
                    +1
                    >Плохой код
                    В Футураме это было не заметно:)
                      +4
                      Мы же не знаем, что чувствовали художники, когда работали с программой :)

                      Вот в VS 2015 есть подтверждённые баги в дизайнере расширений для MS Word. С ним в принципе невозможно работать, даже иногда сохранение исходных файлов блокируется. Тем не менее, надо сделать, чтобы в готовом плагине было незаметно.
                        +1
                        Конечно, если программа такая забагованная, то сколько труда стоило им сделать ленту нормально.
                        Здесь, наверно, должна быть картинка с вылетающим из окна монитором :D
                        +1
                        Мы не знаем сколько слез пролили аниматоры ;)
                          0
                          Нисколько. Если бы плохо работало — купили бы другую программу. $10к для «20th Century Fox» — не очень большая сумма, в сравнении со срывом сроков выпуска продукта из-за ошибок в программе анимации.
                        0
                        А из какой серии футурамы картинка с зеркалом заднего вида?
                          0
                          Похоже, сами нарисовали
                            0
                            Интересно, в Toonz ли?

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

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