🎅🏻 👨🏼‍🚀 ✊🏻 OpenToonz：由内而外 👟 ⚖️ 📞

自从PVS-Studio验证OpenToonz源代码以来，已经过去了将近四年。该项目是用于创建二维动画的非常强大的工具。自上次检查以来，在他的帮助下，创作了诸如玛丽与女巫之花，蝙蝠侠忍者，普罗玛雷等动画作品。由于大型工作室继续使用Toonz，为什么不再次检查源代码的质量？

您可以在文章“ 用于创建Toonz的2D动画的程序包的错误代码 ”中熟悉以前的错误分析。通常，似乎代码的质量似乎并没有改善很多，因此总体印象并不更好。此外，发现了许多与上一篇文章相同的错误。我们将不再考虑它们，因为有很多选择。

但是，您必须了解，错误的存在并不一定会阻止软件产品的积极有效地使用。发现的错误很可能存在于很少使用或完全未使用的代码部分中，否则它们将在使用应用程序的过程中被识别并修复。但是，这并不意味着静态分析是多余的。仅仅是静态分析的意思不是寻找旧的和不相关的错误，而是减少开发过程的成本。可以立即检测到编写代码时发生的许多错误，而不是在软件运行期间发现。因此，通过定期使用静态分析仪，可以在早期纠正错误。这样既节省了开发人员的时间，又节省了公司的资金，并改善了用户体验。同意，不断退订开发人员是不愉快的，一件事或另一件事不起作用。

片段N1

V610未定义的行为。检查移位运算符“ <<”。左操作数'（-1）'为负。

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

目前尚不清楚代码的作者想要在这里实现什么。使用带负数的移位运算符会导致不确定的行为。乍一看，标准中描述移位运算符行为的方式似乎有些混乱，但仍要检查一下：

1.结果的类型是提升后的左操作数的类型。如果右操作数为负或大于或等于提升后的左操作数的位长度，则该行为是不确定的。

2. E1 << E2的值是E1左移E2位的位置；空出的位为零。如果E1具有无符号类型，则结果的值为E1 * 2 ^ E2，比结果类型中可表示的最大值模减少1。否则，如果E1具有带符号的类型且非负值，并且E1 * 2 ^ E2在结果类型中可表示，则这是结果值；否则，行为是不确定的。

因此，如果左右操作数的值为负，则不会定义该行为。如果操作数是带符号类型，非负值并且适合结果类型，则该行为将是正常的。

片段N2

V517检测到使用'if（A）{...} else if（A）{...}'模式。存在逻辑错误的可能性。检查行：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;
}

这里可变M_OFFSET是取决于的值分配不同的值m_currentItem。但是，在BlackSlider上重复检查是没有意义的，并且从条件主体中可以看到m_white变量参与了计算。让我们看一下m_currentItem的可能值。

  LevelControlItem m_currentItem;

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

事实证明，WhiteSlider值也是可能的，只是不对其进行验证。因此，完全有可能由于复制粘贴错误而丢失了某些行为方案。

片段N3

V517检测到使用'if（A）{...} else if（A）{...}'模式。存在逻辑错误的可能性。检查行：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;
        ....
      }
    } 
      ....
  }
}

另一个类似的错误。在这里，相同的条件具有不同的主体，但不再可能得出有关tagName值的可能选项的结论。最有可能的是，只有一个选项被遗漏了，最后我们有了永远不会执行的代码。

片段N4

V547表达式“ chancount == 2”始终为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) {
  ....
}

一个小的逻辑错误潜入了这些检查。在编号为1的测试中，将chancount与1进行比较，在编号为2的测试中，检查此变量是否小于或等于2。结果，唯一的chancount值是数字3 下的条件2。此错误可能不会导致错误的程序操作，但是它会使代码的阅读和理解复杂化。例如，目前尚不清楚为什么else-branch那么...

通常，此片段中考虑的函数需要300多行代码，并且由条件和循环的此类堆组成。

片段N5

V614使用了未初始化的变量'precSegmentIndex'。考虑检查“ insertBoxCorners”函数的第五个实际参数。光栅选择.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;
  ....
}

也许，即使在初始化变量precSegmentIndex，currentSegmentIndex，startSegmentIndex，precChunkIndex的过程中，也犯了一个错误。开发人员可以期望最后一个元素-1的初始化使用与在同一行上声明的其他变量相同的值进行初始化。

片段N6

V590考虑检查's！=“” && s ==“ color”'表达式。表达式过多或打印错误。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") {
    ....  
  }
  ....
}

这个错误，甚至是一个缺陷，本身只会导致不必要的比较。但是，如果我们将代码作为一个整体来看，就会很清楚，由于以前条件下的复制粘贴，出现了额外的比较。

这样的面条占用数十行或更多行代码，很可能包含任何其他逻辑错误，使用这种格式进行搜索可能会导致折磨。

片段N7

V772为空指针调用“删除”运算符将导致未定义的行为。pluginhost.cpp 1327

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

