💚 👩🏼‍💻 🤚 OpenToonz: por dentro y por fuera 👋🏾 🏁 🌘

Han pasado casi cuatro años desde que PVS-Studio verificó el código fuente de OpenToonz. Este proyecto es una herramienta muy poderosa para crear animaciones bidimensionales. Desde el último control, con su ayuda, se crearon obras animadas como Mary and the Witch Flower, Batman Ninja, Promare y otras. Dado que los grandes estudios continúan usando Toonz, ¿por qué no verificar la calidad del código fuente nuevamente?

Puede familiarizarse con el análisis anterior de errores en el artículo " Código incorrecto de un paquete para crear animaciones 2D de Toonz ". En general, no parece que la calidad del código haya mejorado mucho, por lo que la impresión general no es mejor. Además, se encontraron muchos de los mismos errores que en el artículo anterior. No los volveremos a considerar, ya que hay muchas cosas para elegir.

Sin embargo, debe comprender que la presencia de errores no necesariamente impide el uso activo y productivo del producto de software. Lo más probable es que los errores encontrados se encuentren en vivo en partes del código raramente utilizadas o completamente no utilizadas, de lo contrario habrían sido identificadas en el proceso de uso de la aplicación y corregidas. Sin embargo, esto no significa que el análisis estático sea redundante. Es solo que el significado del análisis estático no es buscar errores antiguos e irrelevantes, sino abaratar el proceso de desarrollo. Muchos errores al escribir código se pueden detectar de inmediato, y no durante la operación del software. En consecuencia, con el uso regular de un analizador estático, los errores se corrigen en una etapa temprana. Esto ahorra a los desarrolladores tiempo y dinero de la empresa, y mejora la percepción del producto por parte de los usuarios. De acuerdo, es desagradable darse de baja constantemente a los desarrolladores,que una cosa u otra no funciona.

Fragmento N1

V610 Comportamiento indefinido. Verifique el operador de turno '<<'. El operando izquierdo '(- 1)' es negativo.

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

No está claro lo que el autor del código quería lograr aquí. El uso de operadores de turno con números negativos conduce a un comportamiento indefinido. La forma en que se describe el comportamiento de los operadores de cambio en el estándar parece un poco confuso a primera vista, pero, sin embargo, verifiquemos:

1. El tipo de resultado es el del operando izquierdo promovido. El comportamiento es indefinido si el operando derecho es negativo, o mayor o igual que la longitud en bits del operando izquierdo promovido.

2. El valor de E1 << E2 es E1 posiciones de bit E2 desplazadas a la izquierda; los bits vacantes están llenos de cero. Si E1 tiene un tipo sin signo, el valor del resultado es E1 * 2 ^ E2, módulo reducido uno más que el valor máximo representable en el tipo de resultado. De lo contrario, si E1 tiene un tipo con signo y un valor no negativo, y E1 * 2 ^ E2 es representable en el tipo de resultado, entonces ese es el valor resultante; de lo contrario, el comportamiento es indefinido.

Por lo tanto, el comportamiento no se define si el operando derecho o izquierdo tiene un valor negativo. Si el operando es de tipo con signo, valor no negativo y se ajusta al tipo resultante, entonces el comportamiento será normal.

Fragmento N2

V517El uso del patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 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;
}

Aquí a la variable m_offset se le asignan diferentes valores dependiendo del valor de m_currentItem . Sin embargo, duplicar la verificación en BlackSlider no tiene sentido, y desde el cuerpo de la condición puede ver que la variable m_white está involucrada en el cálculo . Veamos los valores posibles para m_currentItem .

  LevelControlItem m_currentItem;

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

Resulta que el valor WhiteSlider también es posible , para lo cual la verificación simplemente no se realiza. Por lo tanto, es completamente posible que algunos de los escenarios de comportamiento se hayan perdido debido al error de copiar y pegar.

Fragmento N3

V517 El uso del patrón 'if (A) {...} else if (A) {...}' fue detectado. Hay una probabilidad de presencia de error lógico. Líneas de verificación: 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;
        ....
      }
    } 
      ....
  }
}

Otro error similar. Aquí, las mismas condiciones tienen cuerpos diferentes, pero ya no es posible concluir sobre las posibles opciones para el valor tagName . Lo más probable es que se haya perdido alguna opción, y al final tenemos un código que nunca se ejecutará.

Fragmento N4

V547 La expresión 'chancount == 2' siempre es verdadera. 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) {
  ....
}

Un pequeño error lógico se deslizó en estas comprobaciones. En la prueba bajo el número uno, el número de cambio se compara con 1, y en la prueba número 2 se verifica que esta variable sea menor o igual a 2. Como resultado, el único valor de cambio posible es 2 a la condición bajo el número 3. Este error puede no conducir al funcionamiento incorrecto del programa, pero complica la lectura y comprensión del código. Por ejemplo, no está claro por qué la rama de otra cosa entonces ...

En general, la función considerada en este fragmento toma un poco más de 300 líneas de código y consiste en tales montones de condiciones y bucles.

Fragmento N5

V614 Variable no inicializada 'precSegmentIndex' utilizada. Considere verificar el quinto argumento real de la función '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;
  ....
}

Quizás, se cometió un error aquí incluso al inicializar las variables precSegmentIndex , currentSegmentIndex , startSegmentIndex , precChunkIndex . El desarrollador podría esperar que la inicialización del último elemento -1 se inicialice con el mismo valor que otras variables declaradas en la misma línea.

Fragmento N6

V590 Considere la posibilidad de inspeccionar la expresión 's! = "" && s == "color"'. La expresión es excesiva o contiene un error de imprenta. 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") {
    ....  
  }
  ....
}

Este error, incluso más bien un defecto, en sí mismo solo conduce a una comparación innecesaria. Sin embargo, si miramos el código como un todo, quedará claro que la comparación adicional apareció como resultado de copiar y pegar de las condiciones anteriores.

Tales fideos, que ocupan docenas o más líneas de código, pueden contener cualquier otro error lógico, y su búsqueda con este formato puede convertirse en un tormento.

Fragmento N7

V772 Llamar a un operador 'eliminar' para un puntero nulo causará un comportamiento indefinido. pluginhost.cpp 1327

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

Aquí el mensaje del analizador en sí mismo es bastante exhaustivo: llamar al operador de eliminación en un puntero a un tipo nulo conduce a un comportamiento indefinido. Si se necesitara una función universal para eliminar las interfaces, podría valer la pena convertirla en un repetitivo.

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

Fragmento N8

V568 Es extraño que el operador 'sizeof ()' evalúe el tamaño de un puntero a una clase, pero no el tamaño del objeto de clase '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);
  }
  ....
}

Un error bastante común que puede ocurrir tanto por falta de atención como por ignorancia. Aquí, lo más probable, el asunto fue la falta de atención, ya que en el primer término, sin embargo, esto fue desreferenciado. Si necesita el tamaño de un objeto, siempre debe recordar que el puntero de un objeto debe estar desreferenciado. De lo contrario, solo obtendremos el tamaño del puntero.

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

Fragmento N9

V568 Es extraño que el operador 'sizeof ()' evalúe el tamaño de un puntero a una clase, pero no el tamaño del objeto de clase '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);
  }
};

Y aquí, obviamente, se olvidaron de desreferenciar este puntero . Como resultado, no obtenemos el tamaño del objeto, sino el tamaño del puntero. Como resultado, solo se comparan los primeros 4 u 8 bytes (dependiendo de la profundidad de bits). La versión correcta del código:

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

Fragmento N10

V554 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará utilizando 'eliminar'. 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]);
....
}

A menudo se olvida que el tipo de instancia de unique_ptr depende de si se usará delete o delete []. Como resultado, si crea una instancia del puntero como en el fragmento en consideración, mientras asigna memoria a través de un nuevo [], puede producirse un comportamiento indefinido, ya que la liberación se producirá mediante la eliminación. Para evitar esto, debe agregar corchetes al tipo de puntero: std :: unique_ptr <char []>.

Fragmento N11

V521 Estas expresiones que usan el operador ',' son peligrosas. Asegúrese de que la expresión 'm_to, m_from = it-> first.getNumber ()' sea correcta. 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;
  ....
}

Quizás el programador esperaba que pudieras asignar un valor a varias variables simplemente escribiéndolas con una coma. Sin embargo, el operador "," en C ++ funciona de manera diferente. En él, se ejecuta el primer operando y el resultado se "restablece", luego se calcula el segundo operando. Y, aunque la variable m_to se inicializa en un ciclo posterior, si algo sale mal o alguien refactoriza incorrectamente, es posible que m_to no reciba un valor. Y en general, en cualquier caso, este código parece extraño.

Fragmento N12

V532 Considere inspeccionar la declaración del patrón '* puntero ++'. Probablemente significaba: '(* puntero) ++'. 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++; // <=
    }
  }
}

Una pequeña falla, que puede confundir aún más a la persona que lee el código. El incremento, según lo previsto, desplaza el puntero. Sin embargo, después de esto hay una desreferenciación sin sentido. Es mejor simplemente escribir pix ++ .

Fragmento N13

V773 Se salió de la función sin soltar el puntero 'autoCloseUndo'. Una pérdida de memoria es posible. vectortapetool.cpp 575

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

Hubo más de 20 advertencias de este tipo. A menudo, en algún lugar al final de la función, se libera la memoria, pero para el retorno anterior se omite este paso necesario. Entonces está aquí. Al final, el puntero se pasa a TUndoManager :: manager () -> add () , que se encarga de eliminar el puntero, pero hay un retorno anterior por el que olvidó llamar a este método. Por lo tanto, siempre vale la pena recordar sus punteros cada vez que salga de la función, y no solo ingrese la eliminación en algún lugar al final del bloque o antes del último retorno .

Sin embargo, si para una versión abreviada del código este error parece obvio, entonces en un código real y más complicado, rastrear tal problema puede ser difícil. Aquí el analizador estático siempre cansado ayudará.

Fragmento N14

V522 Puede tener lugar la desreferenciación de la 'región' de puntero nulo. 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;
  }
  ....
}

Aquí podemos suponer que el desarrollador mezcló las reglas de la evaluación de cortocircuito y pensó que si la primera comprobación del puntero devuelve falso , no se eliminará la referencia de dicho puntero nulo. Sin embargo, para el operador "||" Es todo lo contrario.

Fragmento N15

V561 Probablemente sea mejor asignar valor a la variable 'ca' que declararlo de nuevo. Declaración anterior: xshcellmover.cpp, línea 319. xshcellmover.cpp 323

V561 Probablemente sea mejor asignar valor a la variable 'cb' que declararlo de nuevo. Declaración previa: xshcellmover.cpp, línea 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) {
    ....
  }
  ....
}

Quizás otro copiar y pegar, pero con una esencia no muy común del error. La llamada a x fue reemplazada por y , pero se olvidaron de eliminar el tipo de la variable al comienzo de la línea, debido a lo cual ocurre la notificación local. Como resultado, en lugar de cambiar la orientación de posición para el ca y cb inicial , se crean nuevos ca y cb locales , con lo cual no sucede nada más. Pero ca y cb externos siguen existiendo con valores para x .

Conclusión N1

En el proceso de escribir el artículo, me resultó interesante tocar e intentar hacer algo en este programa. Tal vez tuve suerte, pero el comportamiento extraño no tarda en llegar: colgar, me muestra la manipulación de la tableta después de la flacidez y la plaza extraña pulsando las teclas Ctrl + Z . Lamentablemente, no pude repetir este comportamiento.

Pero, de hecho, a pesar de este comportamiento e inculcar el hábito de presionar regularmente Ctrl + S , OpenToonz impresiona con su escala y funcionalidad. Aún así, no es en vano que los grandes estudios también lo usen.

Y mi arte como extra:

Conclusión N2

En el caso de OpenToonz, es obvio que tratar de corregir todos los errores detectados por el analizador a la vez será una gran tarea que detendrá el proceso de desarrollo. Para tales casos, existe el enfoque de “Supresión masiva”, cuando la deuda técnica se ingresa en la base supresora del analizador y el trabajo adicional con el analizador se lleva a cabo sobre la base de nuevas respuestas. Bueno, si aparece el tiempo, puede resolver la deuda técnica.

PD: Les recuerdo que los desarrolladores de proyectos de código abierto pueden usar la opción de licencia gratuita PVS-Studio . Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Victoria Khanieva. OpenToonz .

OpenToonz: por dentro y por fuera

More articles: