Cómo encontrar trabajo para los Fixis: analizando Godot Engine

Programador Unicornio — Mon, 02 Sep 2024 11:35:15 +0000

Desarrollar y jugar videojuegos puede ser una actividad increíblemente divertida, absorbente y gratificante. Sin embargo, nada arruina tanto la experiencia de juego como un bug astutamente oculto. Por eso, hoy vamos a utilizar el analizador PVS-Studio para examinar el motor de juegos de código abierto Godot Engine. Veamos qué tan bueno es y si está listo para ofrecernos experiencias inolvidables tanto en el desarrollo como en el juego. Empecemos.

Godot

Godot es un motor de juegos 2D y 3D versátil, diseñado para soportar todo tipo de proyectos. Se puede utilizar para crear juegos o aplicaciones que luego pueden lanzarse en plataformas de escritorio o móviles, así como en la web.

El motor es relativamente joven, pero ya está ganando impulso y es popular entre aquellos que valoran el código abierto, la gratuidad y una buena extensibilidad. Godot es bastante amigable para los principiantes y, por lo tanto, es popular entre los desarrolladores noveles.

Algunos juegos que se han desarrollado con este motor son 1000 days to escape, City Game Studio: Your Game Dev Adventure Begins, Precipice.

La versión de Godot Engine en la que se realizó la conprobación es la 4.2.2.

Por cierto, ya habíamos revisado Godot Engine en 2018. Puedes consultar el artículo anterior aquí.

Antes de sumergirnos en el análisis de código con el analizador, me gustaría comentarte que puedes obtener una prueba gratuita de 30 días de PVS-Studio. Esta prueba te da acceso a todas las funcionalidades incluidas en la licencia PVS-Studio Enterprise. Solo tienes que solicitarla proporcionando tu dirección de correo electrónico. Y si te dedicas al desarrollo de videojuegos, ya sea con Godot, Unreal Engine o Unity, te podría interesar echarle un vistazo.

Resultados de la comprobación con PVS-Studio

Lo primero que me gustaría abordar después de revisar el informe son los problemas con las macros en el proyecto. El problema con ellas es que los parámetros no están envueltos entre paréntesis. Veamos algunos ejemplos de cómo esto puede volverse en nuestra contra.

Fragmentos N1-N2

#define HAS_WARNING(flag) (warning_flags & flag)

Esta macro es necesaria para verificar si se ha establecido una determinada bandera de advertencia o no.

La variable warning_flags es una máscara de bits y tiene el tipo uint_32t. Esto significa que su valor consta de 32 bits, donde cada bit corresponde a 1 si la bandera está establecida, y 0 si no lo está. La macro se utiliza en operadores condicionales, donde se convierte implícitamente al tipo bool. Para entender su funcionamiento, consideremos una versión simplificada donde usaremos 8 bits en lugar de 32.

Supongamos que tenemos una bandera X que corresponde al cuarto bit en la máscara y actualmente está activada. Entonces el valor de la variable warning_flags en sistema binario se verá así:

00001000

Ahora supongamos que decidimos verificar con nuestra macro si la bandera X está establecida.

Pasamos a la macro una variable flag con el valor 00001000 y como resultado de la operación AND bit a bit obtenemos un valor distinto de cero, que se convierte a bool con el valor true.

Ahora supongamos que queremos verificar la bandera Y, que corresponde al tercer bit, con el mismo valor de la variable warning_flags. Pasamos a la macro una variable con el valor 00000100 y como resultado de la operación AND bit a bit obtenemos un valor cero, que se convierte a bool con el valor false.

Parece que todo está bien, ¿qué podría salir mal? Pero ¿qué pasa si alguien quiere verificar si se ha establecido una de varias banderas? Entonces podría llamar a la macro así:

if (HAS_WARNING(flags::X | flags::Y)) ....

Y entonces el resultado de tal operación siempre será true, incluso si ninguna de las banderas está establecida. ¿Por qué sucede esto? Vamos a actuar como el preprocesador y simplemente sustituir la expresión pasada a la macro:

if (warning_flags & flags::X | flags::Y) ....

Ahora consultemos la tabla de prioridades de operadores:

Prioridad	Operador	Descripción	Asociatividad
....	....	....	....
11	a & b	AND bit a bit	De izquierda a derecha
....	....	....	....
13	\		OR bit a bit

Los operadores se enumeran de arriba a abajo en orden decreciente de prioridad. De esto se deduce que la expresión se procesará de la siguiente manera:

if (( warning_flags & flags::X ) | flags::Y) ....

Supongamos que en warning_flags no están establecidas las banderas X e Y que nos interesan. Entonces la primera operación AND bit a bit devolverá el valor 0, y luego se establecerá la bandera Y en él. Obtenemos una verificación que siempre es verdadera.

De hecho, el analizador emite la siguiente advertencia sobre esta macro:

V1003 The macro 'HAS_WARNING' is a dangerous expression. The parameter 'flag' must be surrounded by parentheses. shader_language.cpp 40

Y, como se indica en el mensaje, para corregirlo solo es necesario envolver el parámetro de la macro entre paréntesis:

#define HAS_WARNING(flag) (warning_flags & (flag))

Otro ejemplo de una macro peligrosa:

#define IS_SAME_ROW(i, row) (i / current_columns == row)

Advertencia del analizador:

V1003 The macro 'IS_SAME_ROW' is a dangerous expression. The parameters 'i', 'row' must be surrounded by parentheses. item_list.cpp 643

Si pasamos a la macro una expresión en lugar de una sola variable, por ejemplo, algo así:

IS_SAME_ROW(current + 1, row)

Entonces, como resultado de la sustitución del preprocesador, obtendremos:

(current + 1 / current_columns == row)

Donde el orden de cálculo no es en absoluto el que se esperaba.

Para protegernos de tales situaciones, es suficiente envolver los parámetros de la macro entre paréntesis:

#define IS_SAME_ROW(i, row) ((i) / current_columns == (row))

Fragmento N3

Ahora consideremos la siguiente condición:

const auto hint_r = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_R;
const auto hint_gray = ShaderLanguage::ShaderNode::Uniform::HINT_ROUGHNESS_GRAY;

if (tex->detect_roughness_callback
    && (   p_texture_uniforms[i].hint >= hint_r
        || p_texture_uniforms[i].hint <= hint_gray))
{
  ....
}

Esta condición siempre será true (excepto cuando el puntero tex->detect_roughness_callback sea nulo).

Para entender por qué, necesitamos mirar el enum Hint en la estructura Uniform:

struct Uniform
{
  ....
  enum Hint 
  {
    HINT_NONE,
    HINT_RANGE,
    HINT_SOURCE_COLOR,
    HINT_NORMAL,
    HINT_ROUGHNESS_NORMAL,
    HINT_ROUGHNESS_R,
    HINT_ROUGHNESS_G,
    HINT_ROUGHNESS_B,
    HINT_ROUGHNESS_A,
    HINT_ROUGHNESS_GRAY,
    HINT_DEFAULT_BLACK,
    HINT_DEFAULT_WHITE,
    HINT_DEFAULT_TRANSPARENT,
    HINT_ANISOTROPY,
    HINT_SCREEN_TEXTURE,
    HINT_NORMAL_ROUGHNESS_TEXTURE,
    HINT_DEPTH_TEXTURE,
    HINT_MAX
  };
  ....
};

Bajo la superficie de este enum se encuentra un tipo entero, y a los valores HINT_ROUGHNESS_R y HINT_ROUGHNESS_GRAY les corresponden los números 5 y 9.

Basándonos en esto, en la condición se verifica que p_texture_uniforms[i].hint >= 5 o p_texture_uniforms[i].hint <= 9. Esto significa que cualquier valor de p_texture_uniforms[i].hint pasará estas verificaciones, de lo cual advierte PVS-Studio:

V547 Expression is always true. material_storage.cpp 929

