👨🏽‍🤝‍👨🏻 ✖️ 🤙🏽 OpenToonz: à l'intérieur et à l'extérieur 🍗 🎎 🛌

Près de quatre ans se sont écoulés depuis que PVS-Studio a vérifié le code source d'OpenToonz. Ce projet est un outil très puissant pour créer une animation en deux dimensions. Depuis le dernier contrôle, avec son aide, des œuvres d'animation telles que Mary et la fleur de sorcière, Batman Ninja, Promare et d'autres ont été créées. Étant donné que les grands studios continuent d'utiliser Toonz, pourquoi ne pas vérifier à nouveau la qualité du code source?

Vous pouvez vous familiariser avec l'analyse précédente des erreurs dans l'article " Mauvais code d'un package pour créer des animations 2D de Toonz ". En général, il ne semble pas que la qualité du code se soit beaucoup améliorée, donc l'impression générale n'est pas meilleure. En outre, bon nombre des mêmes erreurs ont été trouvées que dans l'article précédent. Nous ne les examinerons pas à nouveau, car il y a beaucoup de choix.

Cependant, vous devez comprendre que la présence d'erreurs n'empêche pas nécessairement l'utilisation active et productive du produit logiciel. Très probablement, les erreurs trouvées se trouvent dans des parties du code rarement utilisées ou complètement inutilisées, sinon elles auraient été identifiées dans le processus d'utilisation de l'application et corrigées. Cependant, cela ne signifie pas que l'analyse statique est redondante. C'est juste que le sens de l'analyse statique n'est pas de rechercher des erreurs anciennes et non pertinentes, mais de réduire le coût du processus de développement. De nombreuses erreurs lors de l'écriture de code peuvent être détectées immédiatement, et non pendant le fonctionnement du logiciel. En conséquence, avec l'utilisation régulière d'un analyseur statique, les erreurs sont corrigées à un stade précoce. Cela permet aux développeurs de gagner du temps et de l'argent pour l'entreprise et améliore la perception du produit par les utilisateurs. D'accord, il est désagréable de se désabonner constamment aux développeurs,qu'une chose ou l'autre ne fonctionne pas.

Fragment N1

V610 Comportement indéfini . Vérifiez l'opérateur de décalage «<<». L'opérande gauche '(- 1)' est négatif.

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

On ne sait pas exactement ce que l'auteur du code voulait réaliser ici. L'utilisation d'opérateurs de décalage avec des nombres négatifs conduit à un comportement indéfini. La façon dont le comportement des opérateurs de décalage est décrit dans la norme semble un peu déroutant à première vue, mais néanmoins, vérifions-le:

1. Le type du résultat est celui de l'opérande gauche promu. Le comportement n'est pas défini si l'opérande droit est négatif, ou supérieur ou égal à la longueur en bits de l'opérande gauche promu.

2. La valeur de E1 << E2 correspond aux positions de bits E2 décalées vers la gauche E2; les bits libérés sont remplis de zéro. Si E1 a un type non signé, la valeur du résultat est E1 * 2 ^ E2, modulo réduit un de plus que la valeur maximale représentable dans le type de résultat. Sinon, si E1 a un type signé et une valeur non négative et que E1 * 2 ^ E2 est représentable dans le type de résultat, alors c'est la valeur résultante; sinon, le comportement n'est pas défini.

Ainsi, le comportement n'est pas défini si l'opérande droit ou gauche a une valeur négative. Si l'opérande est de type signé, de valeur non négative et s'inscrit dans le type résultant, le comportement sera normal.

Fragment N2

V517L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifier les lignes: 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;
}

Ici, la variable m_offset reçoit des valeurs différentes en fonction de la valeur de m_currentItem . Cependant, la duplication de la vérification sur BlackSlider est inutile, et à partir du corps de la condition, vous pouvez voir que la variable m_white est impliquée dans le calcul . Examinons les valeurs possibles pour m_currentItem .

  LevelControlItem m_currentItem;

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

Il s'avère que la valeur WhiteSlider est également possible , pour laquelle la vérification n'est tout simplement pas effectuée. Ainsi, il est tout à fait possible que certains des scénarios de comportement aient été perdus en raison d'une erreur de copier-coller.

Fragment N3

V517 L'utilisation du modèle 'if (A) {...} else if (A) {...}' a été détectée. Il y a une probabilité de présence d'erreur logique. Vérifier les lignes: 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;
        ....
      }
    } 
      ....
  }
}

