Verificação do código do Telegram Open Network pelo analisador PVS-Studio

O Telegram Open Network (TON) é uma plataforma dos criadores do Telegram messenger, que, além do blockchain, contém um grande conjunto de serviços. Os desenvolvedores recentemente publicaram o código da plataforma escrito em C ++ e o publicaram no GitHub. Queríamos dar uma olhada no projeto antes de seu lançamento oficial.

1. Introdução

O Telegram Open Network é um conjunto de vários serviços. Por exemplo, o projeto tem seu próprio sistema de pagamento baseado na criptomoeda Gram, também há uma máquina virtual TON VM - contratos inteligentes funcionam nele. Há um serviço de mensagens - TON Messages. O projeto em si visa combater a censura na Internet.

O projeto usa o sistema de montagem do CMake, portanto não houve problemas especiais com a montagem e a verificação. O código está escrito em C ++ 14 e possui 210 mil linhas:

O projeto é pequeno e de alta qualidade; portanto, não houve muitos erros, mas eles merecem atenção e correção.

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

PVS-Studio Warning: V601 O valor 'false' é convertido implicitamente para o tipo inteiro. mc-config.cpp 884

Parece que, nesse ponto, os desenvolvedores alteraram o status do erro retornado. Aparentemente, a função deve retornar um valor negativo em caso de falha, e não verdadeiro / falso. Pelo menos, é o retorno de -1 que pode ser observado no código abaixo.

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

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

O código TON é escrito usando um padrão de codificação no qual os membros da classe são nomeados com um sublinhado no final. No entanto, essa ortografia, como neste caso, pode passar despercebida e levar a um erro. O parâmetro que entra nesta função tem um nome semelhante a um membro da classe, portanto é fácil confundi-lo. Provavelmente, era ele quem 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()); // <= .... } 

PVS-Studio Warning: 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á atendida, porque seu cheque já estava dentro do if anterior.

Há outro erro: a macro CHECK não é segura, porque a condição dentro dela não é agrupada em uma construção do [...} while (0) . Isso é para evitar colisões com outras condições no restante . Em outras palavras, esse código não funcionará conforme o esperado:

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

Validação de variável de sinal

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

Aviso 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á cumprida, porque digite char neste caso tem um sinal. Ao atribuir um valor a uma variável do tipo int , o sinal será expandido; portanto, o intervalo de valores da variável ainda permanecerá no limite [-128, 127] e não em [0, 256].

Vale ressaltar que o tipo de caractere nem sempre será assinado - esse comportamento depende da plataforma e do compilador. Portanto, essa condição ainda pode ser teoricamente preenchida ao montar em outra plataforma.

Mudança de bit de bit negativo

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

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

A operação de deslocamento bit a bit negativo para a direita é um comportamento não especificado: não se sabe como o sinal se comportará nesse caso - ele será expandido ou preenchido com zeros?

Verificar nulo após novo

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

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

Tudo fica claro com o aviso do analisador - em caso de alocação de memória sem êxito, uma exceção será lançada e um ponteiro nulo não será retornado. A verificação não faz sentido.

Verificar novamente

 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: A expressão V547 'path' é sempre verdadeira. fift-main.cpp 136

Esse código foi obtido em um dos utilitários internos. O operador ternário neste caso é supérfluo: a condição que está registrada já existe dentro do if anterior. Provavelmente, eles esqueceram de remover o operador ternário quando desejavam abandonar os caminhos padrão (pelo menos nada foi dito 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); // <= } 

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

Aparentemente, na última linha desta função, foi planejado usar a variável tmp_info . Aqui está o código para a 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)); } 

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

Aviso do PVS-Studio: V1037 Dois ou mais ramificações de casos executam as mesmas ações. Verifique as linhas: 639, 645 builtins.cpp 639

Leitores atentos podem ter notado que a operação <= está ausente neste código. E, de fato, deve ser o número 6. Isso pode ser encontrado em dois lugares. O primeiro é a 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)); .... } 

Na função define_builtins , você pode ver a chamada compile_cmp_int para <= com o parâmetro mode igual a 6.

Bem, a segunda é a mesma função compile_cmp_int , que possui 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); } 

Sob o índice 6, está a palavra LEQ , que significa Menor ou Igual.

Outro bug bonito relacionado à classe de erros nas funções de comparação .

Outros

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

PVS-Studio Warning: 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 - isso pode levar a efeitos colaterais se uma expressão complexa for passada para a condição. Se a máscara for apenas uma constante, não haverá problemas com esse código, mas nada nos impede de passar outra coisa para a macro.

Em conclusão

O código TON acabou por não ser tão grande e não há muitos erros nele. Pelo qual, é claro, vale a pena prestar homenagem aos programadores da equipe do Telegram. No entanto, mesmo eles não estão imunes a erros. O analisador de código é uma ferramenta poderosa capaz de encontrar lugares perigosos nos estágios iniciais, mesmo em bases de código de alta qualidade, portanto, seu uso não deve ser negligenciado. A análise estática não é uma ferramenta para verificações únicas, mas faz parte do processo de desenvolvimento: " Incorpore a análise estática ao processo e não procure por erros ".

Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Sergey Larin. Verificando a rede aberta do telegrama com o PVS-Studio .