👶 🏣 🍪 OpenToonz: Inside and Out 😈 🌴 ™️

Almost four years have passed since PVS-Studio verified the OpenToonz source code. This project is a very powerful tool for creating two-dimensional animation. Since the last check, with his help, such animated works as Mary and the Witch Flower, Batman Ninja, Promare and others were created. Since large studios continue to use Toonz, why not check the quality of the source code again?

You can get acquainted with the previous analysis of errors in the article " Bad code of a package for creating 2D animations of Toonz ". In general, it does not seem that the quality of the code has improved much, so the overall impression is not better. In addition, many of the same errors were found as in the previous article. We will not consider them again, since there are many things to choose from.

However, you must understand that the presence of errors does not necessarily prevent the active and productive use of the software product. Most likely, the errors found live in rarely used or completely unused parts of the code, otherwise they would have been identified in the process of using the application and fixed. However, this does not mean that static analysis is redundant. It’s just that the meaning of static analysis is not in finding old and irrelevant errors, but in reducing the cost of the development process. Many errors when writing code can be detected immediately, and not during the operation of the software. Accordingly, with the regular use of a static analyzer, errors are corrected at an early stage. This saves both developers time and company money, and improves the perception of the product by users. Agree, it’s unpleasant to constantly unsubscribe to developers,that one thing or the other does not work.

Fragment N1

V610 Undefined behavior. Check the shift operator '<<'. The left operand '(- 1)' is negative.

decode_mcu_AC_refine (j_decompress_ptr cinfo, JBLOCKROW *MCU_data)
{
  int p1, m1;
  p1 = 1 << cinfo->Al;    
  m1 = (-1) << cinfo->Al; 
  ....
}

It is not clear what the author of the code wanted to achieve here. Using shift operators with negative numbers leads to undefined behavior. The way the behavior of the shift operators is described in the standard seems a bit confusing at first glance, but still check it:

1. The type of the result is that of the promoted left operand. The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand.

2. The value of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are zero-filled. If E1 has an unsigned type, the value of the result is E1 * 2 ^ E2, reduced modulo one more than the maximum value representable in the result type. Otherwise, if E1 has a signed type and non-negative value, and E1 * 2 ^ E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined.

So, the behavior is not defined if the right or left operand has a negative value. If the operand is of signed type, non-negative value and fits into the resulting type, then the behavior will be normal.

Fragment N2

V517The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 156, 160. cameracapturelevelcontrol.cpp 156

void CameraCaptureLevelHistogram::mousePressEvent(QMouseEvent* event) {
  if (event->button() != Qt::LeftButton) return;
  if (m_currentItem == Histogram) {
    m_histogramCue = true;
    return;
  }
  if (m_currentItem == None) return;
  QPoint pos = event->pos();
  if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_black;
  else if (m_currentItem == GammaSlider)
    m_offset = pos.x() - SIDE_MARGIN - gammaToHPos(m_gamma, m_black, m_white);
  else if (m_currentItem == BlackSlider)  // <=
    m_offset = pos.x() - SIDE_MARGIN - m_white;
  else if (m_currentItem == ThresholdSlider)
    m_offset = pos.x() - SIDE_MARGIN - m_threshold;
}

Here the variable m_offset is assigned different values depending on the value of m_currentItem . However, duplicating the check on BlackSlider is pointless, and from the body of the condition you can see that the m_white variable is involved in the calculation . Let's look at the possible values for m_currentItem .

  LevelControlItem m_currentItem;

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

It turns out that the WhiteSlider value is also possible , for which verification is just not performed. Thus, it is entirely possible that some of the behavior scenarios was lost due to copy-paste error.

Fragment N3

V517 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 784, 867. tpalette.cpp 784

void TPalette::loadData(TIStream &is) {
  ....
  std::string tagName;
  while (is.openChild(tagName)) {
    if (tagName == "version") {
      ....
    } else if (tagName == "stylepages") { // <=
      while (!is.eos()) {
        if (....){
          ....
        }
        ....
        is.closeChild();
        }
    } else if (tagName == "refImgPath") {
      ....
    } else if (tagName == "animation") {
      ....
    } else if (tagName == "stylepages") { // <=
      int key = '0';
      while (!is.eos()) {
        int styleId = 0;
        ....
      }
    } 
      ....
  }
}

Another similar error. Here, the same conditions have different bodies, but it is no longer possible to conclude about the possible options for tagName . Most likely, just some option was missed, and in the end we have code that will never be executed.

