Get best of the week

OpenToonz: Innen und außen

image1.png

Fast vier Jahre sind vergangen, seit PVS-Studio den OpenToonz-Quellcode überprüft hat. Dieses Projekt ist ein sehr leistungsfähiges Werkzeug zum Erstellen zweidimensionaler Animationen. Seit der letzten Überprüfung wurden mit seiner Hilfe animierte Werke wie Mary und die Hexenblume, Batman Ninja, Promare und andere geschaffen. Da große Studios weiterhin Toonz verwenden, sollten Sie die Qualität des Quellcodes erneut überprüfen.

Sie können sich mit der vorherigen Fehleranalyse im Artikel " Schlechter Code eines Pakets zum Erstellen von 2D-Animationen von Toonz " vertraut machen . Im Allgemeinen scheint sich die Qualität des Codes nicht wesentlich verbessert zu haben, sodass der Gesamteindruck nicht besser ist. Darüber hinaus wurden viele der gleichen Fehler wie im vorherigen Artikel gefunden. Wir werden sie nicht noch einmal betrachten, da es viele Dinge zur Auswahl gibt.

Sie müssen jedoch verstehen, dass das Vorhandensein von Fehlern die aktive und produktive Verwendung des Softwareprodukts nicht unbedingt verhindert. Höchstwahrscheinlich leben die gefundenen Fehler in selten verwendeten oder vollständig nicht verwendeten Teilen des Codes, da sie sonst bei der Verwendung der Anwendung identifiziert und behoben worden wären. Dies bedeutet jedoch nicht, dass die statische Analyse redundant ist. Es ist nur so, dass die Bedeutung der statischen Analyse nicht darin besteht, nach alten und irrelevanten Fehlern zu suchen, sondern den Entwicklungsprozess zu verbilligen. Viele Fehler beim Schreiben von Code können sofort und nicht während des Betriebs der Software erkannt werden. Dementsprechend werden bei regelmäßiger Verwendung eines statischen Analysators Fehler frühzeitig korrigiert. Dies spart sowohl Entwicklern Zeit als auch Unternehmensgeld und verbessert die Wahrnehmung des Produkts durch die Benutzer. Stimmen Sie zu, es ist unangenehm, Entwickler ständig abzumelden.das eine oder andere funktioniert nicht.

Fragment N1

V610 Undefiniertes Verhalten. Überprüfen Sie den Schaltoperator '<<'. Der linke Operand '(- 1)' ist negativ.

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

Es ist nicht klar, was der Code-Autor hier erreichen wollte. Die Verwendung von Schichtoperatoren mit negativen Zahlen führt zu undefiniertem Verhalten. Die Art und Weise, wie das Verhalten der Schichtoperatoren im Standard beschrieben wird, erscheint auf den ersten Blick etwas verwirrend, überprüfen Sie es jedoch dennoch:

1. Der Typ des Ergebnisses ist der des heraufgestuften linken Operanden. Das Verhalten ist undefiniert, wenn der rechte Operand negativ ist oder größer oder gleich der Länge in Bits des heraufgestuften linken Operanden ist.

2. Der Wert von E1 << E2 ist E1 linksverschobene E2-Bitpositionen; Leerzeichen werden mit Null gefüllt. Wenn E1 einen vorzeichenlosen Typ hat, ist der Wert des Ergebnisses E1 * 2 ^ E2, modulo um eins mehr als der im Ergebnistyp darstellbare Maximalwert. Andernfalls ist dies der resultierende Wert, wenn E1 einen vorzeichenbehafteten Typ und einen nicht negativen Wert hat und E1 * 2 ^ E2 im Ergebnistyp darstellbar ist. Andernfalls ist das Verhalten undefiniert.

Das Verhalten ist also nicht definiert, wenn der rechte oder linke Operand einen negativen Wert hat. Wenn der Operand vom Typ mit Vorzeichen und nicht negativem Wert ist und in den resultierenden Typ passt, ist das Verhalten normal.

Fragment N2

