Поганий код пакета для створення 2D-анімації Toonz

На днях стало відомо про те, що 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

Кожен об'єкт цих класів (або успадкованих від них класів) буде руйнуватися не повністю. Формально це призведе до невизначеного поведінці програми; на практиці це, швидше за все, призведе до витоку пам'яті та інших ресурсів.

Розробникам необхідно перевірити ще два схожих місця:
  • V599 The virtual destructor is not present, although the 'MessageParser' class contains virtual functions. tipcsrv.cpp 91
  • V599 The virtual destructor is not present, although the 'ColumnToCurveMapper' class contains virtual functions. functionselection.cpp 278
Небезпечне використання покажчиків


V503 This is a nonsensical comparison: pointer < 0. styleselection.cpp 104
bool pasteStylesDataWithoutUndo (....)
{
....
if (palette->getStylePage(styleId) < 0) { // <=
// styleId non e' операційної primo viene: uso quello
// (cut/paste операційної primo viene per spostare stili)
palette->setStyle(styleId, style);
} else {
// styleId e' gia' операційної primo viene. 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();

У коді присутні ще три функції, що повертає значення яких не використовується. Всі ці місця потребують виправлення:
  • V530 The return value of function 'left' is required to be utilized. tfarmserver.cpp 569
  • V530 The return value of function 'ftell' is required to be utilized. tiio_bmp.cpp 804
  • V530 The return value of function 'accumulate' is required to be utilized. 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())); // <=
}
....
}

В операціях з рядками використовується неініціалізований ітератор 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 string1 less then string2;
  • 0 string1 identical to string2;
  • > 0 string1 greater than string2.
Зверніть увагу. '> 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.

Аналогічні підозрілі місця:
  • V746 Type slicing. An exception should be caught by reference rather than by value. iocommand.cpp 2650
  • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 522
  • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 537
  • V746 Type slicing. An exception should be caught by reference rather than by value. projectpopup.cpp 635
  • V746 Type slicing. An exception should be caught by reference rather than by value. tlevel_io.cpp 130
  • V746 Type slicing. An exception should be caught by reference rather than by value. calligraph.cpp 161
  • V746 Type slicing. An exception should be caught by reference rather than by value. calligraph.cpp 165
  • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 210
  • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 214
  • V746 Type slicing. An exception should be caught by reference rather than by value. patternmap.cpp 218
  • V746 Type slicing. An exception should be caught by reference rather than by value. scriptbinding_level.cpp 221
Неправильні умови


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 than the priority '+' 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». У цьому легко переконатися, якщо побудувати таблицю істинності всього виразу.

Copy-paste програмування


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 програмування, я не буду описувати, а наведу списком. Ці місця треба обов'язково перевірити розробникам:
  • V501 There are identical sub-expressions 's.m_repoStatus == «modified»' to the left and to the right of the '||' operator. svnupdatedialog.cpp 210
  • V501 There are identical sub-expressions 'm_lineEdit->hasFocus()' to the left and to the right of the '||' operator. framenavigator.cpp 44
  • V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability 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 probability 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
Різні помилки


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

Цікава помилка в списку ініціалізації класу. Поле 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 — знахідка для будь-якого статичного аналізатора коду: в ньому дуже багато серйозних помилок для такого невеликого проекту. В статтю не просто не увійшли всі повідомлення аналізатора, до неї не увійшли навіть багато цікаві і серйозні попередження з-за їх великої кількості. Звичайно ж, я відпишу про перевірку на форум розробників. Можливо, їм буде цікаво підвищення якості коду.

Намір відкрити код свого програмного продукту Універсальний Scene Description (USD) заявляла і компанія Pixar. З нетерпінням буду чекати цієї події.

Пропоную всім бажаючим спробувати PVS-Studio на своїх проектах C/C++/C#. Аналізатор працює в середовищі Windows і підтримує різні складальні системи.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Svyatoslav Razmyslov. Toonz code leaves much to be desired.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.