Fragment N4

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

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

A small logical error crept into these checks. In the test under number one, the chancount is compared with 1, and in the test number 2 it is checked that this variable is less than or equal to 2. As a result, the only possible chancount value is 2 to the condition under number 3. This error may not lead to incorrect operation of the program, but it complicates the reading and understanding of the code. For example, it is not clear why the else-branch then ...

In general, the function considered in this fragment takes a little more than 300 lines of code and consists of such heaps of conditions and loops.

Fragment N5

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

TStroke getIntersectedStroke(TStroke &stroke, TRectD bbox) {
  ....
  int precSegmentIndex, currentSegmentIndex, startSegmentIndex,
      precChunkIndex = -1;
  ....
  if (....) {
    insertBoxCorners(bbox, points, outPoints, currentSegmentIndex,
                     precSegmentIndex);
    ....
  }
}

void insertBoxCorners(...., int currentSegmentIndex, int precSegmentIndex) {
  ....
  bool sameIndex = (precSegmentIndex == currentSegmentIndex);
  ....
  int segmentIndex = precSegmentIndex;
  ....
}

Perhaps, a mistake was made here even during the initialization of the variables precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex . The developer could expect that initialization of the last element -1 initializes with the same value as other variables declared on the same line.

Fragment N6

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

void CleanupParameters::loadData(TIStream &is, bool globalParams) {
  ....
  std::string s = is.getTagAttribute("sharpness");
  ....
  if (....)
  {
    ....
  } else if (tagName == "lineProcessing")
    ....
    if (s != "" && isDouble(s)) 
      ....
    if (s != "" && isDouble(s))
      ....
    if (s != "" && s == "color") // <=
      ....
  } else if (tagName == "despeckling") {
    ....  
  }
  ....
}

This mistake, even rather a flaw, in itself leads only to an unnecessary comparison. However, if we look at the code as a whole, it will become clear that the extra comparison appeared as a result of copy-paste from the previous conditions.

Such noodles, which occupies dozens or more lines of code, may well contain any other logic errors, and their search with this formatting can turn into torment.

Fragment N7

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

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

Here the analyzer message itself is already quite exhaustive: calling the delete operator on a pointer to a void type leads to undefined behavior. If a universal function was needed to remove interfaces, it might be worth making it a boilerplate.

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

Fragment N8

V568 It's odd that 'sizeof ()' operator evaluates the size of a pointer to a class, but not the size of the 'm_xshHandle' class object. tstageobjectcmd.cpp 455

class DVAPI TStageObjectParams {
  ....
};

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

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

A common enough mistake that can occur both through inattention and ignorance. Here, most likely, the matter was inattention, since in the first term this was nevertheless dereferenced. If you need the size of an object, you should always remember that the pointer of an object must be dereferenced. Otherwise, we just get the size of the pointer itself.

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

Fragment N9

V568 It's odd that 'sizeof ()' operator evaluates the size of a pointer to a class, but not the size of the 'this' class object. shaderfx.cpp 107

struct RectF {
  GLfloat m_val[4];
  ....
  bool operator==(const RectF &rect) const {
    return (memcmp(m_val, rect.m_val, sizeof(this)) == 0);
  }
};

And here, obviously, they forgot to dereference the this pointer . As a result, we get not the size of the object, but the size of the pointer. As a result, only the first 4 or 8 bytes are compared (depending on the bit depth). The correct version of the code:

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

Fragment N10

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

void makeScreenSaver(TFilePath scrFn, TFilePath swfFn,
                     std::string screenSaverName) {
  struct _stat results;
....
  int swfSize = results.st_size;
  std::unique_ptr<char> swf(new char[swfSize]);
....
}

It is often forgotten that the type by which unique_ptr is instantiated depends on whether delete or delete [] will be used. As a result, if you instantiate the pointer as in the fragment under consideration, while allocating memory through new [], undefined behavior may occur, since the release will occur through delete. To avoid this, you need to add square brackets to the pointer type: std :: unique_ptr <char []>.

Fragment N11

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

class LoadImagesPopup final : public FileBrowserPopup {
  ....
  int m_from, m_to, ....;
  ....
}
void LoadImagesPopup::onFilePathClicked(....) {
  TLevel::Iterator it;
  ....
  it = level->begin();
  m_to, m_from = it->first.getNumber();  // <=
  for (; it != level->end(); ++it) m_to = it->first.getNumber();

  if (m_from == -2 && m_to == -2) m_from = m_to = 1;

  m_minFrame = m_from;
  m_maxFrame = m_to;
  ....
}