V517Die Verwendung des Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit, dass ein logischer Fehler vorliegt. Überprüfen Sie die Zeilen: 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;
}

Hier werden der Variablen m_offset je nach Wert von m_currentItem unterschiedliche Werte zugewiesen . Das Duplizieren der Prüfung in BlackSlider ist jedoch sinnlos, und anhand des Zustands der Bedingung können Sie erkennen, dass die Variable m_white an der Berechnung beteiligt ist . Schauen wir uns die möglichen Werte für m_currentItem an .

  LevelControlItem m_currentItem;

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

image3.png

Es stellt sich heraus, dass auch der WhiteSlider- Wert möglich ist , für den keine Überprüfung durchgeführt wird. Daher ist es durchaus möglich, dass einige der Verhaltensszenarien aufgrund eines Fehlers beim Kopieren und Einfügen verloren gingen.

Fragment N3

V517 Die Verwendung des Musters 'if (A) {...} else if (A) {...}' wurde erkannt. Es besteht die Wahrscheinlichkeit, dass ein logischer Fehler vorliegt. Überprüfen Sie die Zeilen: 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;
        ....
      }
    } 
      ....
  }
}

Ein weiterer ähnlicher Fehler. Hier haben dieselben Bedingungen unterschiedliche Körper, aber es ist nicht mehr möglich, auf die möglichen Optionen für den Wert tagName zu schließen . Höchstwahrscheinlich wurde nur eine Option übersehen, und am Ende haben wir Code, der niemals ausgeführt wird.

Fragment N4

V547 Der Ausdruck 'chancount == 2' ist immer wahr. 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) {
  ....
}

Ein kleiner logischer Fehler hat sich in diese Prüfungen eingeschlichen. Im Test unter Nummer eins wird der Chancount mit 1 verglichen, und im Test unter Nummer 2 wird überprüft, ob diese Variable kleiner oder gleich 2 ist. Infolgedessen ist der einzig mögliche Chancount- Wert 2 für die Bedingung unter Nummer 3. Dieser Fehler führt möglicherweise nicht zu einer fehlerhaften Programmoperation. Dies erschwert jedoch das Lesen und Verstehen des Codes. Zum Beispiel ist nicht klar, warum der else-Zweig dann ...

Im Allgemeinen benötigt die in diesem Fragment betrachtete Funktion etwas mehr als 300 Codezeilen und besteht aus solchen Haufen von Bedingungen und Schleifen.

image4.png

Fragment N5

V614 Nicht initialisierte Variable 'precSegmentIndex' verwendet. Überprüfen Sie das fünfte tatsächliche Argument der Funktion 'insertBoxCorners'. 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;
  ....
}

Möglicherweise wurde hier sogar während der Initialisierung der Variablen precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex ein Fehler gemacht . Der Entwickler könnte erwarten, dass die Initialisierung des letzten Elements -1 mit demselben Wert wie andere in derselben Zeile deklarierte Variablen initialisiert wird.

Fragment N6

V590 Überprüfen Sie den Ausdruck 's! = "" && s == "color"'. Der Ausdruck ist übertrieben oder enthält einen Druckfehler. 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") {
    ....  
  }
  ....
}

Dieser Fehler, auch eher ein Fehler, führt an sich nur zu einem unnötigen Vergleich. Wenn wir jedoch den Code als Ganzes betrachten, wird klar, dass der zusätzliche Vergleich als Ergebnis des Kopierens und Einfügens aus den vorherigen Bedingungen aufgetreten ist.

image5.png

Solche Nudeln, die Dutzende oder mehr Codezeilen belegen, können durchaus andere logische Fehler enthalten, und ihre Suche mit dieser Formatierung kann zu Qualen führen.

Fragment N7

V772 Das Aufrufen eines Löschoperators für einen ungültigen Zeiger führt zu undefiniertem Verhalten. pluginhost.cpp 1327

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

