🆑 🐧 😓 OpenToonz: Por Dentro e Por Fora 🙏 🔺 ♟️

Quase quatro anos se passaram desde que o PVS-Studio verificou o código fonte do OpenToonz. Este projeto é uma ferramenta muito poderosa para criar animação bidimensional. Desde a última verificação, com a ajuda dele, foram criados trabalhos animados como Maria e a Flor das Bruxas, Batman Ninja, Promare e outros. Como os grandes estúdios continuam usando o Toonz, por que não verificar a qualidade do código fonte novamente?

Você pode se familiarizar com a análise anterior de erros no artigo " Código incorreto de um pacote para criar animações 2D do Toonz ". Em geral, não parece que a qualidade do código tenha melhorado muito; portanto, a impressão geral não é melhor. Além disso, muitos dos mesmos erros foram encontrados no artigo anterior. Não os consideraremos novamente, pois há muitas coisas para escolher.

No entanto, você deve entender que a presença de erros não impede necessariamente o uso ativo e produtivo do produto de software. Muito provavelmente, os erros encontrados estão em partes do código raramente usadas ou completamente não utilizadas, caso contrário, eles teriam sido identificados no processo de uso do aplicativo e corrigidos. No entanto, isso não significa que a análise estática seja redundante. É apenas que o significado da análise estática não é encontrar erros antigos e irrelevantes, mas reduzir o custo do processo de desenvolvimento. Muitos erros ao escrever código podem ser detectados imediatamente, e não durante a operação do software. Consequentemente, com o uso regular de um analisador estático, os erros são corrigidos em um estágio inicial. Isso economiza tempo dos desenvolvedores e dinheiro da empresa e melhora a percepção do produto pelos usuários. Concordo, é desagradável cancelar a inscrição constantemente para desenvolvedores,que uma coisa ou outra não funciona.

Fragmento N1

V610 Comportamento indefinido. Verifique o operador de turno '<<'. O operando esquerdo '(- 1)' é negativo.

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

Não está claro o que o autor do código queria alcançar aqui. Usar operadores de turno com números negativos leva a um comportamento indefinido. A maneira como o comportamento dos operadores de turno é descrito no padrão parece um pouco confuso à primeira vista, mas ainda assim verifique:

1. O tipo de resultado é o do operando esquerdo promovido. O comportamento é indefinido se o operando direito for negativo ou maior ou igual ao comprimento em bits do operando esquerdo promovido.

2. O valor de E1 << E2 é E1 com posições de bits E2 com deslocamento à esquerda; bits desocupados são preenchidos com zero. Se E1 tiver um tipo não assinado, o valor do resultado será E1 * 2 ^ E2, módulo reduzido um a mais que o valor máximo representável no tipo de resultado. Caso contrário, se E1 tiver um tipo assinado e um valor não negativo, e E1 * 2 ^ E2 for representável no tipo de resultado, esse será o valor resultante; caso contrário, o comportamento é indefinido.

Portanto, o comportamento não é definido se o operando direito ou esquerdo tiver um valor negativo. Se o operando for do tipo assinado, com valor não negativo e se encaixar no tipo resultante, o comportamento será normal.

Fragmento N2

V517O uso do padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 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;
}

Aqui, a variável m_offset recebe valores diferentes, dependendo do valor de m_currentItem . No entanto, duplicar a verificação no BlackSlider é inútil e, a partir do corpo da condição, você pode ver que a variável m_white está envolvida no cálculo . Vejamos os valores possíveis para m_currentItem .

  LevelControlItem m_currentItem;

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

Acontece que o valor WhiteSlider também é possível , para o qual a verificação simplesmente não é realizada. Portanto, é perfeitamente possível que alguns dos cenários de comportamento tenham sido perdidos devido a um erro de copiar e colar.

Fragmento N3

V517 O uso do padrão 'if (A) {...} else if (A) {...}' foi detectado. Há uma probabilidade de presença de erro lógico. Verifique as linhas: 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;
        ....
      }
    } 
      ....
  }
}

Outro erro semelhante. Aqui, as mesmas condições têm corpos diferentes, mas não é mais possível concluir sobre as opções possíveis para o valor tagName . Provavelmente, apenas alguma opção foi perdida e, no final, temos um código que nunca será executado.