En realidad, el programador quería verificar que p_texture_uniforms[i].hint esté en el rango de 5 a 9. Para esto, es necesario aplicar el AND lógico:

if (tex->detect_roughness_callback
    && (   p_texture_uniforms[i].hint >= hint_r
        && p_texture_uniforms[i].hint <= hint_gray))
{
  ....
}

Una activación similar:

V547 Expression is always true. material_storage.cpp 1003

Fragmento N4

Intenta encontrar el error aquí por ti mismo:
Error FontFile::load_bitmap_font(const String &p_path)
{
  if (kpk.x >= 0x80 && kpk.x <= 0xFF)
  {
    kpk.x = _oem_to_unicode[encoding][kpk.x - 0x80];
  } else if (kpk.x > 0xFF){
    WARN_PRINT(vformat("Invalid BMFont OEM character %x
                        (should be 0x00-0xFF).", kpk.x));
    kpk.x = 0x00;
  }

  if (kpk.y >= 0x80 && kpk.y <= 0xFF) 
  {
    kpk.y = _oem_to_unicode[encoding][kpk.y - 0x80];
  } else if (kpk.y > 0xFF){
    WARN_PRINT(vformat("Invalid BMFont OEM character %x
                        (should be 0x00-0xFF).", kpk.x));
    kpk.y = 0x00;
  }
  ....

Advertencia del analizador:

V778 Two similar code fragments were found. Perhaps, this is a typo and 'y' variable should be used instead of 'x'. font.cpp 1970

Así que PVS-Studio encontró un error que surgió al copiar un fragmento de código. Echemos un vistazo más de cerca a los bloques condicionales. En esencia, son idénticos, excepto que en el primer caso todas las operaciones se realizan sobre kpk.x, y en el segundo sobre kpk.y.

Pero en la segunda condición, como resultado del copy-paste, se coló un error. Presta atención a la llamada a WARN_PRINT: si kpk.y > 0xFF, entonces al formar la advertencia se imprimirá el carácter kpk.x, no kpk.y. Será más difícil buscar el error basándose en los registros :)

P.D.: en realidad, no se debería haber duplicado el código de esta manera. Claramente se ve que los dos bloques de código difieren solo en el campo aplicado. La mejor variante hubiera sido extraer el código en una función y llamarla dos veces para diferentes campos:

Error FontFile::load_bitmap_font(const String &p_path)
{
  constexpr auto check = [](auto &ch)
  {
    if (ch >= 0x80 && ch <= 0xFF)
    {
      auto res = _oem_to_unicode[encoding][ch - 0x80];
      ch = res;
    }
    else if (ch > 0xFF)
    {
      WARN_PRINT(vformat("Invalid BMFont OEM character %x
                              (should be 0x00-0xFF).",ch));
      ch = 0x00;
    }
  };

  check(kpk.x);
  check(kpk.y);
  ....
}

Fragmento N5

Más condiciones, pero esta vez anidadas:

void GridMapEditor::_mesh_library_palette_input(const Ref<InputEvent> &p_ie) 
{
  const Ref<InputEventMouseButton> mb = p_ie;
  // Zoom in/out using Ctrl + mouse wheel
  if (mb.is_valid() && mb->is_pressed() && mb->is_command_or_control_pressed()) 
  {
    if (mb->is_pressed() && mb->get_button_index() == MouseButton::WHEEL_UP) 
    {
      size_slider->set_value(size_slider->get_value() + 0.2);
    }
    ....
  }
}

Advertencia del analizador:

V571 Recurring check. The 'mb->is_pressed()' condition was already verified in line 837. grid_map_editor_plugin.cpp 838

En este fragmento hay una verificación redundante en el operador if anidado. La expresión mb->is_pressed() ya se había comprobado en el nivel superior. Posiblemente sea una doble verificación (común en GUI), pero en ese caso debería haberse añadido un comentario al respecto. O tal vez se debería haber comprobado algo diferente.

Activaciones similares:

V571 Recurring check. The '!r_state.floor' condition was already verified in line 1711. physics_body_3d.cpp 1713
V571 Recurring check. The '!wd_window.is_popup' condition was already verified in line 2012. display_server_x11.cpp 2013
V571 Recurring check. The 'member.variable->initializer' condition was already verified in line 946. gdscript_analyzer.cpp 949

Fragmento N6

Y cómo no, un clásico: desreferenciar un puntero antes de verificarlo:
void GridMapEditor::_update_cursor_transform()
{
  cursor_transform = Transform3D();
  cursor_transform.origin = cursor_origin;
  cursor_transform.basis = node->get_basis_with_orthogonal_index(cursor_rot);
  cursor_transform.basis *= node->get_cell_scale();
  cursor_transform = node->get_global_transform() * cursor_transform;

  if (selected_palette >= 0)
  {
    if (node && !node->get_mesh_library().is_null())
    {
      cursor_transform *= node->get_mesh_library()
                              ->get_item_mesh_transform(selected_palette);
    }
  }
  ....
}

Advertencia del analizador:

V595 The 'node' pointer was utilized before it was verified against nullptr. Check lines: 246, 251. grid_map_editor_plugin.cpp 246

Es bastante extraño desreferenciar un puntero y luego verificarlo unas líneas más abajo. Posiblemente, la desreferencia apareció en el código después de la verificación, y el desarrollador no notó la verificación más abajo.

Activaciones similares:

V595 The 'p_ternary_op->true_expr' pointer was utilized before it was verified against nullptr. Check lines: 4518, 4525. gdscript_analyzer.cpp 4518
V595 The 'p_parent' pointer was utilized before it was verified against nullptr. Check lines: 4100, 4104. node_3d_editor_plugin.cpp 4100
V595 The 'item' pointer was utilized before it was verified against nullptr. Check lines: 950, 951. project_export.cpp 950
V595 The 'title_bar' pointer was utilized before it was verified against nullptr. Check lines: 1153, 1163. editor_node.cpp 1153
V595 The 'render_target' pointer was utilized before it was verified against nullptr. Check lines: 2121, 2132. rasterizer_canvas_gles3.cpp 2121
V595 The '_p' pointer was utilized before it was verified against nullptr. Check lines: 228, 231. dictionary.cpp 228
V595 The 'class_doc' pointer was utilized before it was verified against nullptr. Check lines: 1215, 1231. extension_api_dump.cpp 1215

Fragmento N7

template <class T, class U = uint32_t,
          bool force_trivial = false, bool tight = false>
class LocalVector
{
  ....
public:
  operator Vector<T>() const
  {
    Vector<T> ret;
    ret.resize(size());
    T *w = ret.ptrw();
    memcpy(w, data, sizeof(T) * count);
    return ret;
  }
  ....
};

Advertencia del analizador:

V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280

Fragmento interesante. La plantilla de clase LocalVector tiene un operador de conversión a la clase Vector. Durante tal conversión, es necesario copiar el contenido del vector actual al nuevo. Para esto, se utilizó la función memcpy.

Todo está bastante bien mientras el tipo de plantilla T sea trivialmente copiable. Sin embargo, el analizador detectó varias especializaciones de LocalVector donde esta propiedad se viola. Como ejemplo, consideremos la especialización LocalVector:

struct AnimationCompressionDataState
{
  uint32_t components = 3;
  LocalVector<uint8_t> data; // Committed packets.
  struct PacketData
  {
    int32_t data[3] = { 0, 0, 0 };
    uint32_t frame = 0;
  };

  float split_tolerance = 1.5;

  LocalVector<PacketData> temp_packets;

  // used for rollback if the new frame does not fit
  int32_t validated_packet_count = -1;
  ....
};

La clase AnimationCompressionDataState contiene un LocalVector, que en sí mismo no es trivialmente copiable.

Para este caso, la documentación de memcpy tiene una aclaración: "Si los objetos se superponen potencialmente o no son TriviallyCopyable, el comportamiento de memcpy no está especificado y puede ser indefinido".

Corregir el código no es difícil, basta con reemplazar la llamada a memcpy por std::uninitialized_copy:

operator Vector<T>() const
{
  Vector<T> ret;
  ret.resize(size());
  T *w = ret.ptrw();
  std::uninitialized_copy(data, data + count, w);
  return ret;
}

PVS-Studio descubrió otras 38 especializaciones peligrosas, pero por supuesto no voy a proporcionar la lista completa:

V780 Instantiation of LocalVector < AnimationCompressionDataState >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280
V780 Instantiation of LocalVector < LocalVector >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280
V780 Instantiation of LocalVector < Mapping, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280
V780 Instantiation of LocalVector < OAHashMap < uint64_t, Specialization > >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280
V780 Instantiation of LocalVector < Pair < StringName, StringName >, uint32_t, bool, bool >: The object 'w' of a non-passive (non-PDS) type cannot be copied using the memcpy function. local_vector.h 280
...

Fragmento N8

Posible violación de la lógica del programa:

Dictionary GDScriptSyntaxHighlighter::_get_line_syntax_highlighting_impl
                                                             (int p_line)
{
  const String &str = text_edit->get_line(p_line);
  ....
  if (   is_digit(str[non_op])
      || (   str[non_op] == '.' 
          && non_op < line_length 
          && is_digit(str[non_op + 1]) ) )
  {
    in_number = true;
  }
  ....
}

Advertencia del analizador:

V781 The value of the 'non_op' index is checked after it was used. Perhaps there is a mistake in program logic. gdscript_highlighter.cpp 370

El valor de non_op se utiliza primero como índice al acceder a los caracteres de la cadena, y solo después se verifica que sea menor que la longitud.

Fíjate en el acceso a la cadena después de la verificación. Si non_op < line_length, esto aún no significa que (non_op + 1) < line_length. Por lo tanto, en str[non_op + 1] puede ocurrir un desbordamiento de la cadena. Especialmente teniendo en cuenta que bajo la superficie, String no contiene cadenas terminadas en nulo.

La verificación correcta debería verse así:

if (   is_digit(str[non_op])
    || (   str[non_op] == '.' 
        && non_op + 1 < line_length 
        && is_digit(str[non_op + 1]) ) )
{
  in_number = true;
}

Fragmento N9

struct Particles
{
  ....
  int amount = 0;
  ....
};

void ParticlesStorage::_particles_update_instance_buffer(
  Particles *particles,
  const Vector3 &p_axis,
  const Vector3 &p_up_axis)
{
  ....
  uint32_t lifetime_split = ....;
  // Offset VBO so you render starting at the newest particle.
  if (particles->amount - lifetime_split > 0)
  {
    ....
  }
  ....
}

Advertencia del analizador:

V555 The expression 'particles->amount - lifetime_split > 0' will work as 'particles->amount != lifetime_split'. particles_storage.cpp 959

Este ejemplo es interesante porque, a pesar de que la activación del analizador no es completamente correcta, nos señala un lugar al que los desarrolladores deberían prestar atención.

Si la diferencia entre dos variables sin signo es mayor que cero, en realidad esta expresión es semánticamente equivalente a particles->amount != lifetime_split. La condición se evaluará como false solo cuando estas variables sean iguales. Si el operando izquierdo es menor que el derecho, ocurrirá un desbordamiento con envolvimiento, y la expresión resultante será mayor que cero. Si el operando izquierdo es mayor que el derecho, la diferencia será mayor que cero.

Sin embargo, lo notable aquí es otra cosa: ambas variables tienen el mismo rango, pero diferente signo. El compilador, según el estándar, está obligado a realizar conversiones implícitas antes de realizar la resta. Y en esta situación, el tipo común será un int sin signo de 32 bits. Y esto también puede añadir sorpresas si el operando izquierdo tiene un número negativo.

La verificación más correcta en el caso de expresiones con tipos con y sin signo del mismo rango se vería así:

if (particles->amount >= 0 && particles->amount > lifetime_split)

En realidad, hemos reinventado std::cmp_greater, introducido en C++20, y a partir de esta versión del estándar, se puede escribir un código conciso:

if (std::cmp_greater(particles->amount, lifetime_split))

Fragmento N10

void AnimationNodeStateMachineEditor::_delete_tree_draw()
{
  TreeItem *item = delete_tree->get_next_selected(nullptr);
  while (item) 
  {
    delete_window->get_cancel_button()->set_disabled(false);
    return;
  }
  delete_window->get_cancel_button()->set_disabled(true);

}

Advertencia del analizador:

V1044 Loop break conditions do not depend on the number of iterations. animation_state_machine_editor.cpp 693

El bucle while dura exactamente una iteración. Se parece mucho al patrón en el que se necesita tomar solo el primer elemento de un contenedor, y esto se hace con un bucle for:

for (auto &&item : items)
{
  DoSomething(item);
  break;
}

De esta manera, no es necesario verificar si el contenedor contiene el primer elemento. En mi opinión, tal código es más confuso, ya que se espera que los bucles tengan un número final de iteraciones desconocido de antemano.

En el fragmento de arriba, el bucle while no tiene absolutamente ningún sentido. Hubiera sido suficiente con una simple construcción if:

void AnimationNodeStateMachineEditor::_delete_tree_draw()
{
  TreeItem *item = delete_tree->get_next_selected(nullptr);
  if (item)
  { 
    delete_window->get_cancel_button()->set_disabled(false);
    return;
  }

  delete_window->get_cancel_button()->set_disabled(true);
}

Fragmento N11

static const char *script_list[][2] = {
  ....
  { "Myanmar / Burmese", "Mymr" },
  { "Nag Mundari", "Nagm" },
  { "Nandinagari", "Nand" },
  ....
}

El lector puede preguntarse: "¿Y qué hay de malo aquí?" Nosotros mismos no lo habríamos entendido si no fuera por la activación de la regla de diagnóstico V1076. Lo interesante es que esta es la primera activación que hemos anotado. La regla de diagnóstico verifica la presencia de caracteres invisibles en el texto del programa. Tales caracteres son una especie de marcadores que el programador puede no ver debido a la configuración de visualización del texto en el entorno de desarrollo, pero el compilador los ve y procesa perfectamente.

Advertencia del analizador:

V1076 Code contains invisible characters that may alter its logic. Consider enabling the display of invisible characters in the code editor. locales.h 1114

Veamos atentamente la siguiente línea:

{ "Nag Mundari", "Nagm" },

Es precisamente en esta línea donde se encuentra el marcador con un carácter invisible. Si usamos un editor hexadecimal, podemos notar lo siguiente:

Entre las comillas dobles y el carácter N se han colado 3 bytes: E2, 80 y 8B. Estos corresponden al carácter Unicode ZERO WIDTH SPACE (U+200B).

Afortunadamente, la presencia de este carácter en el literal de cadena no afectará la lógica del programa.

Las cadenas del array script_list, que contiene el literal de cadena "infectado", terminan en la tabla hash TranslationServer::script_map. La clave de tal tabla hash será el segundo literal de cadena del par, y el valor será el primero. Esto significa que el literal de cadena con el marcador entrará en la tabla hash como un valor, y la búsqueda en la tabla hash no se verá afectada.

Además, podemos examinar dónde podría potencialmente terminar este valor de la tabla hash. Encontré varios lugares:

El valor terminará en la cadena devuelta por la función TranslationServer::get_locale_name. Analizando las funciones que la llaman, se ve que esta cadena terminará de una forma u otra en la GUI ([1], [2], [3], [4]).
El valor es devuelto por la función TranslationServer::get_script_name. Analizando las funciones que la llaman, también se puede concluir que la cadena terminará en la GUI ([1], [2]).

Lo más probable es que el marcador se introdujera accidentalmente como resultado de copiar el nombre de algún sitio web. Es suficiente con simplemente eliminar este carácter del literal de cadena.

Fragmento N12

void MeshStorage::update_mesh_instances() 
{
  ....
  uint64_t mask = RS::ARRAY_FORMAT_VERTEX | RS::ARRAY_FORMAT_NORMAL 
                | RS::ARRAY_FORMAT_VERTEX;
  ....
}

Advertencias del analizador:

V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1414.
V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1414.

Inicialización extraña de la máscara de bits. Se escribe dos veces RS::ARRAY_FORMAT_VERTEX en ella, aunque posiblemente se quería escribir alguna otra bandera.

La misma activación:

V501 There are identical sub-expressions 'RenderingServer::ARRAY_FORMAT_VERTEX' to the left and to the right of the '|' operator. mesh_storage.cpp 1300.
V578 An odd bitwise operation detected. Consider verifying it. mesh_storage.cpp 1300.

Fragmento N13

void Image::initialize_data(int p_width, int p_height, bool p_use_mipmaps,
                            Format p_format, const Vector<uint8_t> &p_data)
{
  ....
  ERR_FAIL_COND_MSG(p_width > MAX_WIDTH, "The Image width specified (" + 
                                         itos(p_width) +
                                         " pixels) cannot be greater than " +
                                         itos(MAX_WIDTH) +
                                         " pixels.");

  ERR_FAIL_COND_MSG(p_height > MAX_HEIGHT, "The Image height specified (" +
                                           itos(p_height) +
                                           " pixels) cannot be greater than " +
                                           itos(MAX_HEIGHT) +
                                           " pixels.");

  ERR_FAIL_COND_MSG(p_width * p_height > MAX_PIXELS,
                   "Too many pixels for image, maximum is " + itos(MAX_PIXELS));
  ....
}

Advertencia del analizador:

V1083 Signed integer overflow is possible in 'p_width * p_height' arithmetic expression. This leads to undefined behavior. Left operand is in range '[0x1..0x1000000]', right operand is in range '[0x1..0x1000000]'. image.cpp 2200

Así que tenemos dos variables p_width y p_height de tipo int. El valor máximo que puede almacenar un int de 4 bytes es 2'147'483'647.

Primero, el código verifica que p_width <= MAX_WIDTH, donde MAX_WIDTH == 16'777'216. Luego verifica que p_height <= MAX_HEIGHT, donde MAX_HEIGHT == 16'777'216. En la tercera verificación, comparamos que el producto p_width * p_height <= MAX_PIXELS.

Analicemos la situación cuando p_width == p_height && p_width == 16'777'216. El resultado de multiplicar estos dos números es 281'474'976'710'656. Para representar tal resultado, ya se necesita un número de 8 bytes, es decir, hay un desbordamiento de signo evidente. Y, como es sabido, en los lenguajes C y C++ esto lleva a un comportamiento indefinido.

Si no hay funciones auxiliares que verifiquen el desbordamiento, la variante más simple de corrección podría verse así:

ERR_FAIL_COND_MSG((int64_t) p_width * (int64_t) p_height > (int64_t) MAX_PIXELS,
                  "Too many pixels for image, maximum is " + itos(MAX_PIXELS));

Fragmento N14

void RemoteDebugger::debug(....)
{
  ....
  mutex.lock();
  while (is_peer_connected())
  {
    mutex.unlock();
    ....
  }

  send_message("debug_exit", Array());
  if (Thread::get_caller_id() == Thread::get_main_id())
  {
    if (mouse_mode != Input::MOUSE_MODE_VISIBLE)
    {
      Input::get_singleton()->set_mouse_mode(mouse_mode);
    }
  } 
  else 
  {
    MutexLock mutex_lock(mutex);
    messages.erase(Thread::get_caller_id());
  }
}

V1020 The function exited without calling the 'mutex.unlock' function. Check lines: 556, 438. remote_debugger.cpp 556

Fragmento muy interesante con ejecución multihilo. El analizador PVS-Studio descubrió que en algunas rutas de ejecución el mutex podría no desbloquearse. Vamos a analizarlo.

Hay que empezar por ver qué tipo de mutex se está utilizando:
class RemoteDebugger : public EngineDebugger
{
  ....
private:
  // Make handlers and send_message thread safe.
  Mutex mutex;
  ....
};

Profundicemos un poco más para ver qué es este Mutex:

template <class StdMutexT>
class MutexImpl
{
  friend class MutexLock<MutexImpl<StdMutexT>>;
  using StdMutexType = StdMutexT; 
  mutable StdMutexT mutex;
public:
  _ALWAYS_INLINE_ void lock() const { mutex.lock(); }

  _ALWAYS_INLINE_ void unlock() const { mutex.unlock(); }

  _ALWAYS_INLINE_ bool try_lock() const { return mutex.try_lock(); }
};

// Recursive, for general use
using Mutex = MutexImpl<THREADING_NAMESPACE::recursive_mutex>;

Así que, en realidad, no estamos ante un mutex común, sino uno recursivo. Se utiliza junto con un envoltorio RAII personalizado:

template <class MutexT>
class MutexLock
{
  friend class ConditionVariable;

  std::unique_lock<typename MutexT::StdMutexType> lock;

public:
  _ALWAYS_INLINE_ explicit MutexLock(const MutexT &p_mutex)
    : lock(p_mutex.mutex) {}
};

Casi en todas partes, el mutex RemoteDebugger::mutex se utiliza junto con envoltorios RAII, mostraré solo un par de lugares: [1], [2], [3], ....

Sin embargo, en un lugar algo salió mal. El analizador señaló un lugar donde se trabaja con el mutex manualmente. Debido a esto, tenemos varias variantes diferentes de ejecución del código:

El mutex se bloquea, el ciclo no se ejecuta ni una vez (N == 0). Como resultado, el flujo de control abandonará la función RemoteDebugger::debug con el contador de captura incrementado en 1.
El mutex se bloquea, el ciclo se ejecuta N == 1 vez. En este caso, todo estará bien: el contador de captura del mutex recursivo se incrementa y disminuye en el mismo número.
El mutex se bloquea, el ciclo se ejecuta N > 1 veces. Como resultado, el contador de captura del mutex recursivo disminuirá en N – 1 con respecto al momento anterior a su bloqueo manual, lo que puede llevar a un comportamiento indefinido.

Si examinamos las llamadas a la función is_peer_connected en la base de código ([1], [2], [3], ....), en todos los casos ocurren bajo el bloqueo de RemoteDebugger::mutex. Aparentemente, el programador también necesitaba un bloqueo en este caso, pero lo implementó manualmente.

Basándonos en tales suposiciones, podemos corregir el código de la siguiente manera:

void RemoteDebugger::debug(....)
{
  ....
  const auto is_peer_connected_sync = [this]
  {
    MutexLock _ { mutex };
    return is_peer_connected();
  };

  while (is_peer_connected_sync())
  {
    ....
  }
  ....
}

No garantizo que la corrección sea absolutamente correcta, ya que solo los desarrolladores de Godot saben lo que debería estar sucediendo aquí. Pero al menos ahora nos hemos librado del potencial comportamiento indefinido relacionado con el desbloqueo del mutex en cada iteración del ciclo.

Conclusión

Los errores en el código son de lo más variado: desde simples hasta complejos, desde evidentes hasta imperceptibles. Para no estropear el placer y la impresión del producto, es necesario limpiarlo constantemente de bugs y errores. Los analizadores estáticos y dinámicos son muy buenos para esta tarea.

Comenzar a utilizar tales soluciones es más fácil de lo que podría parecer. Por ejemplo, se puede obtener una versión de prueba del analizador PVS-Studio aquí. También existen varios escenarios de uso gratuito.

¡Gracias a todos por leer y que tengan un buen día!

Forem: Programador Unicornio

Cómo encontrar trabajo para los Fixis: analizando Godot Engine

Godot

Conclusión