Hier ist die Analysatormeldung selbst bereits ziemlich vollständig: Das Aufrufen des Löschoperators für einen Zeiger auf einen ungültigen Typ führt zu undefiniertem Verhalten. Wenn eine universelle Funktion zum Entfernen von Schnittstellen benötigt wurde, kann es sich lohnen, sie zu einer Boilerplate zu machen.

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

Fragment N8

V568 Es ist seltsam, dass der Operator 'sizeof ()' die Größe eines Zeigers auf eine Klasse auswertet, nicht jedoch die Größe des Klassenobjekts '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);
  }
  ....
}

Ein Fehler, der häufig genug ist und sowohl durch Unaufmerksamkeit als auch durch Unwissenheit auftreten kann. Hier, wahrscheinlich war es eine Frage der Unaufmerksamkeit, da im ersten Term dies wurde jedoch dereferenziert. Wenn Sie die Größe eines Objekts benötigen, sollten Sie immer daran denken, dass der Zeiger eines Objekts dereferenziert werden muss. Ansonsten erhalten wir nur die Größe des Zeigers selbst.

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

Fragment N9

V568 Es ist seltsam, dass der Operator 'sizeof ()' die Größe eines Zeigers auf eine Klasse auswertet, nicht jedoch die Größe des Klassenobjekts '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);
  }
};

Und hier haben sie offensichtlich vergessen, diesen Zeiger zu dereferenzieren . Als Ergebnis erhalten wir nicht die Größe des Objekts, sondern die Größe des Zeigers. Infolgedessen werden nur die ersten 4 oder 8 Bytes verglichen (abhängig von der Bittiefe). Die richtige Version des Codes:

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

Fragment N10

V554 Falsche Verwendung von unique_ptr. Der mit 'new []' zugewiesene Speicher wird mit 'delete' bereinigt. screenavermaker.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]);
....
}

Es wird oft vergessen, dass der Typ, mit dem unique_ptr instanziiert wird, davon abhängt, ob delete oder delete [] verwendet wird. Wenn Sie den Zeiger wie im betrachteten Fragment instanziieren, während Sie Speicher durch new [] zuweisen, kann dies zu undefiniertem Verhalten führen, da die Freigabe durch Löschen erfolgt. Um dies zu vermeiden, müssen Sie dem Zeigertyp eckige Klammern hinzufügen: std :: unique_ptr <char []>.

Fragment N11

V521 Solche Ausdrücke mit dem Operator ',' sind gefährlich. Stellen Sie sicher, dass der Ausdruck 'm_to, m_from = it-> first.getNumber ()' korrekt ist. 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;
  ....
}

Vielleicht hat der Programmierer erwartet, dass Sie mehreren Variablen einen Wert zuweisen können, indem Sie sie einfach mit einem Komma ausschreiben. Der Operator "," in C ++ funktioniert jedoch anders. Darin wird der erste Operand ausgeführt und das Ergebnis wird "zurückgesetzt", dann wird der zweite Operand berechnet. Und obwohl die Variable m_to in einem nachfolgenden Zyklus initialisiert wird, ist es möglich, dass m_to keinen Wert erhält , wenn etwas schief geht oder jemand ungenau umgestaltet . Und im Allgemeinen sieht dieser Code auf jeden Fall seltsam aus.

Fragment N12

V532 Überprüfen Sie die Anweisung des Musters '* pointer ++'. Vermutlich gemeint: '(* Zeiger) ++'. 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++; // <=
    }
  }
}

Ein kleiner Fehler, der die Person, die den Code liest, weiter verwirren kann. Das Inkrement verschiebt wie beabsichtigt den Zeiger. Danach kommt es jedoch zu einer bedeutungslosen Dereferenzierung. Besser einfach pix ++ schreiben .

Fragment N13

V773 Die Funktion wurde beendet, ohne den Zeiger 'autoCloseUndo' loszulassen. Ein Speicherverlust ist möglich. vectortapetool.cpp 575

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