Perhaps the programmer expected that you could assign one value to several variables by simply writing them out with a comma. However, the "," operator in C ++ works differently. In it, the first operand is executed, and the result is "reset", then the second operand is calculated. And, although the m_to variable is initialized in a subsequent cycle, if something goes wrong or someone inaccurately refactors, it is possible that m_to will not receive a value. And in general, in any case, this code looks strange.

Fragment N12

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

template <class T, class Q>
void doGammaCorrect(TRasterPT<T> raster, double gamma) {
  Gamma_Lut<Q> lut(....);

  int j;
  for (j = 0; j < raster->getLy(); j++) {
    T *pix    = raster->pixels(j);
    T *endPix = pix + raster->getLx();
    while (pix < endPix) {
      pix->r = lut.m_table[pix->r];
      pix->b = lut.m_table[pix->b];
      pix->g = lut.m_table[pix->g];
      *pix++; // <=
    }
  }
}

A small flaw, which can further confuse the person reading the code. The increment, as intended, shifts the pointer. However, after this there is a meaningless dereferencing. Better to just write pix ++ .

Fragment N13

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

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

There were more than 20 such warnings. Often somewhere at the end of the function the memory is freed, but for earlier return this necessary step is skipped. So it is here. At the end, the pointer is passed to TUndoManager :: manager () -> add () , which takes care of deleting the pointer, but there is a return above for which you forgot to call this method. So it is always worth remembering your pointers whenever you exit the function, and not just enter the delete somewhere at the end of the block or before the last return .

However, if for an abbreviated version of the code this error seems obvious, then in real, more complicated code, tracking down such a problem can be difficult. Here the ever-tired static analyzer will help.

Fragment N14

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

bool isStyleUsed(const TVectorImageP vi, int styleId) {
  ....
  int regionCount = vi->getRegionCount();
  for (i = 0; i < regionCount; i++) {
    TRegion *region = vi->getRegion(i);
    if (region || region->getStyle() != styleId) return true;
  }
  ....
}

Here we can assume that the developer mixed up the rules of short-circuit evaluation and thought that if the first check of the pointer returns false , then dereferencing of such a null pointer will not occur. However, for the operator "||" it's quite the opposite.

Fragment N15

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

V561 It's probably better to assign value to 'cb' variable than to declare it anew. Previous declaration: xshcellmover.cpp, line 320. xshcellmover.cpp 324xshcellmover.cpp 323

void redo() const override {
  int ca       = m_cellsMover.getStartPos().x;
  int cb       = m_cellsMover.getPos().x;
  ....
  if (!m_cellsMover.getOrientation()->isVerticalTimeline()) {
    int ca = m_cellsMover.getStartPos().y;
    int cb = m_cellsMover.getPos().y;
  }
  ....
  if (ca != cb) {
    ....
  }
  ....
}

Perhaps another copy-paste, but with a not very ordinary essence of the error. The call to x was replaced by y , but they forgot to remove the type of the variable at the beginning of the line, due to which local re-reporting occurs. As a result, instead of changing the position orientation for the initial ca and cb , new local ca and cb are created , with which nothing further happens. But external ca and cb continue to exist with values for x .

Conclusion N1

In the process of writing the article, it became interesting for me to poke and try to do something in this program. Maybe I was lucky, but strange behavior not long in coming: hang, display my manipulation of the tablet after the sagging and the odd square by pressing the Ctrl + the Z . Unfortunately, I could not repeat this behavior.

But in fact, despite this behavior and instilling the habit of regularly pressing Ctrl + S , OpenToonz impresses with its scale and functionality. Still, it is not in vain that large studios also use it.

And my art as a bonus:

Conclusion N2

In the case of OpenToonz, it is obvious that trying to fix all the errors detected by the analyzer at once will be a big task that will stall the development process. For such cases, there is the “Mass Suppression” approach, when technical debt is entered into the suppressor base of the analyzer and further work with the analyzer is carried out on the basis of fresh responses. Well, if time appears, then you can sort out the technical debt.

PS I remind you that developers of open source projects can use the free licensing option PVS-Studio . If you want to share this article with an English-speaking audience, then please use the translation link: Victoria Khanieva. OpenToonz .

OpenToonz: Inside and Out

More articles: