Verificación del código de Telegram Open Network por el analizador PVS-Studio

Telegram Open Network (TON) es una plataforma de los creadores del Telegram messenger, que, además de blockchain, contiene un gran conjunto de servicios. Los desarrolladores publicaron recientemente un código de plataforma escrito en C ++ y lo publicaron en GitHub. Queríamos ver el proyecto antes de su lanzamiento oficial.

Introduccion

Telegram Open Network es un conjunto de varios servicios. Por ejemplo, el proyecto tiene su propio sistema de pago basado en la criptomoneda Gram, también hay una máquina virtual TON VM: los contratos inteligentes funcionan en ella. Hay un servicio de mensajería: TON Messages. El proyecto en sí tiene como objetivo combatir la censura de Internet.

El proyecto utiliza el sistema de ensamblaje CMake, por lo que no hubo problemas especiales con el ensamblaje y la verificación. El código está escrito en C ++ 14 y tiene 210 mil líneas:

El proyecto es pequeño y de alta calidad, por lo que no hubo tantos errores, pero son dignos de atención y corrección.

Código de retorno

static int process_workchain_shard_hashes(....) { .... if (f == 1) { if ((shard.shard & 1) || cs.size_ext() != 0x20000) { return false; // <= } .... int r = process_workchain_shard_hashes(....); if (r < 0) { return r; } .... return cb.store_bool_bool(true) && cb.store_ref_bool(std::move(left)) && cb.store_ref_bool(std::move(right)) && cb.finalize_to(branch) ? r : -1; .... } 

Advertencia de PVS-Studio: V601 El valor 'falso' se convierte implícitamente en el tipo entero. mc-config.cpp 884

Parece que en este punto los desarrolladores alteraron el estado de error devuelto. Aparentemente, la función debería devolver un valor negativo en caso de falla, y no verdadero / falso. Al menos, es el retorno de -1 lo que se puede observar en el siguiente código.

Comparar una variable consigo misma

 class LastBlock : public td::actor::Actor { .... ton::ZeroStateIdExt zero_state_id_; .... }; void LastBlock::update_zero_state(ton::ZeroStateIdExt zero_state_id) { .... if (zero_state_id_ == zero_state_id_) { return; } LOG(FATAL) << ....; } 

Advertencia de PVS-Studio: V501 Hay subexpresiones idénticas a la izquierda y a la derecha del operador '==': zero_state_id_ == zero_state_id_ LastBlock.cpp 66

El código TON se escribe utilizando un estándar de codificación en el que los miembros de la clase se nombran con un guión bajo al final. Sin embargo, dicha ortografía, como en este caso, puede pasar desapercibida y provocar un error. El parámetro que entra en esta función tiene un nombre similar a un miembro de la clase, por lo que son fáciles de confundir. Lo más probable es que fuera él quien participara en la comparación.

Macro insegura

 namespace td { namespace detail { [[noreturn]] void process_check_error(const char *message, const char *file, int line); } // namespace detail } #define CHECK(condition) \ if (!(condition)) { \ ::td::detail::process_check_error(#condition, __FILE__, __LINE__); \ } void BlockDb::get_block_handle(BlockIdExt id, ....) { if (!id.is_valid()) { promise.set_error(....); return; } CHECK(id.is_valid()); // <= .... } 

Advertencia de PVS-Studio: V581 Las expresiones condicionales de las declaraciones 'if' ubicadas una junto a la otra son idénticas. Verifique las líneas: 80, 84. blockdb.cpp 84

La condición dentro de la macro CHECK nunca se cumplirá, porque su verificación ya estaba dentro del if anterior.

Hay otro error: la macro CHECK no es segura, porque la condición en su interior no está envuelta en una construcción do {...} while (0) . Esto es para evitar colisiones con otras condiciones en otra cosa . En otras palabras, este código no funcionará como se esperaba:

 if (X) CHECK(condition) else foo(); 

Validación de variable de signo

 class Slice { .... char operator[](size_t i) const; .... }; td::Result<int> CellSerializationInfo::get_bits(td::Slice cell) const { .... int last = cell[data_offset + data_len - 1]; if (!last || last == 0x80) { // <= return td::Status::Error("overlong encoding"); } .... } 

Advertencia de PVS-Studio: V560 Una parte de la expresión condicional siempre es falsa: último == 0x80. boc.cpp 78

La segunda parte de la condición nunca se cumplirá, porque escriba char en este caso tiene un signo. Al asignar un valor a una variable de tipo int , el signo se expandirá, por lo tanto, el rango de valores de la variable permanecerá en el límite [-128, 127] y no en [0, 256].

Vale la pena señalar que el tipo char no siempre se firmará; este comportamiento depende de la plataforma y el compilador. Entonces, esta condición aún se puede cumplir teóricamente cuando se ensambla en otra plataforma.

Bit negativo Bit Bit Shift

 template <class Tr> bool AnyIntView<Tr>::export_bits_any(....) const { .... int mask = (-0x100 >> offs) & 0xff; .... } 

Advertencia de PVS-Studio: V610 Comportamiento no especificado. Verifique el operador de turno '>>'. El operando izquierdo '-0x100' es negativo. bigint.hpp 1925

La operación de desplazar bit a bit un número negativo hacia la derecha es un comportamiento no especificado: no se sabe cómo se comportará el signo en este caso: ¿se expandirá o se llenará con ceros?

Comprobar nulo después de nuevo

 CellBuilder* CellBuilder::make_copy() const { CellBuilder* c = new CellBuilder(); if (!c) { // <= throw CellWriteError(); } .... } 

Advertencia de PVS-Studio: V668 No tiene sentido probar el puntero 'c' contra nulo, ya que la memoria se asignó utilizando el operador 'nuevo'. La excepción se generará en caso de error de asignación de memoria. CellBuilder.cpp 531

Todo está claro en la advertencia del analizador: en caso de que la asignación de memoria no sea exitosa, se generará una excepción y no se devolverá un puntero nulo. La verificación no tiene sentido.

Volver a comprobar

 int main(int argc, char* const argv[]) { .... if (!no_env) { const char* path = std::getenv("FIFTPATH"); if (path) { parse_include_path_set(path ? path : "/usr/lib/fift", source_include_path); } } .... } 

Advertencia de PVS-Studio: V547 La expresión 'ruta' siempre es verdadera. fift-main.cpp 136

Este código fue tomado en una de las utilidades internas. El operador ternario en este caso es superfluo: la condición que está registrada ya existe dentro del if anterior. Lo más probable es que olvidaron eliminar el operador ternario cuando querían abandonar las rutas estándar (al menos no se dijo nada sobre ellas en el mensaje de ayuda).

Variable no utilizada

 bool Op::set_var_info_except(const VarDescrList& new_var_info, const std::vector<var_idx_t>& var_list) { if (!var_list.size()) { return set_var_info(new_var_info); } VarDescrList tmp_info{new_var_info}; tmp_info -= var_list; return set_var_info(new_var_info); // <= } 

Advertencia de PVS-Studio: V1001 La variable 'tmp_info' se asigna pero no se usa al final de la función. analyzer.cpp 140

Aparentemente, en la última línea de esta función, se planeó usar la variable tmp_info . Aquí está el código para la misma función, pero con otros especificadores de parámetros:

 bool Op::set_var_info_except(VarDescrList&& new_var_info, const std::vector<var_idx_t>& var_list) { if (var_list.size()) { new_var_info -= var_list; // <= } return set_var_info(std::move(new_var_info)); } 

Mas o menos?

 int compute_compare(const VarDescr& x, const VarDescr& y, int mode) { switch (mode) { case 1: // > return x.always_greater(y) ? 1 : (x.always_leq(y) ? 2 : 3); case 2: // = return x.always_equal(y) ? 1 : (x.always_neq(y) ? 2 : 3); case 3: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 4: // < return x.always_less(y) ? 1 : (x.always_geq(y) ? 2 : 3); case 5: // <> return x.always_neq(y) ? 1 : (x.always_equal(y) ? 2 : 3); case 6: // >= return x.always_geq(y) ? 1 : (x.always_less(y) ? 2 : 3); case 7: // <=> return x.always_less(y) ? 1 : (x.always_equal(y) ? 2 : (x.always_greater(y) ? 4 : (x.always_leq(y) ? 3 : (x.always_geq(y) ? 6 : (x.always_neq(y) ? 5 : 7))))); default: return 7; } } 

Advertencia de PVS-Studio: V1037 Dos o más ramificaciones de casos realizan las mismas acciones. Líneas de verificación: 639, 645 builtins.cpp 639

Los lectores atentos pueden haber notado que falta la operación <= en este código. Y de hecho, debería ser el número 6. Esto se puede encontrar en dos lugares. El primero es la inicialización:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... if (x.is_int_const() && y.is_int_const()) { r.set_const(compute_compare(x.int_const, y.int_const, mode)); x.unused(); y.unused(); return push_const(r.int_const); } int v = compute_compare(x, y, mode); .... } void define_builtins() { .... define_builtin_func("_==_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 2)); define_builtin_func("_!=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 5)); define_builtin_func("_<_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 4)); define_builtin_func("_>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 1)); define_builtin_func("_<=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 6)); define_builtin_func("_>=_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 3)); define_builtin_func("_<=>_", arith_bin_op, std::bind(compile_cmp_int, _1, _2, 7)); .... } 

En la función define_builtins , puede ver la llamada compile_cmp_int para <= con el parámetro de modo igual a 6.

Bueno, la segunda es la misma función compile_cmp_int , que tiene los nombres de las operaciones:

 AsmOp compile_cmp_int(std::vector<VarDescr>& res, std::vector<VarDescr>& args, int mode) { .... static const char* cmp_names[] = {"", "GREATER", "EQUAL", "GEQ", "LESS", "NEQ", "LEQ", "CMP"}; .... return exec_op(cmp_names[mode], 2); } 

Debajo del índice 6 está la palabra LEQ , que significa Menos o Igual.

Otro hermoso error relacionado con la clase de errores en las funciones de comparación .

Otros

 #define VM_LOG_IMPL(st, mask) \ LOG_IMPL_FULL(get_log_interface(st), ...., VERBOSITY_NAME(DEBUG), \ (get_log_mask(st) & mask) != 0, "") // <= 

Advertencia de PVS-Studio: V1003 La macro 'VM_LOG_IMPL' es una expresión peligrosa. El parámetro 'máscara' debe estar rodeado de paréntesis. log.h 23

La macro VM_LOG_IMPL no es segura. Su segundo parámetro no está rodeado de paréntesis; esto puede provocar efectos secundarios si se pasa una expresión compleja a la condición. Si la máscara es solo una constante, entonces no habrá problemas con este código, pero nada nos impide pasar algo más a la macro.

En conclusión

El código TON resultó no ser tan grande y no contiene muchos errores. Por lo cual, por supuesto, vale la pena rendir homenaje a los programadores del equipo de Telegram. Sin embargo, incluso ellos no son inmunes a los errores. El analizador de código es una herramienta poderosa que puede encontrar lugares peligrosos en las primeras etapas, incluso en bases de código de alta calidad, por lo que no se debe descuidar su uso. El análisis estático no es una herramienta para las comprobaciones únicas, sino que forma parte del proceso de desarrollo: " Incruste el análisis estático en el proceso y no busque errores ".

Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace a la traducción: Sergey Larin. Comprobación de Telegram Open Network con PVS-Studio .