Vérification du code Telegram Open Network par l'analyseur PVS-Studio

Telegram Open Network (TON) est une plate-forme des créateurs du messager Telegram, qui, en plus de la blockchain, contient un large éventail de services. Les développeurs ont récemment publié du code de plateforme écrit en C ++ et l'ont publié sur GitHub. Nous voulions découvrir le projet avant son lancement officiel.

Présentation

Telegram Open Network est un ensemble de différents services. Par exemple, le projet a son propre système de paiement basé sur la crypto-monnaie Gram, il y a aussi une machine virtuelle TON VM - des contrats intelligents y travaillent. Il existe un service de messagerie - TON Messages. Le projet lui-même vise à lutter contre la censure d'Internet.

Le projet utilise le système d'assemblage CMake, il n'y a donc pas eu de problèmes particuliers d'assemblage et de vérification. Le code est écrit en C ++ 14 et compte 210 mille lignes:

Le projet est petit et de grande qualité, il n'y a donc pas eu tellement d'erreurs, mais elles méritent attention et correction.

Code retour

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

PVS-Studio Warning: V601 La valeur 'false' est implicitement convertie en type entier. mc-config.cpp 884

Il semble qu'à ce stade, les développeurs aient foiré le statut d'erreur renvoyé. Apparemment, la fonction devrait retourner une valeur négative en cas d'échec, et non vrai / faux. Au moins, c'est le retour de -1 qui peut être observé dans le code ci-dessous.

Comparer une variable avec elle-même

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

Avertissement PVS-Studio: V501 Il existe des sous-expressions identiques à gauche et à droite de l'opérateur '==': zero_state_id_ == zero_state_id_ LastBlock.cpp 66

Le code TON est écrit à l'aide d'une norme de codage dans laquelle les membres de la classe sont nommés avec un trait de soulignement à la fin. Cependant, une telle orthographe, comme dans ce cas, peut passer inaperçue et conduire à une erreur. Le paramètre entrant dans cette fonction a un nom similaire à un membre de la classe, ils sont donc faciles à confondre. C'est probablement lui qui devrait participer à la comparaison.

Macro non sécurisée

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

Avertissement PVS-Studio: V581 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 80, 84. blockdb.cpp 84

La condition à l'intérieur de la macro CHECK ne sera jamais remplie, car son chèque était déjà dans le précédent if .

Il y a une autre erreur: la macro CHECK n'est pas sûre, car la condition à l'intérieur n'est pas encapsulée dans une construction do {...} while (0) . C'est pour éviter les collisions avec d'autres conditions ailleurs . En d'autres termes, ce code ne fonctionnera pas comme prévu:

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

Signer la validation des variables

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

Avertissement PVS-Studio: V560 Une partie de l'expression conditionnelle est toujours fausse: dernier == 0x80. boc.cpp 78

La deuxième partie de la condition ne sera jamais remplie, car type char dans ce cas a un signe. Lors de l'attribution d'une valeur à une variable de type int , le signe s'élargit, donc la plage de valeurs de la variable restera toujours dans la limite [-128, 127], et non dans [0, 256].

Il convient de noter que le type char ne sera pas toujours signé - ce comportement dépend de la plate-forme et du compilateur. Cette condition peut donc théoriquement être remplie lors de l'assemblage sur une autre plate-forme.

Décalage de bit négatif

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

PVS-Studio Warning: V610 Comportement non spécifié. Vérifiez l'opérateur de décalage '>>'. L'opérande gauche '-0x100' est négatif. bigint.hpp 1925

L'opération de décalage au niveau du bit d'un nombre négatif vers la droite est un comportement non spécifié: on ne sait pas comment le signe se comportera dans ce cas - va-t-il se dilater ou se remplir de zéros?

Vérifier null après nouveau

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

Avertissement PVS-Studio: V668 Il est inutile de tester le pointeur «c» par rapport à null, car la mémoire a été allouée à l'aide de l'opérateur «nouveau». L'exception sera générée en cas d'erreur d'allocation de mémoire. CellBuilder.cpp 531

Tout est clair à partir de l'avertissement de l'analyseur - en cas d'allocation de mémoire infructueuse, une exception sera levée et un pointeur nul ne sera pas retourné. La vérification n'a pas de sens.

Revérifier

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

PVS-Studio Warning: V547 Expression 'path' est toujours vrai. fift-main.cpp 136

Ce code a été pris dans l'un des utilitaires internes. L'opérateur ternaire dans ce cas est superflu: la condition qui y est vérifiée existe déjà à l'intérieur du précédent if . Très probablement, ils ont oublié de supprimer l'opérateur ternaire lorsqu'ils ont voulu abandonner les chemins standard (au moins rien n'a été dit à leur sujet dans le message d'aide).

Variable inutilisée

 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); // <= } 

PVS-Studio Warning: V1001 La variable 'tmp_info' est affectée mais n'est pas utilisée à la fin de la fonction. analyzer.cpp 140

Apparemment, dans la dernière ligne de cette fonction, il était prévu d'utiliser la variable tmp_info . Voici le code de la même fonction, mais avec d'autres spécificateurs de paramètres:

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

Plus ou moins?

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

Avertissement PVS-Studio: V1037 Deux ou plusieurs branches de cas effectuent les mêmes actions. Lignes de contrôle: 639, 645 builtins.cpp 639

Les lecteurs attentifs peuvent avoir remarqué que l'opération <= est manquante dans ce code. Et en effet, ce devrait être le numéro 6. Cela peut être trouvé à deux endroits. Le premier est l'initialisation:

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

Dans la fonction define_builtins , vous pouvez voir l'appel compile_cmp_int pour <= avec le paramètre mode égal à 6.

Eh bien, la seconde est la même fonction compile_cmp_int , qui a les noms des opérations:

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

Sous l'index 6 se trouve le mot LEQ , qui signifie inférieur ou égal.

Un autre beau bug lié à la classe des erreurs dans les fonctions de comparaison .

Autre

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

Avertissement PVS-Studio: V1003 La macro 'VM_LOG_IMPL' est une expression dangereuse. Le paramètre 'mask' doit être entouré de parenthèses. log.h 23

La macro VM_LOG_IMPL n'est pas sûre. Son deuxième paramètre n'est pas entouré de parenthèses - cela peut entraîner des effets secondaires si une expression complexe est transmise à la condition. Si mask n'est qu'une constante, alors ce code ne posera aucun problème, mais rien ne nous empêche de passer autre chose dans la macro.

En conclusion

Le code TON s'est avéré pas si gros et ne contient pas beaucoup d'erreurs. Pour cela, bien sûr, il vaut la peine de rendre hommage aux programmeurs de l'équipe Telegram. Cependant, même ils ne sont pas à l'abri des erreurs. L'analyseur de code est un outil puissant qui est capable de trouver des endroits dangereux dans les premiers stades même dans des bases de code de haute qualité, donc son utilisation ne doit pas être négligée. L'analyse statique n'est pas un outil pour les vérifications ponctuelles, mais fait partie du processus de développement: " Intégrez l'analyse statique dans le processus, et ne cherchez pas de bogues avec elle ."

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Larin. Vérification de Telegram Open Network avec PVS-Studio .