Nuestros lectores están creciendo, por lo que escribimos artículos una y otra vez que explican cómo usar correctamente la metodología de análisis de código estático. Consideramos muy importante explicar que las herramientas de análisis estático no deben usarse esporádicamente, sino regularmente. Una vez más, demostramos esto con un ejemplo práctico, volviendo a verificar el proyecto Godot.
Use analizadores regularmente
Al prepararme para hablar en la conferencia de desarrolladores de juegos, decidí obtener nuevos ejemplos de errores interesantes que la herramienta
PVS-Studio puede detectar. Para este propósito, se probaron varios motores de juego, uno de los cuales fue Godot. No encontré ningún error particularmente interesante para el informe, pero quería escribir un artículo sobre errores comunes. Estos errores demuestran muy bien la relevancia del uso regular de herramientas de análisis de código estático.
Cabe señalar que ya
verificamos este proyecto en 2015, y el autor trabajó con los errores que escribimos.
Aquí puedes ver el commit correspondiente.
Han pasado 3 años. El proyecto ha cambiado. El analizador PVS-Studio también ha cambiado y han aparecido nuevos diagnósticos. Por lo tanto, no es sorprendente que haya podido escribir fácil y rápidamente suficientes ejemplos de errores para escribir este artículo.
Sin embargo, algo más es importante. Al desarrollar Godot o cualquier otro proyecto, constantemente aparecen nuevos errores y se corrigen. Los errores no detectados "se asientan" en el código durante mucho tiempo, y luego se pueden detectar muchos de ellos al ejecutar el análisis de código estático. Debido a esto, a veces existe la falsa sensación de que los analizadores estáticos encuentran solo algunos errores poco interesantes en secciones de código poco utilizadas. Por supuesto, este es el caso si usa el analizador incorrectamente y lo ejecuta solo de vez en cuando, por ejemplo, poco antes del lanzamiento de la versión.
Sí, al escribir artículos, nosotros mismos realizamos verificaciones únicas de proyectos abiertos. Pero tenemos un objetivo diferente. Queremos demostrar las capacidades de un analizador de código para detectar defectos. Esta tarea tiene poco que ver con mejorar la calidad del código del proyecto en su conjunto y reducir los costos asociados con la corrección de errores.
Una vez más sobre lo principal. El objetivo del análisis de código estático no es encontrar errores antiguos. Estos viejos errores suelen ser insignificantes, de lo contrario habrían interferido con los usuarios y ya se habrían encontrado y corregido. El punto del análisis estático es corregir rápidamente los errores en el código recién escrito o modificado. Esto reduce el tiempo de depuración, el número de quejas de los usuarios y, en última instancia, reduce el presupuesto del proyecto desarrollado.
Ahora pasemos a los errores que los lectores de nuestras publicaciones adoran tanto.
Copiar y pegar errores
Veamos lo que noté al estudiar el informe PVS-Studio. Comenzaré con mi diagnóstico V501 favorito, que encuentra
errores en casi todos los proyectos que verificamos :).
Error N1virtual bool can_export(....) { .... if (!exists_export_template("uwp_" + platform_infix + "_debug.zip", &err) || !exists_export_template("uwp_" + platform_infix + "_debug.zip", &err)) { valid = false; r_missing_templates = true; } .... }
Advertencia de PVS-Studio: V501 CWE-570 Hay subexpresiones idénticas '! Exists_export_template ("uwp_" + platform_infix + "_debug.zip", & err)' a la izquierda y a la derecha de '||' operador export.cpp 1135
Error clásico de copiar y pegar. La llamada de función se ha copiado pero no editado. El nombre del segundo archivo a procesar debe terminar con "_release.zip".
Errores N2, N3 static String dump_node_code(SL::Node *p_node, int p_level) { .... if (bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW || bnode->statements[i]->type == SL::Node::TYPE_CONTROL_FLOW) { code += scode;
Advertencia de PVS-Studio: V501 CWE-570 Hay subexpresiones idénticas 'bnode-> declaraciones [i] -> type == SL :: Node :: TYPE_CONTROL_FLOW' a la izquierda y a la derecha de '||' operador test_shader_lang.cpp 183
void EditorSpinSlider::_notification(int p_what) { if (p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT || p_what == MainLoop::NOTIFICATION_WM_FOCUS_OUT) { if (grabbing_spinner) { Input::get_singleton()->set_mouse_mode(Input::MOUSE_MODE_VISIBLE); grabbing_spinner = false; grabbing_spinner_attempt = false; } } .... }
Advertencia de PVS-Studio: V501 CWE-570 Hay subexpresiones idénticas 'p_what == MainLoop :: NOTIFICATION_WM_FOCUS_OUT' a la izquierda y a la derecha de '||' operador editor_spin_slider.cpp 157
Creo que los errores son claramente visibles y no requieren ninguna explicación. Exactamente el mismo clásico Copy-Paste, como en el primer caso.
Error N4 String SoftBody::get_configuration_warning() const { .... Transform t = get_transform(); if ((ABS(t.basis.get_axis(0).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(1).length() - 1.0) > 0.05 || ABS(t.basis.get_axis(0).length() - 1.0) > 0.05)) { if (!warning.empty()) .... }
Advertencia de PVS-Studio: V501 CWE-570 Hay subexpresiones idénticas a la izquierda y a la derecha de '||' operador soft_body.cpp 399
Aquí la primera línea se copió dos veces. Pero el número del eje de coordenadas se cambió solo en la segunda línea. Y se olvidaron de editar la tercera línea. Esto no es más que el "
Efecto de última línea ".
Nota Por el momento, además del "Efecto de última línea", he hecho las siguientes observaciones interesantes: "
La función más peligrosa en el mundo C / C ++ ", "El
mal vive en funciones de comparación ". Y ahora haré el anuncio de un nuevo artículo, cuya redacción planeo hacer en un futuro cercano. El título de trabajo es "0, 1, 2". Debería ser interesante e instructivo. Los invito a suscribirse a uno de los canales para no perderse:
twitter ,
vk.com ,
Instagram ,
telegram y
rss "old school".
Error N5 void ScrollContainer::_notification(int p_what) { .... if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= h_scroll->get_minimum_size().x; .... }
Advertencia de PVS-Studio: V778 CWE-682 Se encontraron dos fragmentos de código similares. Quizás, este es un error tipográfico y se debe usar la variable 'v_scroll' en lugar de 'h_scroll'. scroll_container.cpp 249
Con respecto a este fragmento de código, no estoy completamente seguro de que haya un error. Sin embargo, estoy de acuerdo con el analizador en que el segundo bloque parece muy sospechoso. Lo más probable es que el código fue escrito usando Copy-Paste, y en el segundo bloque de texto se olvidaron de reemplazar
h_scroll con
v_scroll .
El código probablemente debería ser así:
if (h_scroll->is_visible_in_tree() && h_scroll->get_parent() == this) size.y -= h_scroll->get_minimum_size().y; if (v_scroll->is_visible_in_tree() && v_scroll->get_parent() == this) size.x -= v_scroll->get_minimum_size().x;
Error N6Otro caso en el que se copió un fragmento de código lo suficientemente grande y se modificó sin éxito. La línea de error está marcada por mi comentario "// <=".
void ShaderGLES2::bind_uniforms() { .... const Map<uint32_t, Variant>::Element *E = uniform_defaults.front(); while (E) { int idx = E->key(); int location = version->uniform_location[idx]; if (location < 0) { E = E->next(); continue; } Variant v; v = E->value(); _set_uniform_variant(location, v); E = E->next(); } const Map<uint32_t, CameraMatrix>::Element *C = uniform_cameras.front(); while (C) { int idx = E->key();
Advertencia de PVS-Studio: V522 CWE-476 Puede producirse una desreferenciación del puntero nulo 'E'. shader_gles2.cpp 102
El error se detectó indirectamente. Gracias al análisis
del flujo de datos, PVS-Studio reveló que el puntero
E puede ser cero en el momento de la desreferenciación.
El error es que en el fragmento de código copiado se olvidaron de reemplazar
E en
C en un solo lugar. Debido a este error, la función funciona de una manera muy extraña y hace cosas extrañas.
Errores tipográficos
Error N7Es difícil para los programadores que escriben en lenguajes distintos de C o C ++ imaginar que se puede hacer un error tipográfico escribiendo una coma en lugar de un asterisco *, y se compilará el código. Sin embargo, esto es así.
LRESULT OS_Windows::WndProc(....) { .... BITMAPINFO bmi; ZeroMemory(&bmi, sizeof(BITMAPINFO)); bmi.bmiHeader.biSize = sizeof(BITMAPINFOHEADER); bmi.bmiHeader.biWidth = dib_size.x; bmi.bmiHeader.biHeight = dib_size.y; bmi.bmiHeader.biPlanes = 1; bmi.bmiHeader.biBitCount = 32; bmi.bmiHeader.biCompression = BI_RGB; bmi.bmiHeader.biSizeImage = dib_size.x, dib_size.y * 4; .... }
Advertencia de PVS-Studio: V521 CWE-480 Estas expresiones que usan el operador ',' son peligrosas. Asegúrate de que la expresión sea correcta. os_windows.cpp 776
A la variable
bmi.bmiHeader.biSizeImage se le asigna el valor de la variable
dib_size.x . A continuación, se ejecuta el operador de coma ',', cuya
prioridad es menor que la del operador '='. El resultado de la expresión
dib_size.y * 4 no se utiliza de ninguna manera.
En lugar de una coma en la expresión, se debe usar el operador de multiplicación '*'. En primer lugar, tal expresión tendría sentido. En segundo lugar, a continuación hay una opción similar, pero ya correcta, para inicializar la misma variable:
bmi.bmiHeader.biSizeImage = dib_size.x * dib_size.y * 4;
Errores N8, N9 void Variant::set(....) { .... int idx = p_index; if (idx < 0) idx += 4; if (idx >= 0 || idx < 4) { Color *v = reinterpret_cast<Color *>(_data._mem); (*v)[idx] = p_value; valid = true; } .... }
Advertencia de PVS-Studio: V547 CWE-571 Expresión 'idx> = 0 || idx <4 'siempre es cierto. variant_op.cpp 2152
Cualquier índice se considerará correcto. Para corregir el error, debe reemplazar el
|| en
&& :
if (idx >= 0 && idx < 4) {
Este error lógico probablemente surgió debido al descuido, por lo tanto, me inclino a atribuirlo a errores tipográficos.
Exactamente el mismo error se puede observar en el mismo archivo a continuación. La culpa del error de reproducción, aparentemente, es copiar y pegar.
Error múltiple: V547 CWE-571 Expresión 'idx> = 0 || idx <4 'siempre es cierto. variant_op.cpp 2527
Error N10Un ejemplo de un error del que uno quiere exclamar: ¡¿WTF?!
void AnimationNodeBlendSpace1D::add_blend_point( const Ref<AnimationRootNode> &p_node, float p_position, int p_at_index) { ERR_FAIL_COND(blend_points_used >= MAX_BLEND_POINTS); ERR_FAIL_COND(p_node.is_null()); ERR_FAIL_COND(p_at_index < -1 || p_at_index > blend_points_used); if (p_at_index == -1 || p_at_index == blend_points_used) { p_at_index = blend_points_used; } else { for (int i = blend_points_used - 1; i > p_at_index; i++) { blend_points[i] = blend_points[i - 1]; } } .... }
Advertencia de PVS-Studio: V621 CWE-835 Considere inspeccionar el operador 'para'. Es posible que el bucle se ejecute incorrectamente o no se ejecute en absoluto. animation_blend_space_1d.cpp 113
Tenga en cuenta la condición de detención del bucle:
i> p_at_index . Siempre es cierto, ya que la variable
i se inicializa con el valor
blend_points_used - 1 . Además, de dos comprobaciones anteriores se deduce que
blend_points_used> p_at_index .
La condición puede volverse falsa solo cuando se produce un desbordamiento de la variable de signo
i , que es un comportamiento indefinido. Además, no alcanzará el desbordamiento, ya que la matriz irá más allá del límite mucho antes.
Ante nosotros, en mi opinión, es un error tipográfico hermoso cuando escribieron '>' en lugar de '<'. Sí, tengo una visión pervertida de la belleza de los errores :).
El ciclo correcto:
for (int i = blend_points_used - 1; i < p_at_index; i++) {
Error N11Otro caso igualmente brillante de un error tipográfico en la condición del ciclo.
void AnimationNodeStateMachineEditor::_state_machine_pos_draw() { .... int idx = -1; for (int i = 0; node_rects.size(); i++) { if (node_rects[i].node_name == playback->get_current_node()) { idx = i; break; } } .... }
Advertencia de PVS-Studio: V693 CWE-835 Considere inspeccionar la expresión condicional del bucle. Es posible que se use 'i <X.size ()' en lugar de 'X.size ()'. animation_state_machine_editor.cpp 852
Puede producirse un desbordamiento de la matriz, ya que el valor de
i aumenta sin control. Código seguro:
for (int i = 0; i < node_rects.size(); i++) {
Error N12 GDScriptDataType GDScriptCompiler::_gdtype_from_datatype( const GDScriptParser::DataType &p_datatype) const { .... switch (p_datatype.kind) { .... case GDScriptParser::DataType::NATIVE: { result.kind = GDScriptDataType::NATIVE; result.native_type = p_datatype.native_type; } break; case GDScriptParser::DataType::SCRIPT: { result.kind = GDScriptDataType::SCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } case GDScriptParser::DataType::GDSCRIPT: { result.kind = GDScriptDataType::GDSCRIPT; result.script_type = p_datatype.script_type; result.native_type = result.script_type->get_instance_base_type(); } break; .... }
Advertencia de PVS-Studio: V796 CWE-484 Es posible que falte la declaración 'break' en la declaración del interruptor. gdscript_compiler.cpp 135
Accidentalmente olvidé escribir una declaración de
descanso . Por lo tanto, cuando entra en el
caso GDScriptParser :: DataType :: SCRIPT, a las variables se les asignarán valores, como si fuera el
caso GDScriptParser :: DataType :: GDSCRIPT .
Error N13El siguiente error se puede clasificar como Copiar-Pegar. Sin embargo, no estoy seguro de si se copió una línea tan corta. Por lo tanto, consideraremos que esto es un error tipográfico simple al escribir.
void CPUParticles::_particles_process(float p_delta) { .... if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.velocity.z = 0.0; } .... }
Advertencia de PVS-Studio: V519 CWE-563 La variable 'p.velocity.z' recibe valores asignados dos veces sucesivamente. Quizás esto sea un error. Líneas de verificación: 664, 665. cpu_particles.cpp 665
Dos veces la asignación de la misma variable. A continuación puede ver el siguiente fragmento de código:
if (flags[FLAG_DISABLE_Z]) { p.velocity.z = 0.0; p.transform.origin.z = 0.0; }
Lo más probable es que, para el primer caso, debería haber sido escrito de la misma manera.
Error N14 bool AtlasTexture::is_pixel_opaque(int p_x, int p_y) const { if (atlas.is_valid()) { return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_x + region.position.y + margin.position.y ); } return true; }
Advertencia de PVS-Studio: el parámetro V751 'p_y' no se usa dentro del cuerpo de la función. texture.cpp 1085
Fragmento de la descripción del diagnóstico
V751 :
El analizador detectó una función sospechosa, uno de cuyos parámetros nunca se utiliza. Al mismo tiempo, su otro parámetro se usa varias veces, lo que, posiblemente, indica la presencia de un error.Como puede ver, esto es realmente así, y es muy sospechoso. La variable
p_x se usa dos veces, y
p_y no se usa. Lo más probable es que deba escribirse:
return atlas->is_pixel_opaque( p_x + region.position.x + margin.position.x, p_y + region.position.y + margin.position.y );
Por cierto, en el código fuente, la llamada a la función se escribe en una línea. Debido a esto, el error es más difícil de notar. Si el autor del código escribió los argumentos reales en una columna, como hice en el artículo, entonces el error inmediatamente me llamaría la atención. Recuerde que el formato de tabla es muy útil y evita muchos errores tipográficos. Consulte el capítulo "Alinee el mismo tipo de código con una" tabla "en el artículo"
El tema principal de la programación, refactorización y todo eso ".
Error N15 bool SpriteFramesEditor::can_drop_data_fw(....) const { .... Vector<String> files = d["files"]; if (files.size() == 0) return false; for (int i = 0; i < files.size(); i++) { String file = files[0]; String ftype = EditorFileSystem::get_singleton()->get_file_type(file); if (!ClassDB::is_parent_class(ftype, "Texture")) { return false; } } .... }
Advertencia de PVS-Studio: V767 Acceso sospechoso al elemento de la matriz de 'archivos' por un índice constante dentro de un bucle. sprite_frames_editor_plugin.cpp 602
El bucle procesa el mismo archivo en todas las iteraciones del bucle. Error tipográfico aquí:
String file = files[0];
Debe ser:
String file = files[i];
Otros errores
Error N16 CSGBrush *CSGBox::_build_brush() { .... for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); else face_points[3 - j][(i + k) % 3] = v[k] * (i >= 3 ? -1 : 1); .... } .... }
El analizador PVS-Studio produce inmediatamente dos respuestas a este código:
- V547 CWE-570 La expresión 'i> = 3' siempre es falsa. csg_shape.cpp 939
- V547 CWE-571 La expresión 'i> = 3' siempre es verdadera. csg_shape.cpp 941
De hecho, este operador ternario en ambas expresiones se ve muy extraño:
i >= 3 ? -1 : 1
En un caso, la condición siempre es verdadera, y en el otro es falsa. Es difícil decir cómo este código debería verse bien. Quizás es simplemente redundante y se puede escribir así:
for (int i = 0; i < 6; i++) { .... if (i < 3) face_points[j][(i + k) % 3] = v[k]; else face_points[3 - j][(i + k) % 3] = -v[k]; .... }
Puedo estar equivocado, y el código necesita ser reparado de una manera completamente diferente.
Error N17Casi no hubo errores como V595, aunque generalmente se
encuentran en abundancia en cualquier proyecto . Aparentemente, estos errores se corrigieron después de una verificación previa, y luego los errores de este tipo casi no aparecieron. Solo vi unos pocos falsos positivos y un error.
bool CanvasItemEditor::_get_bone_shape(....) { .... Node2D *from_node = Object::cast_to<Node2D>( ObjectDB::get_instance(bone->key().from)); .... if (!from_node->is_inside_tree()) return false;
Advertencia de PVS-Studio: V595 CWE-476 El puntero 'from_node' se utilizó antes de verificarlo con nullptr. Verifique las líneas: 565, 567. canvas_item_editor_plugin.cpp 565
El puntero
from_node primero se desreferencia para llamar a la función
is_inside_tree, y solo entonces se verifica la igualdad
nullptr . Los cheques deben intercambiarse:
if (!from_node) return false; if (!from_node->is_inside_tree()) return false;
Error N18 enum JoystickList { .... JOY_AXIS_MAX = 10, .... }; static const char *_axes[] = { "Left Stick X", "Left Stick Y", "Right Stick X", "Right Stick Y", "", "", "L2", "R2" }; int InputDefault::get_joy_axis_index_from_string(String p_axis) { for (int i = 0; i < JOY_AXIS_MAX; i++) { if (p_axis == _axes[i]) { return i; } } ERR_FAIL_V(-1); }
Advertencia de PVS-Studio: V557 CWE-125 Array overrun es posible. El valor del índice 'i' podría alcanzar 9. input_default.cpp 1119
La matriz
_axes consta de ocho elementos. En este caso, la constante
JOY_AXIS_MAX , que establece el número de iteraciones del bucle, es 10. Resulta que el bucle va más allá del límite de la matriz.
Error N19Y la última función muy extraña, que, aparentemente, está diseñada para probar algo. La función es larga, así que la mostraré como una imagen (haga clic en la imagen para ampliarla).
Advertencia de PVS-Studio: V779 CWE-561 Código inalcanzable detectado. Es posible que haya un error presente. test_math.cpp 457
Hay varias
declaraciones de retorno incondicionales en una función. En la imagen los marqué con óvalos rojos. Parece que se recopilaron varias pruebas unitarias diferentes para esta función, pero se olvidó de eliminar los
NULL de retorno adicionales. Como resultado, la función no verifica lo que debería verificar. Casi todo el cuerpo de una función consiste en código inalcanzable.
Quizás, por supuesto, esta es una especie de idea astuta. Pero me parece que esto sucedió por casualidad y que el código debería repararse.
Terminemos con esto. Seguramente, si observa detenidamente el informe del analizador, puede encontrar otros errores. Sin embargo, incluso escribió más que suficiente para escribir un artículo. Entonces será aburrido para mí y para el lector :).
Conclusión
El artículo describe errores que no existirían si el código se analizara regularmente utilizando PVS-Studio. Sin embargo, lo que es más importante, utilizando análisis regulares, uno podría encontrar y eliminar muchos otros errores de inmediato. Mi colega describió esta idea con más detalle en su nota: “
Filosofía del análisis de código estático: tenemos 100 programadores, el analizador encontró pocos errores, ¿es inútil? ". Recomiendo pasar 10 minutos leyendo este breve pero muy importante artículo.
Gracias por su atencion Invito a todos a descargar y probar el análisis estático de PVS-Studio para probar sus propios proyectos.
Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Andrey Karpov.
Godot: sobre el uso regular de analizadores estáticos .