Han pasado casi cuatro años desde que el equipo de 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. A medida que los grandes estudios continúan usando Toonz, ¿por qué no verificar nuevamente la calidad del código fuente?
La revisión de errores anterior está disponible en el siguiente artículo "El código de Toonz deja mucho que desear". La impresión general es bastante similar, ya que parece que la calidad del código no ha mejorado mucho. 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 mencionarse que los errores no impedirán necesariamente el uso activo y productivo de un producto de software. Lo más probable es que los errores encontrados vivan en secciones del código que rara vez se usan o que no se usan por completo; de lo contrario, se habrían identificado en el proceso de uso de la aplicación y se habrían solucionado. No obstante, esto no significa que el análisis estático sea redundante. Es solo que el significado del análisis estático no es encontrar errores antiguos e irrelevantes, sino reducir el costo del proceso de desarrollo. Muchos errores pueden revelarse justo al escribir el código, antes de la producción del software. En consecuencia, con el uso regular de un analizador estático, los errores se corrigen en una etapa temprana. Esto ahorra tiempo al desarrollador y dinero a la empresa, y mejora la experiencia del usuario. Probablemente estarías de acuerdo en que es desagradable molestar a los desarrolladores cada vez que una u otra cosa no funciona.
Fragmento N1
V610 Comportamiento indefinido. Compruebe 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;
....
}Las intenciones del autor no son muy claras en este fragmento. El uso de operadores de cambio con números negativos conduce a un comportamiento indefinido. El estándar ofrece una descripción un poco confusa del comportamiento de los operadores de turnos, pero aún así vamos a comprobarlo:
1. El tipo del resultado es el del operando izquierdo promocionado. El comportamiento no está definido si el operando derecho es negativo, o mayor o igual a la longitud en bits del operando izquierdo promocionado.
2. El valor de E1 <
Entonces, el comportamiento no está definido si el operando derecho o izquierdo tiene un valor negativo. Si el operando es del tipo con signo, tiene un valor no negativo y se ajusta al tipo resultante, entonces el comportamiento será normal.
Fragmento N2
V517 Se detectó el uso del patrón 'if (A) {...} else if (A) {...}'. Hay una probabilidad de presencia de error lógico. Verifique las líneas:156, 160. cameracapturelevelcontrol.cpp 156
Aquí el m_offset a la variable se le asignan diferentes valores dependiendo del valor de m_currentItem . Sin embargo, la verificación duplicada de BlackSlider no tiene sentido Como podemos ver en el cuerpo de la condición, el m_white variable está involucrada en el cálculo. Veamos los valores posibles para m_currentItem .
Resulta que el valor de WhiteSlider también es posible, mientras que la verificación de este valor no se realiza. Por lo tanto, es posible que algunos de los escenarios de comportamiento se hayan perdido debido a un error de copiar y pegar.
Fragmento N3
V517 Se detectó el uso del patrón 'if (A) {...} else if (A) {...}'. Hay una probabilidad de presencia de error lógico. Ver líneas:784, 867. tpalette.cpp 784
Otro error similar. Aquí, las mismas condiciones tienen diferentes cuerpos, pero ya es imposible concluir sobre las posibles opciones para el tagName valor. Lo más probable es que se haya perdido alguna opción y al final tengamos el código que nunca se ejecutará.
Fragmento N4
V547 La expresión 'chancount ==2' siempre es verdadera. psd.cpp 720
Un pequeño error lógico se deslizó en estos controles. En el cheque número uno, chancount se compara con 1 y la segunda verificación verifica si esta variable es menor o igual a 2. Finalmente, en cuanto a la tercera condición, el único valor posible de chancount es 2. Es posible que un error de este tipo no provoque un funcionamiento incorrecto del programa, pero complica la lectura y la comprensión del código. Por ejemplo, el propósito de la rama else no está claro...
En conjunto, la función considerada en este fragmento ocupa un poco más de 300 líneas de código y consiste en montones de condiciones y bucles.
Fragmento N5
V614 Se utilizó la variable no inicializada 'precSegmentIndex'. Considere verificar el quinto argumento real de la función 'insertBoxCorners'. selección de rasters.cpp 803
Quizás, el error aquí se cometió incluso al inicializar precSegmentIndex , índice de segmento actual , índice de segmento de inicio , precChunkIndex variables 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 inspeccionar la expresión 's !="" &&s =="color"'. La expresión es excesiva o contiene un error tipográfico. parámetros de limpieza.cpp 416
Este error, que es más bien una falla, 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 la pieza copiada y pegada de las condiciones anteriores.
Todo este lío desordenado que ocupa docenas o más líneas de código bien puede contener otros errores lógicos, y su búsqueda con este formato puede convertirse en un tormento.
Fragmento N7
V772 Llamar a un operador 'eliminar' para un puntero vacío provocará un comportamiento indefinido. pluginhost.cpp 1327
Aquí el mensaje del analizador en sí ya es bastante completo:llamada de eliminar operador para el puntero a void conduce a un comportamiento indefinido. Si el desarrollador necesitara una función universal para eliminar interfaces, podría valer la pena crearla como plantilla.
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 la clase 'm_xshHandle'. tstageobjectcmd.cpp 455
Un error bastante común que puede ocurrir tanto por falta de atención como por desconocimiento. Aquí, muy probablemente, fue una cuestión de desatención, ya que en el primer sumando este fue desreferenciado de todos modos. Si necesita el tamaño de un objeto, siempre debe recordar que el puntero a ese objeto debe ser desreferenciado. De lo contrario, solo obtenemos el tamaño del puntero.
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
Aparentemente, el autor olvidó quitar la referencia al puntero this . Como resultado, obtenemos el tamaño del puntero en lugar del tamaño del objeto. Como resultado, solo se comparan los primeros 4 u 8 bytes (dependiendo del bitness). Versión de código correcta:
Fragmento N10
V554 Uso incorrecto de unique_ptr. La memoria asignada con 'nuevo []' se limpiará con 'eliminar'. protector de pantalla.cpp 29
A menudo se olvida que dependiendo del tipo con el que se instancian unique_ptr, se utilizará delete o delete[]. Como resultado, si se crea una instancia del puntero como en el fragmento bajo consideración, mientras se asigna memoria a través de new[], puede ocurrir un comportamiento indefinido, ya que la liberación ocurrirá a través de delete. Para evitar esto, se deben agregar corchetes al tipo de puntero:std::unique_ptr
Fragmento N11
V521 Tales 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
Quizás el programador esperaba que se pudiera asignar un valor a varias variables simplemente escribiéndolas separadas por comas. Sin embargo, el operador "," funciona de manera diferente en C ++. Lo que sucede es que se ejecuta el primer operando y se descarta el resultado, luego se calcula el segundo operando. Aunque el m_to la variable se inicializa en el ciclo subsiguiente, si algo sale mal y alguien realiza una refactorización incorrecta, m_to podría no obtener el valor en absoluto. De todos modos, este código parece extraño.
Fragmento N12
V532 Considere inspeccionar la declaración del patrón '*pointer++'. Probablemente significó:'(*puntero)++'. trop.cpp 140
Un pequeño defecto, que puede confundir aún más a la persona que lee el código. Según lo previsto, el incremento desplaza el puntero, seguido de la desreferencia sin sentido. Lo mejor es 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
Hubo más de 20 de tales advertencias. A menudo, en algún lugar al final de la función, la memoria se libera. Sin embargo, para return anteriores casos se omitió este paso necesario. Lo mismo sucede aquí. Al final, el puntero se pasa a TUndoManager::manager()->add() que se encarga de liberar la memoria. Sin embargo, los autores olvidaron llamar a este método para el return arriba. Por lo tanto, siempre vale la pena recordar los punteros cada vez que salga de la función, y no solo escribir la eliminación en algún lugar al final del bloque o antes del último return .
Sin embargo, mientras que para una versión abreviada del código este error parece obvio, en un código realmente complicado puede ser difícil identificar tal problema. Aquí el siempre cansado analizador estático será de ayuda.
Fragmento N14
V522 Es posible que se elimine la referencia del puntero nulo 'región'. paletacmd.cpp 94
Aquí podemos suponer que el desarrollador mezcló las reglas de evaluación de cortocircuito y pensó que si la primera verificación del puntero devuelve falso, entonces no ocurrirá la desreferenciación 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 declararla de nuevo. Declaración anterior:xshcellmover.cpp, línea 319. xshcellmover.cpp 323
V561 Probablemente sea mejor asignar valor a la variable 'cb' que declararla de nuevo. Declaración anterior:xshcellmover.cpp, línea 320. xshcellmover.cpp 324xshcellmover.cpp 323
Probablemente, es otro caso de copiar y pegar, pero con una esencia no trivial del error. La llamada a x fue reemplazado por y , pero el autor olvidó eliminar el tipo de la variable al comienzo de la línea, por lo que se produce una nueva declaración local. Como resultado, en lugar de cambiar la orientación de la posición de la ca inicial y cb , nueva ca local y cb se crean, con lo cual no pasa nada más. Pero externo ca y cb continuar existiendo con valores para x .
Conclusión N1
En el proceso de escribir el artículo, se me hizo interesante jugar con este programa. Tal vez tuve suerte, pero el comportamiento extraño no se hizo esperar:colgó, luego mostró mis manipulaciones con la tableta después de volver al funcionamiento normal, seguido de un cuadrado extraño después de presionar Ctrl + Z . Desafortunadamente, no pude reproducir este comportamiento.
Pero, de hecho, a pesar de este comportamiento y del desarrollo del hábito de presionar regularmente Ctrl + S , OpenToonz impresiona por su escala y funcionalidad. Aún así, no en vano los grandes estudios también lo utilizan.
Aquí está mi arte como bonificación:
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 paralizará el proceso de desarrollo. Para tales casos, existe el enfoque de "Supresión de masas", cuando la deuda técnica entra en el analizador suprime la base y el trabajo adicional con el analizador se lleva a cabo sobre la base de nuevas respuestas. Bueno, si aparece el tiempo, entonces puede resolver la deuda técnica.
PD Les recuerdo que los desarrolladores de proyectos de código abierto pueden utilizar la opción de licencia gratuita de PVS-Studio.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;
} LevelControlItem m_currentItem;
enum LevelControlItem {
None = 0,
BlackSlider,
WhiteSlider,
GammaSlider,
ThresholdSlider,
Histogram,
NumItems
};
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;
....
}
}
....
}
}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) {
....
}
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;
....
}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") {
....
}
....
}
static void release_interface(void *interf) {
if (interf) delete interf;
}template<class T>
static void release_interface(T *interf) {
if (interf) delete interf;
}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);struct RectF {
GLfloat m_val[4];
....
bool operator==(const RectF &rect) const {
return (memcmp(m_val, rect.m_val, sizeof(this)) == 0);
}
};return (memcmp(m_val, rect.m_val, sizeof(*this)) == 0);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]);
....
}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;
....
}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++; // <=
}
}
}void joinLineToLine(....) {
....
UndoAutoclose *autoCloseUndo = 0;
....
autoCloseUndo = new UndoAutoclose(....);
....
if (pos < 0) return;
....
TUndoManager::manager()->add(autoCloseUndo);
}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;
}
....
}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) {
....
}
....
}
