Verificando a rede aberta do telegrama com o PVS-Studio

O Telegram Open Network (TON) é uma plataforma da mesma equipe que desenvolveu o messenger Telegram. Além da blockchain, a TON fornece um grande conjunto de serviços. Os desenvolvedores recentemente disponibilizaram publicamente o código da plataforma, escrito em C ++, e o carregou no GitHub. Decidimos verificar o projeto antes de seu lançamento oficial.

1. Introdução

O Telegram Open Network é um conjunto de vários serviços. Entre outras coisas, fornece um sistema de pagamento próprio com base na criptomoeda Gram e uma máquina virtual chamada TON VM, que executa contratos inteligentes. Ele também oferece um serviço de mensagens, TON Messages. O projeto como um todo é visto como uma contramedida à censura na Internet.

O projeto foi desenvolvido com o CMake, por isso não tive dificuldades em criar e verificar. O código fonte está escrito em C ++ 14 e é executado em 210 mil LOC:

Como o projeto é pequeno e de alta qualidade, não há muitos bugs, mas eles ainda devem ser resolvidos.

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

Mensagem de diagnóstico do PVS-Studio: V601 O valor 'false' é convertido implicitamente no tipo inteiro. mc-config.cpp 884

Parece que a função retorna o tipo errado de status de erro aqui. Aparentemente, a função deve retornar um valor negativo para falha, em vez de verdadeiro / falso. É pelo menos o que faz mais no código, onde retorna -1.

Comparando uma variável consigo mesma

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

Mensagem de diagnóstico do PVS-Studio: V501 Existem subexpressões idênticas à esquerda e à direita do operador '==': zero_state_id_ == zero_state_id_ LastBlock.cpp 66

TON segue um padrão de codificação que prescreve que os nomes dos alunos terminem com um sublinhado. Em casos como esse, no entanto, essa notação pode levar a um erro, pois você corre o risco de ignorar o sublinhado. O nome do argumento passado para essa função é semelhante ao do membro da classe, o que facilita a mistura deles. É esse argumento que provavelmente deveria participar da comparação.

Macro não segura

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

Mensagem de diagnóstico do PVS-Studio: V581 As expressões condicionais das instruções 'if' localizadas uma ao lado da outra são idênticas. Verifique as linhas: 80, 84. blockdb.cpp 84

A condição dentro da macro CHECK nunca será executada, pois já foi verificada pela instrução if anterior.

Também há outro erro aqui: a macro CHECK é insegura, pois a condição dentro dela não está envolvida em uma construção do {...} while (0) . Essa embalagem é necessária para evitar colisões com outras condições no ramo else . Em outras palavras, o código a seguir não funcionaria conforme o esperado:

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

Verificando uma variável assinada

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

Mensagem de diagnóstico do PVS-Studio: V560 Uma parte da expressão condicional é sempre falsa: last == 0x80. boc.cpp 78

A segunda parte da condição nunca será executada porque o tipo char é assinado neste caso. Ao atribuir um valor a uma variável do tipo int , a extensão do sinal ocorrerá, portanto seus valores ainda estarão dentro do intervalo [-128, 127], não [0, 256].

Deve-se notar que char nem sempre é assinado: seu comportamento depende da plataforma e do compilador. Portanto, em teoria, a condição em questão ainda pode ser executada ao criar uma plataforma diferente.

Deslocando bit a bit um número negativo

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

Mensagem de diagnóstico do PVS-Studio: V610 Comportamento não especificado. Verifique o operador de turno '>>'. O operando esquerdo '-0x100' é negativo. bigint.hpp 1925

Executar uma operação de mudança à direita bit a bit em um número negativo é um comportamento não especificado: é impossível saber com antecedência se o sinal será estendido ou preenchido com zeros.

Verificação nula após nova

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

Mensagem de diagnóstico do PVS-Studio: V668 Não há sentido em testar o ponteiro 'c' contra nulo, pois a memória foi alocada usando o operador 'novo'. A exceção será gerada no caso de erro de alocação de memória. CellBuilder.cpp 531

A mensagem diz tudo: se a alocação de memória falhar, o programa lançará uma exceção em vez de retornar um ponteiro nulo. Isso significa que o cheque é inútil.

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

Mensagem de diagnóstico do PVS-Studio: A expressão 'caminho' da V547 é sempre verdadeira. fift-main.cpp 136

Esse trecho é retirado de um dos utilitários internos do projeto. O operador ternário é redundante neste caso: a condição que ele já verifica é verificada pela instrução if anterior. Parece que os desenvolvedores esqueceram de remover esse operador ternário quando decidiram descartar o uso de caminhos padrão (não há pelo menos nenhuma menção sobre eles na mensagem de ajuda).

Variável não 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); // <= } 

Mensagem de diagnóstico do PVS-Studio: V1001 A variável 'tmp_info' é atribuída, mas não é usada no final da função. analyzer.cpp 140

Aparentemente, os desenvolvedores usariam uma variável chamada tmp_info na última linha desta função. Aqui está o código dessa mesma função, mas com outros 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)); } 

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

Mensagem de diagnóstico do PVS-Studio: V1037 Dois ou mais ramos de casos executam as mesmas ações. Verifique as linhas: 639, 645 builtins.cpp 639

Se você ler atentamente, notou que esse código não possui uma operação <=. De fato, é nessa operação que o caso 6 deve lidar. Podemos deduzir isso observando dois pontos. O primeiro é o código de inicialização:

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

A função define_builtins , como você pode ver, contém uma chamada compile_cmp_int para o operador <= com o parâmetro mode definido como 6.

O segundo ponto é a própria função compile_cmp_int , que lista os nomes das operações:

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

O índice 6 corresponde à palavra LEQ , que significa "Menor ou igual".

É outro bom bug da classe de bugs encontrados nas funções de comparação .

Diversos

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

Mensagem de diagnóstico do PVS-Studio: V1003 A macro 'VM_LOG_IMPL' é uma expressão perigosa. O parâmetro 'mask' deve estar entre parênteses. log.h 23

A macro VM_LOG_IMPL não é segura. Seu segundo parâmetro não está entre parênteses, o que poderia causar efeitos colaterais indesejáveis ​​se uma expressão complexa for passada para a condição. Mas se a máscara for apenas uma constante, esse código será executado sem problemas. Dito isto, nada impede que você passe mais nada para a macro.

Conclusão

O TON acabou sendo muito pequeno, então existem poucos bugs para encontrar lá, pelos quais a equipe de desenvolvedores do Telegram certamente deve ser creditada. Mas todo mundo comete erros de vez em quando, mesmo esses caras. Os analisadores de código são ferramentas poderosas capazes de detectar pontos perigosos no código-fonte nos estágios iniciais de desenvolvimento, mesmo nas bases de código de maior qualidade, portanto, não os negligencie. A análise estática não deve ser executada de tempos em tempos, mas deve fazer parte do processo de desenvolvimento: " Introduzir a análise estática no processo, não basta procurar por erros ".