На днях стало известно о том, что 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Каждый объект этих классов (или унаследованных от них классов) будет разрушаться не полностью. Формально это приведёт к неопределённому поведению программы; на практике это, скорее всего, приведёт к утечке памяти и иных ресурсов.
Разработчикам необходимо проверить ещё два похожих места:
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();В коде присутствуют ещё три функции, возвращаемое значение которых не используется. Все эти места требуют исправления:
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'означает любые числа, а вовсе не 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.
Аналогичные подозрительные места:
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". В этом легко убедиться, если построить таблицу истинности всего условного выражения.
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 программирования, я не буду описывать, а приведу списком. Эти места надо обязательно проверить разработчикам:
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)
#endifV546 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 и поддерживает различные сборочные системы.