Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
Recently the world got to know that Digital Video, the makers of TOONZ, and DWANGO, a Japanese publisher, have signed an agreement for the acquisition by Dwango of Toonz, an animation software which was independently developed by Digital Video (Rome, Italy).
Digital Video and Dwango agreed to close the deal under the condition Dwango will publish, and develop, an Open Source platform based on Toonz (OpenToonz). It will include features developed by Studio Ghibli (*Toonz Ghibli Edition) which has been a long time Toonz user. "Howl's Moving Castle", "Spirited Away", "Ponyo on the Cliff by the Sea" and many other fantasy films - are among the most known fantasy films. One more cartoon of their production "Futurama" inspired our team to write this article about the source code of OpenToonz.
OpenToonz is a software for producing a 2D animation. It is based on the "Toonz" project, which was developed by Digital Video in Italy. Later it was customized by Studio Ghibli, and has now been used for creating its works for many years already. Besides the animation films, this project was also used for creation of computer games - Discworld, and Claw, for instance.
It should be noted that the price of the kit was about $10000, but the code quality leaves much to be desired. This project is a treasure trove for a static analyzer. The size of OpenToonz source code is about 1/10 of FreeBSD kernel, where we've found more than 40 serious bugs with the help of PVS-Studio, but here we have found much more!
OpenToonz was checked in Visual Studio 2013 using PVS-Studio, version 6.03, which supports C/C++/C#, different build systems, and is still being actively developed. The compilation stage already aroused a lot of suspicion when I saw the number of compiler warnings - by the end of the build there were 1211 of them! It shows that the code wasn't really cared for! Moreover, some of the compiler warnings were disabled by #pragma warning, and even there were several bugs there, that I will speak about later. This article will be a little atypical - we are presenting bugs found in the project, which are usually common for novice programmers who have just started learning C/C++. I will start the description with analyzer warnings which are connected with incorrect usage of memory and pointers.
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();
}The analyzer detected that dynamic memory is allocated and freed in incompatible ways. After the call of new[] operator the memory must be freed with the delete[] operator. Note that square brackets are used here. I want to draw your attention to this for a reason - have a look at the following example:
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;
}
}
....
}In C++ operators new/delete and new[]/delete[] are used in pairs. The use of different operators for allocation and deallocation of dynamic memory is an error. In the code given above, the memory that is allocated for the uPrime array won't be correctly freed.
Unfortunately, this fragment is not the only one. I have noted down 20 more fragments in the file 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]);
....
}Here we have an alternative variant of the bug we have just seen, but here the operator delete is "hidden" inside of the pointer std::auto_ptr. This also leads to undefined behavior.
To correct this, you must specify that delete[] must be used here.
The correct code variant:
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();
}Now let's talk about memory leaks and partial destruction of objects. In this example the objects, inherited from the TTileSet class will not be completely destroyed.
Description of the class 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;
};The class is abstract and contains pure virtual functions. You cannot create objects of this class as it is only used by derived classes. Thus, due to the missing virtual destructor in TTileSet (there is a destructor, but it isn't marked as a virtual one), all derived classes will fail to be completely cleaned.
In the OpenToonz code I found several classes that are inherited from TTileSet:
class DVAPI TTileSetCM32 : public TTileSet
class DVAPI TTileSetCM32 : public TTileSet
class DVAPI TTileSetFullColor : public TTileSet
class DVAPI Tile : public TTileSet::TileEach of these object classes (or those derived from them), will not be destroyed completely. The probable outcome, is undefined behavior; in practice, this is likely to lead to memory leaks, and other resource leaks.
Developers should review the following fragments too:
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 some page: TPalette::Page*. Such a comparison with 0 doesn't make sense. I have researched the way the function getStylePage() is used, and saw that in all other cases the result of this function is verified against null, but here the programmer 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 programmer put operators'&&' and '||' in the incorrect places. Otherwise, if the pointer region is null, it will be dereferenced.
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);
}The Analyzer detected potential use of an uninitialized pointer socket. If the variable m_tcpServer is false, the pointer will not be initialized. But, being uninitialized, it can still be passed to the connect() function.
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;
}
....
}There are a lot of fragments where we may potentially have null pointer dereference. Usually there is a necessary check, but one or more fragments are still unsafe. For example, there is a check batchesTask, but the pointer was already dereferenced before the check.
29 similar fragments are shown here, in the file: 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() method does not change the string 'text'. In the documentation it is described as: QString QString::toUpper(), i.e. it is a constant method.
Correct code variant:
QString text = event->text();
if ((event->modifiers() & Qt::ShiftModifier))
text = text.toUpper();In the code there are three functions, whose return value is not used. All of these fragments need to be edited:
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())); // <=
}
....
}The uninitialized iterator it1 is used in the string operations. Most likely, the programmer forgot to replace it with ft iterator.
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 function returns the following values of int type:
Please note that '>0' can be any number, not only 1. These numbers can be: 2, 3, 100, 256, 1024, 5555, and so on. _wcsicmp function result may not fit into a variable of char type, so the comparison operator will 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 caused by adding a character constant to a string literal. It was expected that a symbol would be added to the string, but a numeric value gets added to the pointer to the string, which leads to access beyond the string literal boundary, and an unexpected result.
Here is what this code is equal to:
const char *p1 = "\\";
const int delta = v[i];
const char *p2 = *p1 + delta;
string h = p2;Correct code variant:
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, '+' operator was accidentally written instead of '+='. As a result, a square bracket isn't added to the alias string, as the programmer planned.
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 ....");
}
....
}The keyword throw was accidentally forgotten in the function. As a result, this code does not generate an exception in case of an error situation. Correct code variant:
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 (...) {
....
}
....
}The analyzer detected a potential error, related to catching the exception by value. This means that a new se object of TSystemException will be constructed with the help of a copy constructor. At the same time, the code will lose some information about the exception that was stored in the classes, inherited from TSystemException.
Similar suspicious fragments:
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 bug. Perhaps the programmer wanted to check if the size() value is even or odd. That's why the remainder of division by 2 must be compared with zero.
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;
}In this code snippet, the programmer made a mistake related to the priority of the ternary operator ':?' . Its priority is lower than that of the addition operator. Consequently, if the condition (re < gr) is false, the following evaluations will be done incorrectly: real variables will be added to logical ones.
Never use several ternary operators at once - it's the easiest way to make an error.
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);
....
}The condition marked by an arrow does not depend on the result of the subexpression "state == Z_DATA_ERROR". This is easy to check, if you build a truth table of the whole conditional expression.
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->....);
}
....
}We see that newLeftNode and newRightNode variables are confused in the conditions. As a result of this error, the else branch is never executed. Most probably, one of the conditions should be as follows: 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),....));
....
}There are two logical variables in the code: m_cutLx and m_cutLy that differ just in one letter. But in the example given we see that only m_cutLx gets used. Perhaps, there is a typo in one of them.
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);
}
....
}The analyzer detected two similar comparisons with the constant Aborted. Having done a search in the file, I found a similar code block in line 2028 with this condition:
if (parentTask->m_status == Completed ||
parentTask->m_status == Aborted) {Perhaps, the condition should be similar in this fragment.
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) {
....
}
....
}Here the programmer made a small typo, using y instead of x.
I won't describe six more typos caused by copy-paste programming, I will just give them as a list. These fragments should also be definitely reviewed by the developers:
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)
#endifHere is how the compiler disables the warnings that were finally noticed in this project. The error is that the #pragma warning(default : X) doesn't turn on the warning, but sets it as a DEFAULT one, which can be different from what the programmer expects. The correct variant of the code should be:
#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){};An interesting bug in the list of class initialization. The field m_subld is initialized by itself; perhaps the programmer 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);
....
}It's a large code fragment, where a programmer accesses a val array, consisting of 9 elements, by the index from 1 to 9. Although, there is a loop where we see correct access of the array by the 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 the comparison of enum values which have different types. Using code search I also found that the field of m_action class is initialized with a correct type, but in this fragment it is compared with a constant of a different type.
As was already mentioned, OpenToonz project is a great find for a static code analyzer: even being quite small, it has a great number of serious bugs. Not all bugs are listed in this article; moreover, we weren't able to fit some serious warnings because of their big number. We will notify the developers about the bugs found, perhaps they will be interested in improving their code.
Pixar company also expressed their intention to open the source code of Universal Scene Description (USD). We are looking forward to this.
For those who may be interested: you may find PVS-Studio here, and run it on your C/C++/C# projects. The analyzer works in the Windows environment, and supports various build systems.
0
0
0
0