O fragmento N4

V547 A expressão 'número da conta == 2' é sempre verdadeira. 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) {
  ....
}

Um pequeno erro lógico entrou nessas verificações. No teste sob o número um, o número de canais é comparado com 1 e no número de teste 2, é verificado se essa variável é menor ou igual a 2. Como resultado, o único valor possível do número de canais é 2 para a condição sob o número 3. Esse erro pode não levar à operação incorreta do programa, mas complica a leitura e a compreensão do código. Por exemplo, não está claro por que o ramo else então ...

Em geral, a função considerada neste fragmento ocupa pouco mais de 300 linhas de código e consiste em montes de condições e loops.

Fragmento N5

V614 Variável não inicializada 'precSegmentIndex' usada. Considere verificar o quinto argumento real da função '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;
  ....
}

Talvez um erro tenha sido cometido aqui, mesmo durante a inicialização das variáveis precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex . O desenvolvedor pode esperar que a inicialização do último elemento -1 seja inicializada com o mesmo valor que outras variáveis declaradas na mesma linha.

Fragmento N6

V590 Considere inspecionar a expressão 's! = "" && s == "color"'. A expressão é excessiva ou contém uma impressão incorreta. 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") {
    ....  
  }
  ....
}

Esse erro, mesmo uma falha, leva em si apenas a uma comparação desnecessária. No entanto, se observarmos o código como um todo, ficará claro que a comparação extra apareceu como resultado da cópia e colagem das condições anteriores.

Esse macarrão, que ocupa dezenas ou mais linhas de código, pode muito bem conter outros erros de lógica, e sua pesquisa com essa formatação pode se transformar em tormento.

Fragmento N7

V772 Chamar um operador 'delete' para um ponteiro nulo causará um comportamento indefinido. pluginhost.cpp 1327

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

Aqui, a própria mensagem do analisador já é bastante exaustiva: chamar o operador de exclusão em um ponteiro para um tipo de nulo leva a um comportamento indefinido. Se uma função universal fosse necessária para remover interfaces, pode valer a pena torná-la um padrão.

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

Fragmento N8

V568 É estranho que o operador 'sizeof ()' avalie o tamanho de um ponteiro para uma classe, mas não o tamanho do objeto 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);
  }
  ....
}

Um erro bastante comum que pode ocorrer por desatenção e ignorância. Aqui, muito provavelmente, o assunto foi desatento, uma vez que, no primeiro mandato, isso foi desreferenciado. Se você precisar do tamanho de um objeto, lembre-se sempre de que o ponteiro de um objeto deve ser desreferenciado. Caso contrário, apenas obtemos o tamanho do ponteiro em si.

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

Fragmento N9

V568 É estranho que o operador 'sizeof ()' avalie o tamanho de um ponteiro para uma classe, mas não o tamanho do objeto 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);
  }
};

E aqui, obviamente, eles esqueceram de desreferenciar esse ponteiro . Como resultado, obtemos não o tamanho do objeto, mas o tamanho do ponteiro. Como resultado, apenas os primeiros 4 ou 8 bytes são comparados (dependendo da profundidade dos bits). A versão correta do código:

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

Fragmento N10

V554 Uso incorreto de unique_ptr. A memória alocada com 'new []' será limpa usando '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]);
....
}

É frequentemente esquecido que o tipo pelo qual unique_ptr é instanciado depende se delete ou delete [] será usado. Como resultado, se você instanciar o ponteiro como no fragmento em consideração, ao alocar memória através de um novo [], pode ocorrer um comportamento indefinido, pois a liberação ocorrerá através da exclusão. Para evitar isso, você precisa adicionar colchetes ao tipo de ponteiro: std :: unique_ptr <char []>.

Fragmento N11

V521 Tais expressões usando o operador ',' são perigosas. Certifique-se de que a expressão 'm_to, m_de = it-> first.getNumber ()' esteja correta. 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;
  ....
}

Talvez o programador esperasse que você pudesse atribuir um valor a várias variáveis simplesmente escrevendo-as com uma vírgula. No entanto, o operador "," no C ++ funciona de maneira diferente. Nele, o primeiro operando é executado e o resultado é "redefinido", e o segundo operando é calculado. E, embora a variável m_to seja inicializada em um ciclo subsequente, se algo der errado ou alguém refatorar incorretamente, é possível que m_to não receba um valor. E, em geral, em qualquer caso, esse código parece estranho.

Fragmento N12

V532 Considere examinar a instrução do padrão '* pointer ++'. Provavelmente significava: '(* ponteiro) ++'. 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++; // <=
    }
  }
}

Uma pequena falha, que pode confundir ainda mais a pessoa que está lendo o código. O incremento, como pretendido, muda o ponteiro. No entanto, após isso, há uma desreferenciação sem sentido. Melhor escrever apenas pix ++ .

Fragmento N13

V773 A função foi encerrada sem liberar o ponteiro 'autoCloseUndo'. É possível um vazamento de memória. vectortapetool.cpp 575

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

Havia mais de 20 avisos desse tipo. Freqüentemente, em algum lugar no final da função, a memória é liberada, mas para retorno anterior, essa etapa necessária é ignorada. Então é aqui. No final, o ponteiro é passado para TUndoManager :: manager () -> add () , que cuida da exclusão do ponteiro, mas há um retorno acima para o qual você esqueceu de chamar esse método. Portanto, sempre vale a pena lembrar seus ponteiros sempre que você sair da função e não apenas digitar a exclusão em algum lugar no final do bloco ou antes do último retorno .

No entanto, se, para uma versão abreviada do código, esse erro parecer óbvio, então, em código real e mais complicado, rastrear esse problema pode ser difícil. Aqui, o analisador estático, sempre cansado, ajudará.

Fragmento N14

V522 A desreferenciação do ponteiro nulo 'região' pode ocorrer. 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;
  }
  ....
}

Aqui, podemos assumir que o desenvolvedor misturou as regras da avaliação de curto-circuito e pensou que, se a primeira verificação do ponteiro retornar falsa , a desreferenciação de um ponteiro nulo não ocorrerá. No entanto, para o operador "||" é exatamente o contrário.

Fragmento N15

V561 Provavelmente é melhor atribuir valor à variável 'ca' do que declará-lo novamente. Declaração anterior: xshcellmover.cpp, linha 319. xshcellmover.cpp 323

V561 Provavelmente é melhor atribuir valor à variável 'cb' do que declará-lo novamente. Declaração anterior: xshcellmover.cpp, linha 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) {
    ....
  }
  ....
}

Talvez outra cópia e colagem, mas com uma essência não muito comum do erro. A chamada para x foi substituída por y , mas eles esqueceram de remover o tipo da variável no início da linha, devido ao qual ocorre o reporte local. Como resultado, em vez de alterar a orientação da posição para ca e cb inicial , novos ca e cb locais são criados , com os quais nada acontece mais. Mas ca e cb externos continuam a existir com valores para x .

Conclusão N1

No processo de escrever o artigo, tornou-se interessante para mim cutucar e tentar fazer algo neste programa. Talvez eu tive sorte, mas o comportamento estranho não tarda em chegar: cair, exposição minha manipulação do comprimido após a flacidez ea praça estranho pressionando as teclas Ctrl + Z . Infelizmente, não pude repetir esse comportamento.

Mas, de fato, apesar desse comportamento e instilando o hábito de pressionar Ctrl + S regularmente , o OpenToonz impressiona com sua escala e funcionalidade. Ainda assim, não é em vão que os grandes estúdios também o usam.

E minha arte como um bônus:

Conclusão N2

No caso do OpenToonz, é óbvio que tentar corrigir todos os erros detectados pelo analisador de uma só vez será uma grande tarefa que interromperá o processo de desenvolvimento. Para esses casos, existe a abordagem "Supressão de massa", quando a dívida técnica é inserida na base de supressores do analisador e o trabalho adicional com o analisador é realizado com base em novas respostas. Bem, se o tempo aparecer, você poderá resolver a dívida técnica.

PS Lembro que os desenvolvedores de projetos de código aberto podem usar a opção de licenciamento gratuita PVS-Studio . Se você deseja compartilhar este artigo com um público que fala inglês, use o link da tradução: Victoria Khanieva. OpenToonz .

OpenToonz: Por Dentro e Por Fora

More articles: