Verification of Telegram Open Network code by PVS-Studio analyzer

Telegram Open Network (TON) is a platform from the creators of the Telegram messenger, which, in addition to the blockchain, contains a large set of services. Developers recently published platform code written in C ++ and posted it on GitHub. We wanted to check the project before its official launch.

Introduction

Telegram Open Network is a set of various services. For example, the project has its own payment system based on the Gram cryptocurrency, there is also a TON VM virtual machine - smart contracts work on it. There is a messaging service - TON Messages. The project itself aims to combat Internet censorship.



The project uses the CMake assembly system, so there were no special problems with assembly and verification. The code is written in C ++ 14 and has 210 thousand lines:

The project is small and of high quality, so there were not many errors, but they are worthy of attention and correction.

Return code

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 The 'false' value is implicitly cast to the integer type. mc-config.cpp 884



It seems that at this point the developers have mixed up the returned error status. Apparently, the function should return a negative value in case of failure, and not true / false. At least, it is the return of -1 that can be observed in the code below.

Comparing a variable with itself

 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 There are identical sub-expressions to the left and to the right of the '==' operator: zero_state_id_ == zero_state_id_ LastBlock.cpp 66



TON code is written using a coding standard in which class members are named with an underscore at the end. However, such a spelling, as in this case, may go unnoticed and lead to an error. The parameter coming into this function has a similar name to a member of the class, so it is easy to confuse them. Most likely, it was he who should participate in the comparison.

Unsafe macro

 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 The conditional expressions of the 'if' statements located alongside each other are identical. Check lines: 80, 84. blockdb.cpp 84



The condition inside the CHECK macro will never be fulfilled, because its check was already inside the previous if .



There is another mistake: the CHECK macro is unsafe, because the condition inside it is not wrapped in a do {...} while (0) construct. This is to avoid collisions with other conditions in else . In other words, this code will not work as expected:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Sign Variable Validation

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio warning : V560 A part of conditional expression is always false: last == 0x80. boc.cpp 78



The second part of the condition will never be fulfilled, because type char in this case has a sign. When assigning a value to a variable of type int , the sign will expand, so the range of values ​​of the variable will still remain in the limit [-128, 127] and not in [0, 256].



It is worth noting that the char type will not always be signed - this behavior depends on the platform and the compiler. So this condition can still theoretically be fulfilled when assembling on another platform.

Negative Bit Bit Shift

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '-0x100' is negative. bigint.hpp 1925



The operation of bitwise shifting a negative number to the right is an unspecified behavior: it is not known how the sign will behave in this case - will it expand or fill with zeros?

Check for null after new

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning: V668 There is no sense in testing the 'c' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. CellBuilder.cpp 531



Everything is clear from the analyzer warning - in case of unsuccessful memory allocation, an exception will be thrown, and a null pointer will not be returned. Verification does not make sense.

Recheck

 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' is always true. fift-main.cpp 136



This code was taken in one of the internal utilities. The ternary operator in this case is superfluous: the condition that is checked in it already exists inside the previous if . Most likely, they forgot to remove the ternary operator when they wanted to abandon the standard paths (at least nothing was said about them in the help message).

Unused variable

 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 The 'tmp_info' variable is assigned but is not used by the end of the function. analyzer.cpp 140



Apparently, in the last line of this function, it was planned to use the tmp_info variable. Here is the code for the same function, but with different parameter specifiers:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

More or less?

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning: V1037 Two or more case-branches perform the same actions. Check lines: 639, 645 builtins.cpp 639



Attentive readers may have noticed that the <= operation is missing in this code. And indeed, it should be number 6. This can be found in two places. The first is initialization:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In the define_builtins function, you can see the compile_cmp_int call for <= with the mode parameter equal to 6.



Well, the second is the same compile_cmp_int function, in which there are operation names:

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

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Under the index 6 is the word LEQ , which means Less or Equal.



Another beautiful bug related to the class of errors in comparison functions .

Other

 #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 The macro 'VM_LOG_IMPL' is a dangerous expression. The parameter 'mask' must be surrounded by parentheses. log.h 23



The macro VM_LOG_IMPL is unsafe. Its second parameter is not surrounded by parentheses - this can lead to side effects if a complex expression is passed to the condition. If mask is just a constant, then there will be no problems with this code, but nothing prevents us from passing something else into the macro.

Finally

The TON code turned out to be not so big, and it does not have many errors. For which, of course, it is worth paying tribute to the programmers from the Telegram team. However, even they are not immune from errors. The code analyzer is a powerful tool that is able to find dangerous places in the early stages even in high-quality code bases, so its use should not be neglected. Static analysis is not a tool for one-time checks, but part of the development process: " Embed static analysis into the process, and do not look for bugs with it ."

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Larin. Checking Telegram Open Network with PVS-Studio .