Bad package code for creating Toonz 2D animations

A few days ago it became known that Digital Video, the creators of the TOONZ project, and the Japanese publisher DWANGO signed an agreement on the acquisition by DWANGO of the Toonz project, software for creating 2D animation.

Under the terms of the agreement signed between the parties, OpenToonz, a project developed by Toonz, will be made publicly available. It will also include some elements developed by Studio Ghibli, which in turn are active users of these programs. With their help, for example, Studio Ghibli created the “Howl's Moving Castle”, “Spirited Away”, “Ponyo Fish”, as well as many other paintings. Among them is also the cartoon "Futurama", which inspired me to write this revealing article about the source code of OpenToonz.

Introduction

OpenToonz is a software for creating 2D animations. The basis is the project "Toonz", which was developed by the Italian company Digital Video. Adapting this program, Studio Ghibli has been successfully using it for many years. In addition to animated films, the project was also involved in computer games - Discworld 2 and Claw.

It is worth noting that the price of the package has so far been $ 10,000, but the quality of the code leaves much to be desired. This project is a godsend for any static analyzer. The size of the OpenToonz source code is approximately 1/10 of the FreeBSD core, in which more than 40 serious errors were found using the PVS-Studio analyzer, but there are much more of them here!
')
OpenToonz was tested in Visual Studio 2013 using the PVS-Studio static analyzer version 6.03, which is actively developed and supports the C / C ++ / C # languages, and various build systems. Even during the compilation of the project, I became alert when the number of compiler warnings began to grow rapidly. At the end of the assembly, their number was 1211 pieces! Those. control over the quality of the source code almost did not pay attention. Moreover, some compiler warnings were turned off with the help of the #pragma warning directive , but even there were errors, which I will discuss below. This article is different in that many very real errors were found in the project, which are usually made by beginners who are just starting to learn C / C ++. I will begin the description of the detected analyzer warnings with errors of memory usage and pointers.

Wrong work with memory

V611 The memory was allocated using the 'new' operator. 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(); } 

Here, the analyzer found that dynamic memory is allocated and released in incompatible ways. After calling the operator new [], you must free the memory with the operator delete [] . It is with two square brackets! I focus on this for a reason - see the following example.

V611 The memory was allocated using the 'new T []' 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; } } .... } 

In C ++, the operators new / delete and new [] / delete [] are used in tandem with each other. Using different operators to allocate and free dynamic memory is an error. In the above code, the memory occupied by the uPrime array will not be correctly released.

Unfortunately, this place is not the only one. Another 20 places I wrote to the file OpenToonz_V611.txt .

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

 void makeScreenSaver(....) { .... std::auto_ptr<char> swf(new char[swfSize]); .... } 

Before us is an alternative variant of the considered error, but here the delete statement is “hidden” inside the smart pointer std :: auto_ptr . This also leads to undefined program behavior.

To correct the error, you must specify that the operator delete [] should be used.

The correct code is:

 std::unique_ptr<char[]> swf(new char[swfSize]); 

V599 class contains virtual functions. cellselection.cpp 891

 void redo() const { insertLevelAndFrameIfNeeded(); TTileSet *tiles; // <= bool isLevelCreated; pasteRasterImageInCellWithoutUndo(...., &tiles, ....); delete tiles; // <= TApp::instance()->getCurrentXsheet()->notifyXsheetChanged(); } 

Now let's talk about memory leaks and incomplete destruction of objects. In this example, the objects inherited from the TTileSet class will not be completely destroyed.

TTileSet class description :

 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; }; 

The class contains pure virtual functions and is abstract. You cannot create objects of this class, so it is used only by derived classes. Thus, due to the lack of a virtual destructor for TTileSet (there is a destructor, but it is not marked as virtual) all derived classes will not be completely cleared.

In the source code of OpenToonz, I found several classes that inherit from TTileSet :

 class DVAPI TTileSetCM32 : public TTileSet class DVAPI TTileSetCM32 : public TTileSet class DVAPI TTileSetFullColor : public TTileSet class DVAPI Tile : public TTileSet::Tile 

