OpenToonz: inside and out

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

The previous errors review is available in the following article "Toonz code leaves much to be desired". The overall impression is quite similar, as it seems that the code quality has not improved much. 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, it should be mentioned that errors will not necessarily keep from active and productive usage of a software product. Most likely, the errors found live in rarely used or completely unused sections of the code, otherwise they would have been identified in the process of using the application and fixed. Nonetheless, it 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 can be revealed right when writing code, before the software production. Accordingly, with the regular use of a static analyzer, errors get fixed at an early stage. This saves both developer's time and company's money, and improves user experience. You would probably agree that it's unpleasant to bother developers every time one or another thing is not working.

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

The author's intentions are not very clear in this fragment. Usage of shift operators with negative numbers leads to undefined behavior. The standard gives a bit confusing description of shift operators behavior, but still let's check it out:

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 undefined if the right or left operand has a negative value. If the operand is of the signed type, has a non-negative value and fits into the resulting type, then the behavior will be normal.

Fragment N2

V517 The 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 m_offset variable is assigned different values depending on the value of m_currentItem. However, the duplicated check for BlackSlider is pointless. As we can see from the condition body, 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, whereas the check for this value is not performed. Thus, it is possible that some of the behavior scenarios was lost due to a 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 already impossible to conclude about the possible options for the tagName value. Most likely, just some option was missed, and in the end we have the 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 check number one, chancount is compared with 1 and the second check verifies if this variable is less or equal to 2. Eventually, as for the third condition, the only possible value of chancount is 2. Such an error might not lead to incorrect program operation, but it complicates reading and understanding of the code. For instance, the purpose of the else branch is not clear…

As a whole, 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, the error here was made even when initializing precSegmentIndex, currentSegmentIndex, startSegmentIndex, precChunkIndex variables. The developer could expect that initialization of the last element -1 initializes with the same value as other variables declared in 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 error, which is 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 the copy-pasted piece from the previous conditions.

All this cluttered mess 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 comprehensive: call of the delete operator for the pointer to void leads to undefined behavior. If the developer needed a universal function to remove interfaces, it might be worth making it template.

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 quite common bug that can occur both due to inattention and ignorance. Here, most likely, it was a matter of inattention, since in the first summand this was dereferenced anyway. If you need the size of an object, you should always remember that the pointer to that 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);
  }
};

Apparently, the author forgot to dereference the pointer this. As a result, we get the pointer size instead of the object size. As a result, only the first 4 or 8 bytes are compared (depending on the bitness). Correct code version:

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 depending on the type with which unique_ptr is instantiated, delete or delete[] will be used. As a result, if one instantiates the pointer as in the fragment under consideration, while allocating memory through new[], undefined behavior may occur, since the release will happen through delete. To avoid this, one has 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 one could assign one value to several variables simply by writing them separated by commas. However, the "," operator works differently in C ++. What happens is that the first operand is executed and the result is dropped, then the second operand is calculated. Even though the m_to variable is initialized in the subsequent loop, if something goes wrong and someone makes inaccurate refactoring, m_to might not get the value at all. Anyway, 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. As intended, the increment shifts the pointer, followed by the pointless dereference. It's best 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 of such warnings. Often, somewhere at the end of the function, memory gets freed.However, for earlier return cases this necessary step was skipped. The same happens here. At the end, the pointer is passed to TUndoManager::manager()->add() which takes care of freeing the memory. Nevertheless, authors forgot to call this method for the return above. So it is always worth remembering about the pointers whenever you exit the function, and not just write deletion somewhere at the end of the block or before the last return.

However, while for an abridged version of the code this error seems obvious, in real complicated code it can be difficult to identify such a problem. Here the ever-tired static analyzer will be of 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 short-circuit evaluation rules 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) {
    ....
  }
  ....
}

Probably, it's another copy-paste case, but with nontrivial essence of the error. The call to x was replaced with y, but the author forgot to remove the type of the variable at the beginning of the line, due to which local re-declaration 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 happens further. 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 play around this program. Maybe I was lucky, but the strange behavior was not long in coming: it hung up, then showed my manipulations with the tablet after getting back to normal functioning, followed by to a strange square after pressing Ctrl + Z. Unfortunately, I could not reproduce this behavior.

But in fact, despite this behavior and developing the habit to regularly press Ctrl + S, OpenToonz impresses with its scale and functionality. Still, it is not for nothing that large studios also use it.

Here's 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 get into the analyzer suppress base 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.

P.S. I remind you that developers of open source projects can use the free licensing option of PVS-Studio.