pvs-stdio ue4

Telegram Open Network (TON) is a platform by the same team that developed the Telegram messenger. In addition to the blockchain, TON provides a large set of services. The developers recently made the platform's code, which is written in C++, publicly available and uploaded it to GitHub. We decided to check the project before its official release.

Telegram开放网络(TON)是开发Telegram Messenger的同一团队的平台。 除了区块链，TON还提供大量服务。 开发人员最近使该平台的代码(用C ++编写)公开可用，并将其上载到GitHub。 我们决定在项目正式发布之前对其进行检查。

介绍 (Introduction)

is a set of various services. Among other things, it provides a payment system of its own based on the Gram cryptocurrency, and a virtual machine called TON VM, which executes smart contracts. It also offers a messaging service, TON Messages. The project as a whole is seen as a countermeasure to Internet censorship. 是一组各种服务。 除其他事项外，它提供了基于Gram加密货币的自己的支付系统，以及一个执行TON的虚拟机TON VM。 它还提供了消息服务TON Messages。 整个项目被视为对互联网审查的对策。

The project is built with CMake, so I didn't have any difficulties building and checking it. The source code is written in C++14 and runs to 210 thousand LOC:

该项目是使用CMake构建的，因此我对构建和检查它没有任何困难。 源代码用C ++ 14编写，运行到21万个LOC：

Since the project is a small and high-quality one, there aren't many bugs in it, but they still should be dealt with.

由于该项目是一个小型且高质量的项目，因此其中没有很多错误，但仍应加以解决。

