Comprobación de la red abierta de Telegram con PVS-Studio

Telegram Open Network (TON) es una plataforma del mismo equipo que desarrolló Telegram messenger. Además de blockchain, TON proporciona un amplio conjunto de servicios. Los desarrolladores crearon recientemente el código de la plataforma, que está escrito en C ++, y lo subieron a GitHub. Decidimos verificar el proyecto antes de su lanzamiento oficial.

Introduccion

Telegram Open Network es un conjunto de varios servicios. Entre otras cosas, proporciona un sistema de pago propio basado en la criptomoneda Gram, y una máquina virtual llamada TON VM, que ejecuta contratos inteligentes. También ofrece un servicio de mensajería, TON Messages. El proyecto en su conjunto es visto como una contramedida a la censura de Internet.

El proyecto está construido con CMake, por lo que no tuve dificultades para construirlo y verificarlo. El código fuente está escrito en C ++ 14 y se ejecuta en 210 mil LOC:

Dado que el proyecto es pequeño y de alta calidad, no hay muchos errores en él, pero aún deben abordarse.

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; .... } 

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

Parece que la función devuelve el tipo incorrecto de estado de error aquí. La función aparentemente debería devolver un valor negativo para el fallo en lugar de verdadero / falso. Eso es al menos lo que hace más en el código, donde devuelve -1.

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) << ....; } 

Mensaje de diagnóstico 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

TON sigue un estándar de codificación que prescribe que los nombres de los miembros de la clase deben terminar en un guión bajo. Sin embargo, en casos como este, esta notación puede provocar un error, ya que corre el riesgo de pasar por alto el guión bajo. El nombre del argumento pasado a esta función es similar al del miembro de la clase, lo que hace que sea fácil mezclarlos. Es este argumento el que probablemente tenía la intención de participar 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()); // <= .... } 

Mensaje de diagnóstico 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 ejecutará, ya que la instrucción if anterior ya la ha verificado.

También hay otro error presente aquí: la macro CHECK no es segura ya que la condición dentro de ella no está envuelta en una construcción do {...} while (0) . Tal envoltura es necesaria para evitar colisiones con otras condiciones en la rama else . En otras palabras, el siguiente código no funcionaría como se esperaba:

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

Comprobando una variable firmada

 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"); } .... } 

Mensaje de diagnóstico 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 ejecutará porque el tipo char está firmado en este caso. Al asignar un valor a una variable de tipo int , se producirá una extensión de signo, por lo que sus valores seguirán estando dentro del rango [-128, 127], no [0, 256].

Cabe señalar que char no siempre está firmado: su comportamiento depende de la plataforma y el compilador. Entonces, en teoría, la condición en cuestión aún podría ejecutarse cuando se construye en una plataforma diferente.

Desplazar bit a bit un número negativo

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

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

Ejecutar una operación de desplazamiento a la derecha a nivel de bit en un número negativo es un comportamiento no especificado: es imposible saber de antemano si el signo se extenderá o se rellenará con ceros.

Cheque nulo después de nuevo

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

Mensaje de diagnóstico 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

El mensaje lo dice todo: si la asignación de memoria falla, el programa lanzará una excepción en lugar de devolver un puntero nulo. Significa que el cheque no tiene sentido.

Cheque redundante

 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); } } .... } 

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

Este fragmento se toma de una de las utilidades internas del proyecto. El operador ternario es redundante en este caso: la condición que verifica ya está verificada por la instrucción if anterior. Parece que los desarrolladores olvidaron eliminar este operador ternario cuando decidieron descartar el uso de rutas estándar (al menos no hay ninguna mención de 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); // <= } 

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

Aparentemente, los desarrolladores iban a usar una variable llamada tmp_info en la última línea de esta función. Aquí está el código de esa 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)); } 

¿Mayor o menor que?

 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; } } 

Mensaje de diagnóstico 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

Si lee detenidamente, notó que este código carece de una operación <=. De hecho, es esta operación la que debe tratar el caso 6. Podemos deducir eso mirando dos puntos. El primero es el código de 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)); .... } 

La función define_builtins , como puede ver, contiene una llamada compile_cmp_int para el operador <= con el parámetro de modo establecido en 6.

El segundo lugar es la función compile_cmp_int , que enumera 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); } 

El índice 6 corresponde a la palabra LEQ , que significa "Menos o igual".

Es otro buen error de la clase de errores encontrados en las funciones de comparación .

Misceláneo

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

Mensaje de diagnóstico 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á encerrado entre paréntesis, lo que podría causar efectos secundarios indeseables si se pasa una expresión compleja a la condición. Pero si la máscara es solo una constante, este código se ejecutará sin ningún problema. Dicho esto, nada le impide pasar nada más a la macro.

Conclusión

TON resultó ser bastante pequeño, por lo que hay pocos errores que encontrar allí, por lo que el equipo de desarrolladores de Telegram ciertamente debe recibir crédito. Pero todos cometen errores de vez en cuando, incluso estos tipos. Los analizadores de código son herramientas poderosas capaces de detectar puntos peligrosos en el código fuente en las primeras etapas de desarrollo, incluso en las bases de código de mayor calidad, así que no los descuide. El análisis estático no debe ejecutarse de vez en cuando, sino que debe ser parte del proceso de desarrollo: " Introduzca el análisis estático en el proceso, no solo busque errores con él ".