Each object of these classes (or classes inherited from them) will not be completely destroyed. Formally, this will lead to an undefined program behavior; in practice, this will most likely lead to a leak of memory and other resources.

Developers need to check two more similar places:

• The virtual destructor is not present, although the class contains virtual functions. tipcsrv.cpp 91
• V599 ColumnToCurveMapper class contains virtual functions. functionselection.cpp 278

Dangerous use of pointers

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); } .... } 

The getStylePage () function returns a pointer to a page: TPalette :: Page * . Such a comparison with zero does not make sense. Having made a search for the use of the getStylePage () function, I found that in all other cases, the result of this function is checked only for equality and inequality to zero, and in this place made a mistake.

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; .... } 

Most likely, the operators '&&' and '||' are confused in this code snippet. Otherwise, if the region pointer is zero, then its dereference will be executed.

V614 Potentially uninitialized pointer 'socket' used. Consider checking 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); } 

The analyzer has detected a potential use of an uninitialized socket pointer. If m_tcpServer is false, then the pointer will not be initialized. In this uninitialized form, it can be passed to the connect () function.

V595 The 'batchesTask' pointer was used 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; } .... } 

There are many places in the code where potentially null pointer dereferencing can occur. Usually, a corresponding check is present, but one or several places of using the pointer still remain unsafe. For example, in this code snippet there is a batchesTask check, but before the check, one pointer dereference will already be performed.

Another 29 similar places I wrote in the file OpenToonz_V595.txt .

Errors when working with strings

V530 The return value of the '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(); .... } 

The toUpper () method does not change the string 'text'. In the documentation, it is described as QString QString :: toUpper () const, i.e. is a constant method.

The correct code is:

 QString text = event->text(); if ((event->modifiers() & Qt::ShiftModifier)) text = text.toUpper(); 

There are three more functions in the code whose return value is not used. All these places require correction:

• V530 The return value of the function is required to be utilized. tfarmserver.cpp 569
• V530 The return value of the 'ftell' is required to be utilized. tiio_bmp.cpp 804
• V530 The return value of the function is required. 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())); // <= } .... } 

In operations with strings, the it1 uninitialized iterator is used. Most likely, in one place they forgot to replace it with an iterator ft .

V642 Saving the '_wcsicmp' function is the result of the type of the variable is inappropriate. 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; .... } 

The _wcsicmp function returns the following int values:

• <0 - string1 less then string2 ;
• 0 - string1 identical to string2 ;
• > 0 - string1 greater than string2 .

Note. '> 0' means any numbers, not at all 1. These numbers can be: 2, 3, 100, 256, 1024, 5555, and so on. The result of the _wcsicmp function may not fit into a char variable, which causes the comparison operator to return an unexpected result.

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; } 

The analyzer detected an error related to adding a character constant to a string literal. It was expected that a character would be added to the string, but a numeric value would be added to the pointer to the string, which would lead to the output of the string literal and an unexpected result.

To make it clearer, this code is equivalent to:

 const char *p1 = "\\"; const int delta = v[i]; const char *p2 = *p1 + delta; string h = p2; 

The correct code is:

 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; } 

The analyzer detected an expression whose result is not used. Most likely, the operator '+' was accidentally written in this function, instead of '+ ='. As a result, a square bracket is not added to the alias line at the end, as the programmer planned.

Wrong exceptions

V596 The object was not 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 ...."); } .... } 

The function has accidentally forgotten the throw keyword. Consequence - this code does not generate an exception in case of an error situation. Corrected code:

 throw std::domain_error("failed initialized: error on ...."); 

V746 Type slicing. 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 (...) { .... } .... } 

The analyzer has detected a potential error related to catching an exception by value. This means that a new se object of type TSystemException will be constructed using the copy constructor. This will lose some of the exception information that was stored in classes inherited from TSystemException .

Similar suspicious locations:

• V746 Type slicing. By reference rather than by value. iocommand.cpp 2650
• V746 Type slicing. By reference rather than by value. projectpopup.cpp 522
• V746 Type slicing. By reference rather than by value. projectpopup.cpp 537
• V746 Type slicing. By reference rather than by value. projectpopup.cpp 635
• V746 Type slicing. By reference rather than by value. tlevel_io.cpp 130
• V746 Type slicing. By reference rather than by value. calligraph.cpp 161
• V746 Type slicing. By reference rather than by value. calligraph.cpp 165
• V746 Type slicing. By reference rather than by value. patternmap.cpp 210
• V746 Type slicing. By reference rather than by value. patternmap.cpp 214
• V746 Type slicing. By reference rather than by value. patternmap.cpp 218
• V746 Type slicing. By reference rather than by value. scriptbinding_level.cpp 221

Invalid conditions

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); .... } 

An interesting mistake. Most likely, it was planned to determine whether the value of size () is even or odd. Therefore, the remainder of dividing by 2 should be compared with zero.

V502 Perhaps the '?:' Operator was different. The '?:' Operator has a lower limit 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; } 

In this code snippet, the ':?' Operator made a mistake related to the priority of the ternary. Its priority is lower than that of the addition operator. As a result, if the condition (re <gr) is false, then further calculations are performed incorrectly: real variables begin to add up to logical ones.

Never use several ternary operators together - this is the shortest way to add an error to the code.

V590 Consider inspecting the 'state == (- 3) || state! = 0 'expression. The expression is 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); .... } 

The condition that is marked with an arrow does not depend on the result of the state == Z_DATA_ERROR subexpression. This is easy to see if you build a truth table for the entire conditional expression.

Copy-paste programming

V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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->....); } .... } 

Here the variables newLeftNode and newRightNode in the conditions are confused. As a result of this error, the else branch is never executed. Most likely, one of the conditions should be like this: if (newRightNode-> m_concave) .

V501 There are identical to the left and 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),....)); .... } 

There are two logical variables in the code: m_cutLx and m_cutLy , which differ by only one last letter. In the given example, the same m_cutLx variable is used . Most likely, in one of them made a typo.

V501 There are identical sub-expressions "parentTask-> m_status == Aborted" 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); } .... } 

The analyzer detected two identical comparisons with the Aborted constant. Having performed a file search, in line 2028 of this file I found an identical block of code, but with the following condition:

 if (parentTask->m_status == Completed || parentTask->m_status == Aborted) { 

Most likely, in this place the conditional expression should be similar.

V501 There are identical sub-expressions 'cornerCoords.y> upperBound' to the left 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) { .... } .... } 

Here the programmer made a small typo, using y instead of x .

Another six mistakes made in the result of copy-paste programming , I will not describe, but give a list. These places must be checked by the developers:

• V501 There are identical sub-expressions' s.m_repoStatus == "modified" | operator. svnupdatedialog.cpp 210
• V501 There are identical sub-expressions "m_lineEdit-> hasFocus ()" operator. framenavigator.cpp 44
• V517 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 possibility 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

Mistakes

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 

This is how the compiler warnings are turned off, which were nevertheless noticed in this project. The error is that the #pragma warning (default: X) directive does not include a warning, but sets it to the DEFAULT state, which may not be the same as the programmer expects.

Corrected code:

 #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){}; 

An interesting error in the class initialization list. The m_subld field is initialized by itself, although, most likely, they wanted to write 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); .... } 

A large piece of code in which someone accesses the val array, consisting of 9 elements, by index from 1 to 9. Although there is a cycle next to it, in which the array is correctly referenced by index from 0 to 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; .... } 

The analyzer detected a comparison of enum values ​​with different types. Using a code search, I also discovered that the m_action class field is initialized with the correct type, and in this place is compared with a constant of another type.

Conclusion

As I already mentioned, the OpenToonz project is a godsend for any static code analyzer: there are a lot of serious errors for such a small project. The article not only did not include all the messages of the analyzer, it did not even include many interesting and serious warnings due to their large number. Of course, I will write about the check on the developer forum. Perhaps they will be interested in improving the quality of the code.

The intention to open the code of your software product Universal Scene Description (USD) was announced by Pixar. I look forward to this event.

I suggest everyone to try PVS-Studio on their C / C ++ / C # projects. The analyzer works in the Windows environment and supports various assembly systems.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Svyatoslav Razmyslov. Toonz code leaves much to be desired .