Une autre erreur similaire. Ici, les mêmes conditions ont des corps différents, mais il n'est plus possible de conclure sur les options possibles pour la valeur tagName . Très probablement, une option a été manquée, et à la fin, nous avons du code qui ne sera jamais exécuté.

Fragment N4

V547 L' expression 'chancount == 2' est toujours vraie. 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) {
  ....
}

Une petite erreur logique s'est glissée dans ces vérifications. Dans le test sous le numéro un, le nombre de changements est comparé à 1, et dans le test numéro 2, il est vérifié que cette variable est inférieure ou égale à 2. Par conséquent, la seule valeur de nombre de changements possible est 2 à la condition sous le numéro 3. Cette erreur peut ne pas conduire à un fonctionnement incorrect du programme, mais cela complique la lecture et la compréhension du code. Par exemple, on ne sait pas pourquoi la branche else alors ...

En général, la fonction considérée dans ce fragment prend un peu plus de 300 lignes de code et se compose de tels tas de conditions et de boucles.

Fragment N5

V614 Variable non initialisée 'precSegmentIndex' utilisée. Pensez à vérifier le cinquième argument réel de la fonction '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;
  ....
}

Peut-être, une erreur a été commise ici même lors de l'initialisation des variables precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex . Le développeur pouvait s'attendre à ce que l'initialisation du dernier élément -1 s'initialise avec la même valeur que les autres variables déclarées sur la même ligne.

Fragment N6

V590 Envisagez d'inspecter l' expression 's! = "" && s == "color"'. L'expression est excessive ou contient une erreur d'impression. 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") {
    ....  
  }
  ....
}

Cette erreur, même plutôt un défaut, ne conduit en soi qu'à une comparaison inutile. Cependant, si nous regardons le code dans son ensemble, il deviendra clair que la comparaison supplémentaire est apparue à la suite du copier-coller des conditions précédentes.

Ces nouilles, qui occupent des dizaines ou plus de lignes de code, peuvent bien contenir d'autres erreurs logiques, et leur recherche avec ce formatage peut se transformer en tourment.

Fragment N7

V772 L' appel d'un opérateur de suppression pour un pointeur vide entraînera un comportement indéfini. pluginhost.cpp 1327

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

Ici, le message de l'analyseur lui-même est déjà assez exhaustif: appeler l'opérateur de suppression sur un pointeur vers un type void conduit à un comportement indéfini. Si une fonction universelle était nécessaire pour supprimer les interfaces, cela pourrait valoir la peine d'en faire un passe-partout.

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

Fragment N8

V568 Il est étrange que l'opérateur 'sizeof ()' évalue la taille d'un pointeur vers une classe, mais pas la taille de l'objet de classe '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);
  }
  ....
}

Une erreur assez courante qui peut survenir à la fois par inattention et par ignorance. Ici, très probablement, la question était inattentive, car dans le premier mandat, elle était néanmoins déférencée. Si vous avez besoin de la taille d'un objet, vous devez toujours vous rappeler que le pointeur d'un objet doit être déréférencé. Sinon, nous obtenons simplement la taille du pointeur lui-même.

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

Fragment N9

V568 Il est étrange que l'opérateur 'sizeof ()' évalue la taille d'un pointeur vers une classe, mais pas la taille de l'objet de classe '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);
  }
};

Et ici, évidemment, ils ont oublié de déréférencer le pointeur this . Par conséquent, nous obtenons non pas la taille de l'objet, mais la taille du pointeur. Par conséquent, seuls les 4 ou 8 premiers octets sont comparés (en fonction de la profondeur de bits). La version correcte du code:

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

Fragment N10

V554 Utilisation incorrecte de unique_ptr. La mémoire allouée avec «nouveau []» sera nettoyée à l'aide de «supprimer». 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]);
....
}

On oublie souvent que le type par lequel unique_ptr est instancié dépend de la suppression ou de la suppression de []. Par conséquent, si vous instanciez le pointeur comme dans le fragment considéré, lors de l'allocation de mémoire via new [], un comportement indéfini peut se produire, car la libération se fera par suppression. Pour éviter cela, vous devez ajouter des crochets au type de pointeur: std :: unique_ptr <char []>.

Fragment N11

V521 De telles expressions utilisant l'opérateur «,» sont dangereuses. Assurez-vous que l'expression «m_to, m_from = it-> first.getNumber ()» est correcte. 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;
  ....
}

Peut-être que le programmeur s'attendait à ce que vous puissiez attribuer une valeur à plusieurs variables en les écrivant simplement avec une virgule. Cependant, l'opérateur "," en C ++ fonctionne différemment. Dans ce document, le premier opérande est exécuté et le résultat est "réinitialisé", puis le deuxième opérande est calculé. Et, bien que la variable m_to soit initialisée dans un cycle suivant, si quelque chose se passe mal ou que quelqu'un refacture de manière inexacte, il est possible que m_to ne reçoive pas de valeur. Et en général, en tout cas, ce code a l'air étrange.

Fragment N12

V532 Envisagez d'inspecter l'instruction du modèle '* pointer ++'. Signifiait probablement: '(* pointeur) ++'. 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++; // <=
    }
  }
}

Un petit défaut, qui peut encore plus troubler la personne lisant le code. L'incrément, comme prévu, déplace le pointeur. Cependant, après cela, il y a un déréférencement insignifiant. Mieux vaut simplement écrire pix ++ .

Fragment N13

V773 La fonction a été quittée sans relâcher le pointeur «autoCloseUndo». Une fuite de mémoire est possible. vectortapetool.cpp 575

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

Il y avait plus de 20 avertissements de ce type. Souvent, à la fin de la fonction, la mémoire est libérée, mais pour un retour plus tôt, cette étape nécessaire est ignorée. C'est donc ici. À la fin, le pointeur est passé à TUndoManager :: manager () -> add () , qui s'occupe de supprimer le pointeur, mais il y a un retour ci-dessus pour lequel vous avez oublié d'appeler cette méthode. Il est donc toujours utile de se souvenir de vos pointeurs chaque fois que vous quittez la fonction, et pas seulement d'entrer la suppression quelque part à la fin du bloc ou avant le dernier retour .

Cependant, si pour une version abrégée du code, cette erreur semble évidente, alors dans un code réel et plus compliqué, la recherche d'un tel problème peut être difficile. Ici, l'analyseur statique toujours fatigué vous aidera.

Fragment N14

V522 Le déréférencement de la «région» du pointeur nul pourrait avoir lieu. 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;
  }
  ....
}

Ici, nous pouvons supposer que le développeur a mélangé les règles de l'évaluation des courts-circuits et a pensé que si la première vérification du pointeur renvoie faux , le déréférencement d'un tel pointeur nul ne se produira pas. Cependant, pour l'opérateur "||" c'est tout le contraire.

Fragment N15

V561 Il vaut probablement mieux attribuer une valeur à la variable 'ca' que de la déclarer à nouveau. Déclaration précédente: xshcellmover.cpp, ligne 319. xshcellmover.cpp 323

V561 Il est probablement préférable d'attribuer une valeur à la variable 'cb' que de la déclarer à nouveau. Déclaration précédente: xshcellmover.cpp, ligne 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) {
    ....
  }
  ....
}

Peut-être un autre copier-coller, mais avec une essence de l'erreur pas très ordinaire. L'appel à x a été remplacé par y , mais ils ont oublié de supprimer le type de la variable au début de la ligne, en raison de laquelle un nouveau rapport local se produit. En conséquence, au lieu de changer l'orientation de la position pour le ca et le cb initiaux , de nouveaux ca et cb locaux sont créés , avec lesquels rien ne se passe. Mais ca et cb externes continuent d'exister avec des valeurs pour x .

Conclusion N1

Au cours de la rédaction de l'article, il est devenu intéressant pour moi de pousser et d'essayer de faire quelque chose dans ce programme. Peut - être que j'étais le comportement de la chance, mais étrange ne tarde pas à venir: hang, afficher ma manipulation de la tablette après l'affaissement et le carré étrange en appuyant sur les touches Ctrl + Z . Malheureusement, je n'ai pas pu répéter ce comportement.

Mais en fait, malgré ce comportement et inculquant l'habitude d'appuyer régulièrement sur Ctrl + S , OpenToonz impressionne par son échelle et ses fonctionnalités. Pourtant, ce n'est pas en vain que les grands studios l'utilisent également.

Et mon art en bonus:

Conclusion N2

Dans le cas d'OpenToonz, il est évident qu'essayer de corriger toutes les erreurs détectées par l'analyseur à la fois sera une grosse tâche qui bloquera le processus de développement. Pour de tels cas, il y a l'approche de «suppression de masse», lorsque la dette technique est entrée dans la base de suppresseur de l'analyseur et que des travaux supplémentaires avec l'analyseur sont effectués sur la base de nouvelles réponses. Eh bien, si le temps apparaît, vous pouvez régler la dette technique.

PS Je vous rappelle que les développeurs de projets open source peuvent utiliser l' option de licence gratuite PVS-Studio . Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Victoria Khanieva. OpenToonz .

OpenToonz: à l'intérieur et à l'extérieur

More articles: