# OpenToonz Прошло уже почти четыре года с тех пор, как PVS\-Studio поверял исходный код OpenToonz\. Этот проект является очень мощным инструментом для создания двухмерной анимации\. За время с последней проверки с его помощью были созданы такие мультипликационные произведения, как Мэри и Ведьмин цветок, Бэтмэн\-Нинзя, Промаре и другие\. Раз большие студии продолжают пользоваться Toonz, то почему бы не проверить качество исходного кода еще раз? ![0720_OpenToonz_ru/image1.png](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image1.png) С предыдущим разбором ошибок можно познакомиться в статье "[Плохой код пакета для создания 2D\-анимаций Toonz](https://pvs-studio.ru/ru/blog/posts/cpp/0389/)"\. В целом не похоже, что качество кода сильно повысилось, так что общее впечатление лучше не стало\. К тому же, было обнаружено множество тех же ошибок, что и в предыдущей статье\. Повторно мы их рассматривать не будем, благо выбрать новые есть из чего\. Впрочем, надо понимать, что наличие ошибок вовсе не обязательно мешает активно и продуктивно использовать программный продукт\. Скорее всего, найденные ошибки живут в редко используемых или вовсе в неиспользуемых участках кода, иначе они были бы выявлены в процессе использования приложения и исправлены\. Однако, это не значит, что статический анализ избыточен\. Просто смысл статического анализа не в поиске старых и неактуальных ошибок, а в удешевлении процесса разработки\. Многие ошибки при написании кода могут быть выявлены сразу, а не в процессе эксплуатации ПО\. Соответственно, при регулярном использовании статического анализатора, ошибки исправляются на самом раннем этапе\. Это экономит и время разработчиков, и деньги компании, и улучшает восприятие продукта пользователями\. Согласитесь, неприятно постоянно отписывать разработчикам, что не работает то одно, то другое\. ![0720_OpenToonz_ru/image2.png](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image2.png) **Фрагмент N1** [V610](https://pvs-studio.ru/ru/docs/warnings/v610/) Undefined behavior\. Check the shift operator '<<'\. The left operand '\(\- 1\)' is negative\. ```cpp decode_mcu_AC_refine (j_decompress_ptr cinfo, JBLOCKROW *MCU_data) { int p1, m1; p1 = 1 << cinfo->Al; m1 = (-1) << cinfo->Al; .... } ``` Не совсем ясно, чего автор кода хотел добиться здесь\. Использование операторов сдвига с отрицательными числами приводит к неопределенному поведению\. То, как поведение операторов сдвига описывается в стандарте, выглядит на первый взгляд немного запутанным, но все\-таки сверимся с ним: 1\. The type of the result is that of the promoted left operand\. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand\. 2\. The value of E1 << E2 is E1 left\-shifted E2 bit positions; vacated bits are zero\-filled\. If E1 has an unsigned type, the value of the result is E1 \* 2^E2, reduced modulo one more than the maximum value representable in the result type\. Otherwise, if E1 has a signed type and non\-negative value, and E1\*2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined\. Итак, поведение не определено, если правый или левый операнд имеет отрицательное значение\. Если операнд имеет знаковый тип, неотрицательное значение и вмещается в результирующий тип, то поведение будет нормальным\. **Фрагмент N2** [V517](https://pvs-studio.ru/ru/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 156, 160\. cameracapturelevelcontrol\.cpp 156 ```cpp void CameraCaptureLevelHistogram::mousePressEvent(QMouseEvent* event) { if (event->button() != Qt::LeftButton) return; if (m_currentItem == Histogram) { m_histogramCue = true; return; } if (m_currentItem == None) return; QPoint pos = event->pos(); if (m_currentItem == BlackSlider) // <= m_offset = pos.x() - SIDE_MARGIN - m_black; else if (m_currentItem == GammaSlider) m_offset = pos.x() - SIDE_MARGIN - gammaToHPos(m_gamma, m_black, m_white); else if (m_currentItem == BlackSlider) // <= m_offset = pos.x() - SIDE_MARGIN - m_white; else if (m_currentItem == ThresholdSlider) m_offset = pos.x() - SIDE_MARGIN - m_threshold; } ``` Тут переменной _m\_offset_ присваиваются различные значения в зависимости от значения _m\_currentItem_\. Однако дублирование проверки на _BlackSlider_ бессмысленно, и из тела условия можно увидеть, что в вычислении участвует переменная _m\_white_\. Заглянем в возможные значения для _m\_currentItem_\. ```cpp LevelControlItem m_currentItem; enum LevelControlItem { None = 0, BlackSlider, WhiteSlider, GammaSlider, ThresholdSlider, Histogram, NumItems }; ``` ![0720_OpenToonz_ru/image3.png](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image3.png) Оказывается, что возможно еще и значение _WhiteSlider_, на которое проверка как раз и не производится\. Таким образом, вполне возможно, что какой\-то из сценариев поведения был потерян из\-за ошибки копипасты\. **Фрагмент N3** [V517](https://pvs-studio.ru/ru/docs/warnings/v517/) The use of 'if \(A\) \{\.\.\.\} else if \(A\) \{\.\.\.\}' pattern was detected\. There is a probability of logical error presence\. Check lines: 784, 867\. tpalette\.cpp 784 ```cpp void TPalette::loadData(TIStream &is) { .... std::string tagName; while (is.openChild(tagName)) { if (tagName == "version") { .... } else if (tagName == "stylepages") { // <= while (!is.eos()) { if (....){ { .... } .... is.closeChild(); } } else if (tagName == "refImgPath") { .... } else if (tagName == "animation") { .... } else if (tagName == "stylepages") { // <= int key = '0'; while (!is.eos()) { int styleId = 0; .... } } .... } } ``` Еще одна похожая ошибка\. Здесь также у одинаковых условий разные тела, но сделать вывод о возможных вариантах значения _tagName_ уже нельзя\. Скорее всего, просто какой\-то вариант был упущен, и в итоге мы имеем код, который никогда не будет выполнен\. **Фрагмент N4** [V547](https://pvs-studio.ru/ru/docs/warnings/v547/) Expression 'chancount \=\= 2' is always true\. psd\.cpp 720 ```cpp void TPSDReader::readImageData(...., int chancount) { .... if (depth == 1 && chancount == 1) { // <= 1 .... } else if (depth == 8 && chancount > 1) { .... for (....) { if (chancount >= 3) { .... if (chancount == 4) .... else .... } else if (chancount <= 2) // <= 2 { .... if (chancount == 2) // <= 3 .... else .... } .... } .... } else if (m_headerInfo.depth == 8 && chancount == 1) { .... } ``` В эти проверки закралась небольшая логическая ошибка\. В проверке под номером один _chancount_ сравнивается с 1, а проверке номер 2 проверяется, что эта переменная меньше или равна 2\. В итоге, к условию под номером три единственным возможным значением _chancount_ является 2\. К неправильной работе программы такая ошибка может и не приведет, но она усложняет чтение и понимание кода\. Например, непонятно, зачем тогда else\-ветка\.\.\. В целом рассматриваемая в этом фрагменте функция занимает чуть больше 300 строк кода и состоит из таких вот нагромождений условий и циклов\. ![0720_OpenToonz_ru/image4.jpg](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image4.jpg) **Фрагмент N5** [V614](https://pvs-studio.ru/ru/docs/warnings/v614/) Uninitialized variable 'precSegmentIndex' used\. Consider checking the fifth actual argument of the 'insertBoxCorners' function\. rasterselection\.cpp 803 ```cpp TStroke getIntersectedStroke(TStroke &stroke, TRectD bbox) { .... int precSegmentIndex, currentSegmentIndex, startSegmentIndex, precChunkIndex = -1; .... if (....) { insertBoxCorners(bbox, points, outPoints, currentSegmentIndex, precSegmentIndex); .... } } void insertBoxCorners(...., int currentSegmentIndex, int precSegmentIndex) { .... bool sameIndex = (precSegmentIndex == currentSegmentIndex); .... int segmentIndex = precSegmentIndex; .... } ``` Возможно, здесь ошибка была допущена еще при инициализации переменных _precSegmentIndex_, _currentSegmentIndex_, _startSegmentIndex_, _precChunkIndex_\. Разработчик мог рассчитывать, что инициализация последнего элемента \-1 инициализирует тем же значением, что и другие переменные, объявленные в той же строке\. **Фрагмент N6** [V590](https://pvs-studio.ru/ru/docs/warnings/v590/) Consider inspecting the 's \!\= "" && s \=\= "color"' expression\. The expression is excessive or contains a misprint\. cleanupparameters\.cpp 416 ```cpp void CleanupParameters::loadData(TIStream &is, bool globalParams) { .... std::string s = is.getTagAttribute("sharpness"); .... if (....) { .... } else if (tagName = "lineProcessing") .... if (s != "" && isDouble(s)) .... if (s != "" && isDouble(s)) .... if (s != "" && s == "color") // <= .... } else if (tagName == "despeckling") { .... } .... } ``` Эта ошибка, даже скорее недочет, сама по себе приводит лишь к лишнему сравнению\. Однако если мы взглянем на код в целом, станет ясно, что лишнее сравнение появилось в результате копипасты из предыдущих условий\. ![0720_OpenToonz_ru/image5.png](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image5.png) Такая лапша, занимающая десятки и более строк кода, вполне может хранить в себе еще какие\-либо ошибки логики, и их поиск при таком форматировании может обернуться мучениями\. **Фрагмент N7** [V772](https://pvs-studio.ru/ru/docs/warnings/v772/) Calling a 'delete' operator for a void pointer will cause undefined behavior\. pluginhost\.cpp 1327 ```cpp static void release_interface(void *interf) { if (interf) delete interf; } ``` Тут само сообщение анализатора уже достаточно исчерпывающее: вызов оператора _delete_ для указателя на тип _void_ приводит к неопределенному поведению\. Если была необходима универсальная функция для удаления интерфейсов, возможно её стоило сделать шаблонной\. ```cpp template static void release_interface(T *interf) { if (interf) delete interf; } ``` **Фрагмент N8** [V568](https://pvs-studio.ru/ru/docs/warnings/v568/) It's odd that 'sizeof\(\)' operator evaluates the size of a pointer to a class, but not the size of the 'm\_xshHandle' class object\. tstageobjectcmd\.cpp 455 ```cpp class DVAPI TStageObjectParams { .... }; class RemovePegbarNodeUndo final : public TUndo { .... TXsheetHandle *m_xshHandle; public: int getSize() const override { return sizeof *this + sizeof(TStageObjectParams) + sizeof(m_xshHandle); } .... } ``` [Достаточно часто](https://pvs-studio.ru/ru/blog/examples/v568/) встречающаяся ошибка, которая может произойти как по невнимательности, так и по незнанию\. Тут, скорее всего, было дело в невнимательности, поскольку в первом слагаемом _this_ все\-таки был разыменован\. Если нужен размер объекта, то всегда нужно помнить, что указатель объекта нужно обязательно разыменовать\. Иначе мы просто получаем размер самого указателя\. ```cpp return sizeof *this + sizeof(TStageObjectParams) + sizeof(*m_xshHandle); ``` **Фрагмент N9** [V568](https://pvs-studio.ru/ru/docs/warnings/v568/) It's odd that 'sizeof\(\)' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object\. shaderfx\.cpp 107 ```cpp struct RectF { GLfloat m_val[4]; .... bool operator==(const RectF &rect) const { return (memcmp(m_val, rect.m_val, sizeof(this)) == 0); } }; ``` А тут, очевидно, забыли разыменовать указатель _this_\. В итоге получаем не размер объекта, а размер указателя\. Как результат, сравниваются только первые 4 или 8 байт \(в зависимости от разрядности\)\. Верный вариант кода: ```cpp return (memcmp(m_val, rect.m_val, sizeof(*this)) == 0); ``` **Фрагмент N10** [V554](https://pvs-studio.ru/ru/docs/warnings/v554/) Incorrect use of unique\_ptr\. The memory allocated with 'new \[\]' will be cleaned using 'delete'\. screensavermaker\.cpp 29 ```cpp void makeScreenSaver(TFilePath scrFn, TFilePath swfFn, std::string screenSaverName) { struct _stat results; .... int swfSize = results.st_size; std::unique_ptr swf(new char[swfSize]); .... } ``` Часто забывают, что от типа, которым инстанцируется unique\_ptr, зависит будет использоваться delete или delete\[\]\. В итоге, если инстанцировать указатель как в рассматриваемом фрагменте, при этом выделяя память через new\[\], может возникнуть неопределенное поведение, так как освобождение будет происходить через delete\. Чтобы этого избежать, нужно добавить к типу указателя квадратные скобки: std::unique\_ptr\. **Фрагмент N11** [V521](https://pvs-studio.ru/ru/docs/warnings/v521/) Such expressions using the ',' operator are dangerous\. Make sure the expression 'm\_to, m\_from \= it\-\>first\.getNumber\(\)' is correct\. flipbook\.cpp 509 ```cpp class LoadImagesPopup final : public FileBrowserPopup { .... int m_from, m_to, ....; .... } void LoadImagesPopup::onFilePathClicked(....) { TLevel::Iterator it; .... it = level->begin(); m_to, m_from = it->first.getNumber(); // <= for (; it != level->end(); ++it) m_to = it->first.getNumber(); if (m_from == -2 && m_to == -2) m_from = m_to = 1; m_minFrame = m_from; m_maxFrame = m_to; .... } ``` Возможно, программист ожидал, что можно присвоить одно значение нескольким переменным, просто выписав их через запятую\. Однако оператор "," в С\+\+ работает иным образом\. В нем первый операнд выполняется, и результат "сбрасывается", далее вычисляется второй операнд\. И, хотя переменная _m\_to_ инициализируется в последующем цикле, если что\-то пойдет не так или кто\-то неаккуратно произведет рефакторинг, возможно, что _m\_to_ так и не получит значения\. И вообще, в любом случае этот код выглядит странно\. **Фрагмент N12** [V532](https://pvs-studio.ru/ru/docs/warnings/v532/) Consider inspecting the statement of '\*pointer\+\+' pattern\. Probably meant: '\(\*pointer\)\+\+'\. trop\.cpp 140 ```cpp template void doGammaCorrect(TRasterPT raster, double gamma) { Gamma_Lut lut(....); int j; for (j = 0; j < raster->getLy(); j++) { T *pix = raster->pixels(j); T *endPix = pix + raster->getLx(); while (pix < endPix) { pix->r = lut.m_table[pix->r]; pix->b = lut.m_table[pix->b]; pix->g = lut.m_table[pix->g]; *pix++; // <= } } } ``` Маленький недочет, который может дополнительно путать человека, читающего код\. Инкремент, как и задумывалось, смещает указатель\. Однако после этого происходит бессмысленное разыменование\. Лучше просто написать _pix\+\+_\. **Фрагмент N13** [V773](https://pvs-studio.ru/ru/docs/warnings/v773/) The function was exited without releasing the 'autoCloseUndo' pointer\. A memory leak is possible\. vectortapetool\.cpp 575 ```cpp void joinLineToLine(....) { .... UndoAutoclose *autoCloseUndo = 0; .... autoCloseUndo = new UndoAutoclose(....); .... if (pos < 0) return; .... TUndoManager::manager()->add(autoCloseUndo); } ``` Подобных предупреждений было больше 20\. Зачастую где\-то в конце функции производится освобождение памяти, но для более ранних _return_ этот необходимый шаг оказывается пропущен\. Так и здесь\. В конце указатель передается _TUndoManager::manager\(\)\-\>add\(\)_, который берет на себя удаление указателя, однако выше присутствует _return_ для которого этот метод забыли позвать\. Так что всегда стоит помнить о своих указателях при любом выходе из функции, а не просто вписывать удаление куда\-то в конец блока или перед последним _return_\. Однако, если для сокращенной версии кода, эта ошибка выглядит очевидной, то в реальном более запутанном коде, отследить такую проблему может быть затруднительно\. Тут и поможет никогда неустающий статический анализатор\. **Фрагмент N14** [V522](https://pvs-studio.ru/ru/docs/warnings/v522/) Dereferencing of the null pointer 'region' might take place\. palettecmd\.cpp 94 ```cpp bool isStyleUsed(const TVectorImageP vi, int styleId) { .... int regionCount = vi->getRegionCount(); for (i = 0; i < regionCount; i++) { TRegion *region = vi->getRegion(i); if (region || region->getStyle() != styleId) return true; } .... } ``` Тут можно предположить, что разработчик перепутал правила short\-circuit evaluation и подумал, что если первая проверка указателя вернет _false_, то далее разыменования такого нулевого указателя не произойдёт\. Однако для оператора "\|\|" все совсем наоборот\. **Фрагмент N15** [V561](https://pvs-studio.ru/ru/docs/warnings/v561/) It's probably better to assign value to 'ca' variable than to declare it anew\. Previous declaration: xshcellmover\.cpp, line 319\. xshcellmover\.cpp 323 [V561](https://pvs-studio.ru/ru/docs/warnings/v561/) It's probably better to assign value to 'cb' variable than to declare it anew\. Previous declaration: xshcellmover\.cpp, line 320\. xshcellmover\.cpp 324xshcellmover\.cpp 323 ```cpp void redo() const override { int ca = m_cellsMover.getStartPos().x; int cb = m_cellsMover.getPos().x; .... if (!m_cellsMover.getOrientation()->isVerticalTimeline()) { int ca = m_cellsMover.getStartPos().y; int cb = m_cellsMover.getPos().y; } .... if (ca != cb) { .... } .... } ``` Возможно еще одна копипаста, но с не совсем обычной сутью ошибки\. Обращение к _x_ было заменено на _y_, но вот тип переменной в начале строки, из\-за которого и происходит локальная передекларация, убрать забыли\. В итоге, вместо смены ориентации позиций для исходных _ca_ и _cb_, создаются новые локальные _ca_ и _cb_, с которыми далее ничего не происходит\. А вот внешние _ca_ и _cb_ продолжают свое существование со значениями для _x_\. **Заключение N1** В процессе написания статьи мне стало интересно потыкать и попробовать что\-нибудь сделать в этой программе\. Может мне повезло, но странное поведение не заставило себя ждать: зависание, отображение моих манипуляций с планшетом после отвисания и странный квадрат после нажатия _Ctrl\+Z_\. К сожалению, повторить это поведение у меня не вышло\. ![0720_OpenToonz_ru/image6.png](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image6.png) Но на самом деле, несмотря на такое поведение и привитие привычки регулярно нажимать _Ctrl\+S_, OpenToonz впечатляет своим масштабом и функционалом\. Все\-таки не зря им пользуются и крупные студии\. И мое художество как бонус: ![](https://import.viva64.com/docx/blog/0720_OpenToonz_ru/image7.mp4) **Заключение N2** В случае OpenToonz очевидно, что пытаться исправить сразу все обнаруженные анализатором ошибки станет большой задачей, которая застопорит процесс разработки\. Для таких случаев существует подход "Массового подавления", когда технический долг заносится в suppress\-базу анализатора и дальнейшая работа с анализатором проводится уже на основе свежих срабатываний\. Ну а если появляется время, то можно и поразбирать технический долг\. P\.S\. Напоминаю, что разработчики открытых проектов могут использовать [бесплатный вариант лицензирования PVS\-Studio](https://pvs-studio.ru/ru/blog/posts/0614/)\.