Vérification de Telegram Open Network avec PVS-Studio

Telegram Open Network (TON) est une plateforme de la même équipe qui a développé le messager Telegram. En plus de la blockchain, TON fournit un large éventail de services. Les développeurs ont récemment rendu public le code de la plateforme, qui est écrit en C ++, et l'a téléchargé sur GitHub. Nous avons décidé de vérifier le projet avant sa sortie officielle.

Présentation

Telegram Open Network est un ensemble de différents services. Entre autres choses, il fournit son propre système de paiement basé sur la crypto-monnaie Gram et une machine virtuelle appelée TON VM, qui exécute les contrats intelligents. Il propose également un service de messagerie, TON Messages. Le projet dans son ensemble est considéré comme une contre-mesure à la censure d'Internet.

Le projet est construit avec CMake, donc je n'ai eu aucune difficulté à le construire et à le vérifier. Le code source est écrit en C ++ 14 et s'exécute jusqu'à 210 000 LOC:

Étant donné que le projet est petit et de haute qualité, il n'y a pas beaucoup de bogues, mais ils doivent toujours être traités.

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

Message de diagnostic PVS-Studio: V601 La valeur "false" est implicitement convertie en type entier. mc-config.cpp 884

Il semble que la fonction renvoie ici le mauvais type d'état d'erreur. La fonction devrait apparemment retourner une valeur négative pour l'échec plutôt que vrai / faux. C'est du moins ce qu'il fait plus loin dans le code, où il renvoie -1.

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

Message de diagnostic 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

TON suit une norme de codage qui prescrit que les noms des membres de la classe doivent se terminer par un trait de soulignement. Dans de tels cas, cependant, cette notation peut entraîner un bogue car vous risquez de négliger le trait de soulignement. Le nom de l'argument passé à cette fonction est similaire à celui du membre de la classe, ce qui permet de les mélanger facilement. C'est cet argument qui était très probablement destiné à 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()); // <= .... } 

Message de diagnostic 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 s'exécutera jamais car elle a déjà été vérifiée par l'instruction if précédente.

Il y a aussi une autre erreur ici: 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) . Un tel habillage est nécessaire pour éviter les collisions avec d'autres conditions dans la branche else . En d'autres termes, le code suivant ne fonctionnerait pas comme prévu:

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

Vérification d'une variable signée

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

Message de diagnostic 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 s'exécutera jamais car le type char est signé dans ce cas. Lorsque vous affectez une valeur à une variable de type int , l'extension de signe se produit, de sorte que ses valeurs se situent toujours dans la plage [-128, 127], et non [0, 256].

Il est à noter que char n'est pas toujours signé: son comportement dépend de la plateforme et du compilateur. Donc, en théorie, la condition en question pourrait toujours s'exécuter lors de la construction sur une plate-forme différente.

Décalage binaire d'un nombre négatif

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

Message de diagnostic PVS-Studio: 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'exécution d'une opération de décalage à droite sur un nombre négatif est un comportement non spécifié: il est impossible de savoir à l'avance si le signe sera étendu ou rempli de zéros.

Chèque nul après nouveau

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

Message de diagnostic 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

Le message dit tout: si l'allocation de mémoire échoue, le programme lèvera une exception plutôt que de retourner un pointeur nul. Cela signifie que le chèque est inutile.

Chèque redondant

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

Message de diagnostic PVS-Studio: V547 'Chemin' d'expression est toujours vrai. fift-main.cpp 136

Cet extrait est extrait de l'un des utilitaires internes du projet. L'opérateur ternaire est redondant dans ce cas: la condition qu'il vérifie est déjà vérifiée par l'instruction if précédente. Il semble que les développeurs aient oublié de supprimer cet opérateur ternaire lorsqu'ils ont décidé de supprimer l'utilisation des chemins standard (il n'y a au moins aucune mention de ceux 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); // <= } 

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

Les développeurs allaient apparemment utiliser une variable nommée tmp_info dans la dernière ligne de cette fonction. Voici le code de cette 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 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; } } 

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

Si vous lisez attentivement, vous avez remarqué qu'il manque une opération <= à ce code. En effet, c'est cette opération que le cas 6 devrait traiter. Nous pouvons en déduire en examinant deux points. Le premier est le code d'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)); .... } 

La fonction define_builtins , comme vous pouvez le voir, contient un appel compile_cmp_int pour l'opérateur <= avec le paramètre mode défini sur 6.

Le deuxième point est la fonction compile_cmp_int elle-même, qui répertorie 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); } 

L'index 6 correspond au mot LEQ , qui signifie «inférieur ou égal».

C'est un autre joli bogue de la classe des bogues trouvés dans les fonctions de comparaison .

Divers

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

Message de diagnostic 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 placé entre parenthèses, ce qui pourrait potentiellement provoquer des effets secondaires indésirables si une expression complexe est transmise à la condition. Mais si mask n'est qu'une constante, ce code fonctionnera sans aucun problème. Cela dit, rien ne vous empêche de passer autre chose à la macro.

Conclusion

TON s'est avéré être assez petit, il y a donc peu de bogues à trouver, ce dont l'équipe de développeurs Telegram devrait certainement être créditée. Mais tout le monde fait des erreurs de temps en temps, même ces gars-là. Les analyseurs de code sont des outils puissants capables de détecter les points dangereux dans le code source aux premiers stades de développement, même dans les bases de code les plus de qualité, alors ne les négligez pas. L'analyse statique n'est pas censée être exécutée de temps en temps mais doit faire partie du processus de développement: " Introduisez l'analyse statique dans le processus, ne vous contentez pas de rechercher des bogues ".