Es gab mehr als 20 solcher Warnungen. Oft wird irgendwo am Ende der Funktion der Speicher freigegeben, aber für eine frühere Rückgabe wird dieser notwendige Schritt übersprungen. So ist es hier. Am Ende wird der Zeiger an TUndoManager :: manager () -> add () übergeben , das das Löschen des Zeigers übernimmt. Es gibt jedoch eine Rückgabe, für die Sie vergessen haben, diese Methode aufzurufen. Es lohnt sich also immer, sich Ihre Zeiger zu merken, wenn Sie die Funktion verlassen, und nicht nur irgendwo am Ende des Blocks oder vor der letzten Rückkehr den Löschvorgang einzugeben .

Wenn jedoch für eine abgekürzte Version des Codes dieser Fehler offensichtlich erscheint, kann es in realem, komplizierterem Code schwierig sein, ein solches Problem aufzuspüren. Hier hilft der immer müde statische Analysator.

Fragment N14

V522 Es kann zu einer Dereferenzierung der Nullzeiger-Region kommen. 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;
  }
  ....
}

Hier können wir davon ausgehen, dass der Entwickler die Regeln der Kurzschlussbewertung verwechselt hat und dachte, dass die Dereferenzierung eines solchen Nullzeigers nicht stattfinden wird , wenn die erste Überprüfung des Zeigers false zurückgibt . Für den Operator "||" es ist genau das Gegenteil.

Fragment N15

V561 Es ist wahrscheinlich besser, der Variablen 'ca' einen Wert zuzuweisen, als ihn erneut zu deklarieren. Vorherige Deklaration: xshcellmover.cpp, Zeile 319. xshcellmover.cpp 323

V561 Es ist wahrscheinlich besser, der Variablen 'cb' einen Wert zuzuweisen, als ihn erneut zu deklarieren. Vorherige Deklaration: xshcellmover.cpp, Zeile 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) {
    ....
  }
  ....
}

Vielleicht noch ein Copy-Paste, aber mit einer nicht ganz gewöhnlichen Essenz des Fehlers. Der Aufruf von x wurde durch y ersetzt , aber sie haben vergessen, den Typ der Variablen am Anfang der Zeile zu entfernen, aufgrund dessen eine lokale Neumeldung erfolgt. Infolgedessen werden anstelle einer Änderung der Positionsausrichtung für das anfängliche ca und cb neue lokale ca und cb erstellt , mit denen nichts weiter passiert. Externe ca und cb existieren jedoch weiterhin mit Werten für x .

Schlussfolgerung N1

Während ich den Artikel schrieb, wurde es für mich interessant, in diesem Programm zu stöbern und zu versuchen, etwas zu tun. Vielleicht hatte ich Glück, aber das seltsame Verhalten ließ nicht lange auf sich warten: Hängen Sie auf, zeigen Sie meine Manipulation des Tablets nach dem Durchhängen und dem ungeraden Quadrat an, indem Sie Strg + Z drücken . Leider konnte ich dieses Verhalten nicht wiederholen.

image6.png

Trotz dieses Verhaltens und der Gewohnheit, regelmäßig Strg + S zu drücken, beeindruckt OpenToonz mit seiner Größe und Funktionalität. Dennoch ist es nicht umsonst, dass auch große Studios es nutzen.

Und meine Kunst als Bonus:

image7.gif

Schlussfolgerung N2

Im Fall von OpenToonz ist es offensichtlich, dass der Versuch, alle vom Analysegerät erkannten Fehler auf einmal zu beheben, eine große Aufgabe ist, die den Entwicklungsprozess zum Stillstand bringt. In solchen Fällen gibt es den Ansatz der „Massenunterdrückung“, bei dem technische Schulden in die Suppressorbasis des Analysators eingegeben werden und die weitere Arbeit mit dem Analysator auf der Grundlage neuer Antworten durchgeführt wird. Wenn Zeit erscheint, können Sie die technischen Schulden klären.

PS Ich erinnere Sie daran, dass Entwickler von Open Source-Projekten die kostenlose Lizenzierungsoption PVS-Studio verwenden können . Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Übersetzungslink: Victoria Khanieva. OpenToonz .



More articles:


All Articles