НОВОСТИ OpenToonz: снаружи и внутри

BDFpromo · 26.03.2020

Прошло уже почти четыре года с тех пор, как PVS-Studio поверял исходный код OpenToonz. Этот проект является очень мощным инструментом для создания двухмерной анимации. За время с последней проверки с его помощью были созданы такие мультипликационные произведения, как Мэри и Ведьмин цветок, Бэтмэн-Нинзя, Промаре и другие. Раз большие студии продолжают пользоваться Toonz, то почему бы не проверить качество исходного кода еще раз?

С предыдущим разбором ошибок можно познакомиться в статье "

You must be registered for see links

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

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

Фрагмент N1

You must be registered for see links

Undefined behavior. Check the shift operator '

decode_mcu_AC_refine (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
{
int p1, m1;
p1 = 1 Al;
m1 = (-1) 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
Итак, поведение не определено, если правый или левый операнд имеет отрицательное значение. Если операнд имеет знаковый тип, неотрицательное значение и вмещается в результирующий тип, то поведение будет нормальным.

Фрагмент N2

You must be registered for see links

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

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) // / code>

Тут переменной m_offset присваиваются различные значения в зависимости от значения m_currentItem. Однако дублирование проверки на BlackSlider бессмысленно, и из тела условия можно увидеть, что в вычислении участвует переменная m_white. Заглянем в возможные значения для m_currentItem.

LevelControlItem m_currentItem;

enum LevelControlItem {
None = 0,
BlackSlider,
WhiteSlider,
GammaSlider,
ThresholdSlider,
Histogram,
NumItems
};

Оказывается, что возможно еще и значение WhiteSlider, на которое проверка как раз и не производится. Таким образом, вполне возможно, что какой-то из сценариев поведения был потерян из-за ошибки копипасты.

Фрагмент N3

You must be registered for see links

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

void TPalette::loadData(TIStream &is) {
....
std::string tagName;
while (is.openChild(tagName)) {
if (tagName == "version") {
....
} else if (tagName == "stylepages") { // / code>

Еще одна похожая ошибка. Здесь также у одинаковых условий разные тела, но сделать вывод о возможных вариантах значения tagName уже нельзя. Скорее всего, просто какой-то вариант был упущен, и в итоге мы имеем код, который никогда не будет выполнен.

Фрагмент N4

You must be registered for see links

Expression 'chancount == 2' is always true. psd.cpp 720

void TPSDReader::readImageData(...., int chancount) {
....
if (depth == 1 && chancount == 1) { // 1) {
....
for (....) {
if (chancount >= 3) {
....
if (chancount == 4)
....
else
....
} else if (chancount / / code>

В эти проверки закралась небольшая логическая ошибка. В проверке под номером один chancount сравнивается с 1, а проверке номер 2 проверяется, что эта переменная меньше или равна 2. В итоге, к условию под номером три единственным возможным значением chancount является 2. К неправильной работе программы такая ошибка может и не приведет, но она усложняет чтение и понимание кода. Например, непонятно, зачем тогда else-ветка…

В целом рассматриваемая в этом фрагменте функция занимает чуть больше 300 строк кода и состоит из таких вот нагромождений условий и циклов.

Фрагмент N5

You must be registered for see links

Uninitialized variable 'precSegmentIndex' used. Consider checking the fifth actual argument of the 'insertBoxCorners' function. rasterselection.cpp 803

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

You must be registered for see links

Consider inspecting the 's != "" && s == «color»' expression. The expression is excessive or contains a misprint. cleanupparameters.cpp 416

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") // code>

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

Такая лапша, занимающая десятки и более строк кода, вполне может хранить в себе еще какие-либо ошибки логики, и их поиск при таком форматировании может обернуться мучениями.

Фрагмент N7

You must be registered for see links

Calling a 'delete' operator for a void pointer will cause undefined behavior. pluginhost.cpp 1327

static void release_interface(void *interf) {
if (interf) delete interf;
}

Тут само сообщение анализатора уже достаточно исчерпывающее: вызов оператора delete для указателя на тип void приводит к неопределенному поведению. Если была необходима универсальная функция для удаления интерфейсов, возможно её стоило сделать шаблонной.

template
static void release_interface(T *interf) {
if (interf) delete interf;
}

Фрагмент N8

You must be registered for see links

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

class DVAPI TStageObjectParams {
....
};

class RemovePegbarNodeUndo final : public TUndo {
....
TXsheetHandle *m_xshHandle;

public:
int getSize() const override {
return sizeof *this + sizeof(TStageObjectParams) + sizeof(m_xshHandle);
}
....
}

You must be registered for see links

встречающаяся ошибка, которая может произойти как по невнимательности, так и по незнанию. Тут, скорее всего, было дело в невнимательности, поскольку в первом слагаемом this все-таки был разыменован. Если нужен размер объекта, то всегда нужно помнить, что указатель объекта нужно обязательно разыменовать. Иначе мы просто получаем размер самого указателя.

return sizeof *this + sizeof(TStageObjectParams) + sizeof(*m_xshHandle);

Фрагмент N9

You must be registered for see links

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

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 байт (в зависимости от разрядности). Верный вариант кода:

return (memcmp(m_val, rect.m_val, sizeof(*this)) == 0);

Фрагмент N10

You must be registered for see links

Incorrect use of unique_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. screensavermaker.cpp 29

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

You must be registered for see links

Such expressions using the ',' operator are dangerous. Make sure the expression 'm_to, m_from = it->first.getNumber()' is correct. flipbook.cpp 509

class LoadImagesPopup final : public FileBrowserPopup {
....
int m_from, m_to, ....;
....
}
void LoadImagesPopup:

nFilePathClicked(....) {
TLevel::Iterator it;
....
it = level->begin();
m_to, m_from = it->first.getNumber(); // 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

You must be registered for see links

Consider inspecting the statement of '*pointer++' pattern. Probably meant: '(*pointer)++'. trop.cpp 140

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++; // code>

Маленький недочет, который может дополнительно путать человека, читающего код. Инкремент, как и задумывалось, смещает указатель. Однако после этого происходит бессмысленное разыменование. Лучше просто написать pix++.

Фрагмент N13

You must be registered for see links

The function was exited without releasing the 'autoCloseUndo' pointer. A memory leak is possible. vectortapetool.cpp 575

void joinLineToLine(....) {
....
UndoAutoclose *autoCloseUndo = 0;
....
autoCloseUndo = new UndoAutoclose(....);
....
if (pos < 0) return;
....
TUndoManager::manager()->add(autoCloseUndo);
}

Подобных предупреждений было больше 20. Зачастую где-то в конце функции производится освобождение памяти, но для более ранних return этот необходимый шаг оказывается пропущен. Так и здесь. В конце указатель передается TUndoManager::manager()->add(), который берет на себя удаление указателя, однако выше присутствует return для которого этот метод забыли позвать. Так что всегда стоит помнить о своих указателях при любом выходе из функции, а не просто вписывать удаление куда-то в конец блока или перед последним return.

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

Фрагмент N14

You must be registered for see links

Dereferencing of the null pointer 'region' might take place. palettecmd.cpp 94

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

You must be registered for see links

It's probably better to assign value to 'ca' variable than to declare it anew. Previous declaration: xshcellmover.cpp, line 319. xshcellmover.cpp 323

You must be registered for see links

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

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. К сожалению, повторить это поведение у меня не вышло.

Но на самом деле, несмотря на такое поведение и привитие привычки регулярно нажимать Ctrl+S, OpenToonz впечатляет своим масштабом и функционалом. Все-таки не зря им пользуются и крупные студии.

И мое художество как бонус:

Заключение N2

В случае OpenToonz очевидно, что пытаться исправить сразу все обнаруженные анализатором ошибки станет большой задачей, которая застопорит процесс разработки. Для таких случаев существует подход «Массового подавления», когда технический долг заносится в suppress-базу анализатора и дальнейшая работа с анализатором проводится уже на основе свежих срабатываний. Ну а если появляется время, то можно и поразбирать технический долг.

P.S. Напоминаю, что разработчики открытых проектов могут использовать

You must be registered for see links

.

You must be registered for see links

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Victoria Khanieva.

You must be registered for see links

.

НОВОСТИ OpenToonz: снаружи и внутри

BDFpromo