返回码 (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 diagnostic message: The 'false' value is implicitly cast to the integer type. mc-config.cpp 884

PVS-Studio诊断消息： '值隐式转换为整数类型。 mc-config.cpp 884

It looks like the function returns the wrong type of error status here. The function should apparently return a negative value for failure rather than true/false. That's at least what it does further in the code, where it returns -1.

看起来函数在这里返回了错误的错误状态类型。 该函数显然应该为失败返回负值，而不是true / false。 至少这是在代码中进一步执行的操作，返回-1。

将变量与其自身进行比较 (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 diagnostic message: There are identical sub-expressions to the left and to the right of the '==' operator: zero_state_id_ == zero_state_id_ LastBlock.cpp 66

PVS-Studio诊断消息： '=='运算符左右两侧有相同的子表达式：zero_state_id_ == zero_state_id_ LastBlock.cpp 66

TON follows a coding standard that prescribes that class members' names should end in an underscore. In cases like this, however, this notation may lead to a bug as you risk overlooking the underscore. The name of the argument passed to this function is similar to that of the class member, which makes it easy to mix them up. It is this argument that was most likely meant to participate in the comparison.

TON遵循编码标准，该标准规定类成员的名称应以下划线结尾。 但是，在这种情况下，这种表示法可能会导致错误，因为您可能会忽略下划线。 传递给此函数的参数名称与类成员的名称相似，这使得将它们混合起来变得容易。 正是这种说法最有可能参与比较。

不安全的宏 (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 diagnostic message: The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 80, 84. blockdb.cpp 84

PVS-Studio诊断消息： 彼此并排的'if'语句的条件表达式相同。 检查行：80，84。blockdb.cpp 84

The condition inside the CHECK macro will never execute as it has been already checked by the previous if statement.

CHECK宏中的条件将永远不会执行，因为前面的if语句已经检查过了。

There's also another error present here: the CHECK macro is unsafe since the condition inside it is not wrapped in a do {… } while (0) construct. Such wrapping is needed to avoid collisions with other conditions in the else branch. In other words, the following code wouldn't work as expected:

这里还存在另一个错误： CHECK宏是不安全的，因为它的内部条件没有包装在do {…} while(0)构造中。 需要这种包装以避免与else分支中的其他条件冲突。 换句话说，以下代码无法按预期工作：

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

检查有符号变量 (Checking a signed variable)

class Slice {  ....  char operator[](size_t i) const;  ....};td::Result
    
      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 diagnostic message: A part of conditional expression is always false: last == 0x80. boc.cpp 78

PVS-Studio诊断消息： 条件表达式的一部分始终为false：last == 0x80。 boc.cpp 78

The second part of the condition will never execute because the type char is signed in this case. When assigning a value to a variable of type int, sign extension will occur, so its values will still lie within the range [-128, 127], not [0, 256].

条件的第二部分将永远不会执行，因为在这种情况下，类型为char是签名的。 当将值赋给int类型的变量时，将发生符号扩展，因此其值仍将位于[-128，127]范围内，而不是[0，256]。

It should be noted that char is not always signed: its behavior is platform- and compiler-dependent. So in theory, the condition in question could still execute when building on a different platform.

应该注意的是， char并非总是签名的：它的行为取决于平台和编译器。 因此，从理论上讲，当在其他平台上构建时，所讨论的条件仍然可以执行。

负数按位移位 (Bitwise-shifting a negative number)

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

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

PVS-Studio诊断消息： 未指定的行为。 检查移位运算符“ >>”。 左操作数'-0x100'为负。 bigint.hpp 1925

Executing a bitwise right shift operation on a negative number is unspecified behavior: it's impossible to know in advance if the sign will be extended or padded with zeroes.

对负数执行按位右移运算是未指定的行为：无法事先知道符号是扩展还是填充零。

新后空检查 (Null check after new)

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

PVS-Studio diagnostic message: 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

PVS-Studio诊断消息： 因为将内存使用“ new”运算符分配，所以没有针对空测试“ c”指针的意义。 如果内存分配错误，将生成异常。 CellBuilder.cpp 531

The message says it all: if memory allocation fails, the program will throw an exception rather than return a null pointer. It means the check is pointless.

消息说明了一切：如果内存分配失败，程序将引发异常，而不是返回空指针。 这意味着检查没有意义。

冗余检查 (Redundant check)

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 diagnostic message: Expression 'path' is always true. fift-main.cpp 136

PVS-Studio诊断消息： 表达式“路径”始终为真。 第136章

This snippet is taken from one of the project's internal utilities. The ternary operator is redundant in this case: the condition it checks is already checked by the previous if statement. It looks like the developers forgot to remove this ternary operator when they decided to discard the use of standard paths (there's at least no mention of those in the help message).

此摘录摘自项目的内部实用程序之一。 在这种情况下，三元运算符是多余的：它检查的条件已经由前面的if语句检查了。 当开发人员决定放弃使用标准路径时，看起来好像忘记了删除此三元运算符(帮助消息中至少没有提及那些路径)。

未使用的变量 (Unused variable)

bool Op::set_var_info_except(const VarDescrList& new_var_info,                        const std::vector
    
     & 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 diagnostic message: The 'tmp_info' variable is assigned but is not used by the end of the function. analyzer.cpp 140

PVS-Studio诊断消息： 分配了'tmp_info'变量，但该变量未在功能结束时使用。 分析器.cpp 140

The developers were apparently going to use a variable named tmp_info in the last line of this function. Here's the code of that same function but with other parameter specifiers:

开发人员显然将在此函数的最后一行中使用一个名为tmp_info的变量。 这是相同功能但带有其他参数说明符的代码：

bool Op::set_var_info_except(VarDescrList&& new_var_info,                        const std::vector
    
     & var_list) {  if (var_list.size()) {    new_var_info -= var_list; // <=  }  return set_var_info(std::move(new_var_info));}
    

大于或小于？ (Greater or less than?)

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 diagnostic message: Two or more case-branches perform the same actions. Check lines: 639, 645 builtins.cpp 639

PVS-Studio诊断消息： 两个或更多案例分支执行相同的操作。 检查行：639，645 Builtins.cpp 639

If you read carefully, you noticed that this code lacks a <= operation. Indeed, it is this operation that case 6 should be dealing with. We can deduce that by looking at two spots. The first is the initialization code:

如果仔细阅读，您会发现该代码缺少<=操作。 确实，案例6应该处理的就是这个操作。 我们可以通过观察两个点来推断出这一点。 第一个是初始化代码：

AsmOp compile_cmp_int(std::vector
    
     & res, std::vector
     
      & 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));  ....}
     
    

The define_builtins function, as you can see, contains a call compile_cmp_int for the <= operator with the mode parameter set to 6.

如您所见， define_builtins函数包含对<=运算符的调用compile_cmp_int ，其mode参数设置为6。

The second spot is the compile_cmp_int function itself, which lists the names of operations:

第二点是compile_cmp_int函数本身，其中列出了操作名称：

AsmOp compile_cmp_int(std::vector
    
     & res, std::vector
     
      & args,                      int mode) {  ....  static const char* cmp_names[] = {"", "GREATER", "EQUAL", "GEQ", "LESS",                                    "NEQ", "LEQ", "CMP"};  ....  return exec_op(cmp_names[mode], 2);}
     
    

Index 6 corresponds to the LEQ word, which means «Less or Equal».

索引6对应于LEQ单词，表示«Less or Equal»。

It's another nice bug of the .

这是的 。

杂 (Miscellaneous)

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

PVS-Studio diagnostic message: The macro 'VM_LOG_IMPL' is a dangerous expression. The parameter 'mask' must be surrounded by parentheses. log.h 23

PVS-Studio诊断消息： 宏“ VM_LOG_IMPL”是危险的表达。 参数“掩码”必须用括号括起来。 log.h 23

The VM_LOG_IMPL macro is unsafe. Its second parameter is not enclosed in parentheses, which could potentially cause undesirable side effects if a complex expression is passed to the condition. But if mask is just a constant, this code will run with no problems at all. That said, nothing prevents you from passing anything else to the macro.

VM_LOG_IMPL宏不安全。 它的第二个参数未括在括号中，如果将复杂的表达式传递给该条件，则有可能引起不良的副作用。 但是，如果mask只是一个常量，那么此代码将完全没有问题地运行。 也就是说，没有什么可以阻止您将其他任何内容传递给宏。

结论 (Conclusion)

TON turned out to be pretty small, so there are few bugs to find there, which the Telegram developer team should certainly be given credit for. But everyone makes mistakes every now and then, even these guys. Code analyzers are powerful tools capable of detecting dangerous spots in source code at the early development stages even in the most quality code bases, so don't neglect them. Static analysis is not meant to be run from time to time but should be part of the development process: "".

TON的体积非常小，因此几乎找不到bug，Telegram开发人员团队当然应该对此予以称赞。 但是每个人甚至有时都会犯错。 代码分析器是功能强大的工具，即使在最优质的代码库中，也可以在早期开发阶段检测源代码中的危险点，因此请不要忽略它们。 静态分析并不是要不时地运行，而应该成为开发过程的一部分：“ ”。

翻译自:

pvs-stdio ue4