分析器消息本身已经非常详尽：在指向void类型的指针上调用delete运算符会导致未定义的行为。如果需要通用功能来删除接口，则值得将其作为样板。

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

片段N8

V568很奇怪，'sizeof（）'运算符会评估指向类的指针的大小，而不是'm_xshHandle'类对象的大小。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);
  }
  ....
}

一个很常见的错误，可能由于疏忽和无知而发生。在这里，最有可能的问题是注意力不集中，因为在第一个术语中，这个问题仍然被取消引用。如果需要对象的大小，则应始终记住必须取消引用对象的指针。否则，我们只获得指针本身的大小。

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

片段N9

V568很奇怪，'sizeof（）'运算符会评估指向类的指针的大小，而不是'this'类对象的大小。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);
  }
};

在这里，很明显，他们忘记了取消引用this指针。结果，我们得到的不是对象的大小，而是指针的大小。结果，仅比较前4或8个字节（取决于位深度）。正确的代码版本：

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

片段N10

V554不正确地使用了unique_ptr。分配给'new []'的内存将使用'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]);
....
}

通常会忘记，实例化unique_ptr的类型取决于将使用delete还是delete []。结果，如果像所考虑的片段中那样实例化指针，则通过new []分配内存时，可能会发生未定义的行为，因为释放将通过delete发生。为避免这种情况，您需要在指针类型上添加方括号：std :: unique_ptr <char []>。

片段N11

V521使用'，'运算符的此类表达式很危险。确保表达式'm_to，m_from = it-> first.getNumber（）'是正确的。 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;
  ....
}

也许程序员希望您可以通过简单地用逗号将变量赋值来为一个变量赋一个值。但是，C ++中的“，”运算符的工作方式有所不同。其中，执行第一个操作数，结果为“重置”，然后计算第二个操作数。并且，尽管m_to变量是在随后的周期中初始化的，但是如果出现问题或有人进行了错误的重构，则m_to可能不会收到值。通常，无论如何，这段代码看起来很奇怪。

片段N12

V532考虑检查“ *指针++”模式的语句。可能的意思是：“（*指针）++”。 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++; // <=
    }
  }
}

一个小的缺陷，可能会使阅读代码的人更加困惑。如预期的那样，增量将指针移动。但是，此后将无意义地取消引用。最好只写pix ++。

片段N13

V773在不释放'autoCloseUndo'指针的情况下退出了该函数。可能发生内存泄漏。 vectortapetool.cpp 575

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

有超过20种这样的警告，通常在函数末尾的某个位置释放了内存，但是对于更早的返回，跳过了此必要的步骤。就在这里。最后，将指针传递给TUndoManager :: manager（）-> add（），它负责删除指针，但是上面有一个返回值，您忘了为此调用。因此，每次退出函数时始终值得记住您的指针，而不仅仅是在块末尾或最后一次返回之前输入delete 。

但是，如果对于该代码的缩写版本而言，此错误似乎很明显，那么在实际的，更复杂的代码中，很难找到此类问题。在这里，久负盛名的静态分析仪将为您提供帮助。

片段N14

V522可能会取消引用空指针“区域”。 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;
  }
  ....
}

在这里我们可以假设开发人员混淆了短路评估的规则，并认为如果指针的第一次检查返回false，那么将不会取消引用此类空指针。但是，对于运算符“ ||”相反。

片段N15

V561将值分配给“ ca”变量可能比重新声明它更好。先前的声明：xshcellmover.cpp，第319行。xshcellmover.cpp 323

V561将值分配给'cb'变量可能比重新声明它更好。先前的声明：xshcellmover.cpp，第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) {
    ....
  }
  ....
}

也许是另一种复制粘贴，但是具有错误的不是很普通的本质。对x的调用由y代替，但是他们忘记在行的开头删除变量的类型，因为这会发生局部重新报告。结果，不是更改初始ca和cb的位置方向，而是创建了新的局部ca和cb，此后不再发生任何事情。但是外部ca和cb继续存在，且具有x的值。

结论N1

在撰写本文的过程中，戳戳并尝试在此程序中进行操作对我来说变得很有趣。也许我很幸运，但是不久以后就会出现奇怪的行为：挂起，通过按Ctrl + Z来显示我对平板电脑的下垂和奇数平方的操作。不幸的是，我无法重复这种行为。

但是实际上，尽管有这种行为并养成了定期按Ctrl + S的习惯，但OpenToonz的规模和功能给人留下了深刻的印象。不过，大型工作室也不会使用它。

和我的艺术作为奖励：

结束语N2

对于OpenToonz，很明显，立即修复分析器检测到的所有错误将是一项艰巨的任务，它将使开发过程停滞不前。对于这种情况，存在“质量抑制”方法，即将技术债务输入到分析仪的抑制器基座中，并根据新的响应与分析仪进行进一步的工作。好吧，如果时间到了，那么您可以解决技术债务。

PS：我提醒您，开源项目的开发人员可以使用免费许可选项PVS-Studio。如果您想与说英语的读者分享这篇文章，请使用翻译链接：Victoria Khanieva。OpenToonz。

OpenToonz：由内而外

More articles: