Telegram Open Network (TON) - это платформа от создателей мессенджера Telegram, которая, помимо блокчейна, содержит в себе большой набор сервисов. Недавно разработчики опубликовали код платформы, написанный на C++, и разместили его на GitHub. Нам захотелось проверить проект перед его официальным запуском.
Telegram Open Network - это набор различных сервисов. К примеру, проект имеет свою платежную систему, основанную на криптовалюте Gram, есть и виртуальная машина TON VM - на ней работают смарт-контракты. Имеется сервис обмена сообщениями - TON Messages. Сам проект направлен на борьбу с интернет-цензурой.
Проект использует сборочную систему CMake, так что особых проблем со сборкой и проверкой не возникло. Код написан на языке C++14 и насчитывает 210 тысяч строк:
Проект маленький и качественный, поэтому ошибок оказалось не так много, но они достойны внимания и исправления.
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: V601 The 'false' value is implicitly cast to the integer type. mc-config.cpp 884
Похоже, в этом месте разработчики перепутали возвращаемый статус ошибки. По всей видимости, функция должна возвращать отрицательное значение в случае неудачи, а не true/false. По крайней мере, именно возврат значения -1 можно наблюдать в коде ниже.
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: 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 написан по стандарту кодирования, в котором члены класса именуются с подчеркиванием на конце. Однако такое написание, как в этом случае, может остаться незамеченным и привести к ошибке. Параметр, приходящий в эту функцию, имеет схожее имя с членом класса, поэтому их легко спутать. Скорее всего, именно он и должен участвовать в сравнении.
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: V581 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 80, 84. blockdb.cpp 84
Условие внутри макроса CHECK никогда не выполнится, т.к. его проверка уже была внутри предыдущего if.
Тут есть и другая ошибка: макрос CHECK является небезопасным, т.к. условие внутри него не обернуто в конструкцию do { .... } while (0). Это нужно, чтобы избежать коллизий с другими условиями в else. Другими словами, вот такой код будет работать не так, как ожидается:
if (X)
CHECK(condition)
else
foo();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: V560 A part of conditional expression is always false: last == 0x80. boc.cpp 78
Вторая часть условия никогда не выполнится, т.к. тип char в данном случае имеет знак. При присвоении значения переменной типа int произойдет расширение знака, поэтому диапазон значений переменной все равно останется в пределе [-128, 127], а не в [0, 256].
Стоит отметить, что тип char не всегда будет знаковым - такое поведение зависит от платформы и компилятора. Так что это условие все равно может теоретически выполниться при сборке на другой платформе.
template <class Tr>
bool AnyIntView<Tr>::export_bits_any(....) const {
....
int mask = (-0x100 >> offs) & 0xff;
....
}Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '-0x100' is negative. bigint.hpp 1925
Операция побитового сдвига отрицательного числа вправо является неуточнённым поведением: неизвестно, как будет вести себя знак в таком случае - расширится или заполнится нулями?
CellBuilder* CellBuilder::make_copy() const {
CellBuilder* c = new CellBuilder();
if (!c) { // <=
throw CellWriteError();
}
....
}Предупреждение PVS-Studio: 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
Тут все понятно из предупреждения анализатора - в случае неудачного выделения памяти будет брошено исключение, а не возвращён нулевой указатель. Проверка не имеет смысла.
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: V547 Expression 'path' is always true. fift-main.cpp 136
Этот код был взят в одной из внутренних утилит. Тернарный оператор в данном случае лишний: условие, которое в нем проверяется, уже есть внутри предыдущего if. Скорее всего, тернарный оператор забыли убрать, когда хотели отказаться от стандартных путей (по крайней мере, про них ничего не сказано в сообщении о помощи).
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: V1001 The 'tmp_info' variable is assigned but is not used by the end of the function. analyzer.cpp 140
Видимо, в последней строке этой функции планировалось использовать переменную tmp_info. Вот код той же функции, но с другими спецификаторами параметра:
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));
}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: V1037 Two or more case-branches perform the same actions. Check lines: 639, 645 builtins.cpp 639
Внимательные читатели могли заметить, что в этом коде отсутствует операция <=. И действительно, под номером 6 должна быть именно она. Это можно выяснить в двух местах. Первое - инициализация:
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));
....
}В функции define_builtins можно увидеть вызов compile_cmp_int для <= с параметром mode, равным 6.
Ну и второе - это та же функция compile_cmp_int, в которой есть имена операций:
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);
}Под индексом 6 как раз стоит слово LEQ, что значит Less or Equal.
Очередной красивый баг, относящийся к классу ошибок в функциях сравнения.
#define VM_LOG_IMPL(st, mask) \
LOG_IMPL_FULL(get_log_interface(st), ...., VERBOSITY_NAME(DEBUG), \
(get_log_mask(st) & mask) != 0, "") // <=Предупреждение PVS-Studio: V1003 The macro 'VM_LOG_IMPL' is a dangerous expression. The parameter 'mask' must be surrounded by parentheses. log.h 23
Макрос VM_LOG_IMPL является небезопасным. Второй его параметр не окружен круглыми скобками - это может привести к побочным эффектам, если в условие передается сложное выражение. Если же mask - просто константа, то никаких проблем с этим кодом не будет, но ничто не мешает нам передать в макрос что-то еще.
Код TON оказался не таким большим, и в нем не так много ошибок. За что, конечно же, стоит отдать должное программистам из команды Telegram. Однако от ошибок не застрахованы даже они. Анализатор кода - мощный инструмент, который способен находить опасные места на ранних этапах даже в качественных кодовых базах, поэтому его использованием не стоит пренебрегать. Статический анализ - это не инструмент для разовых проверок, а часть процесса разработки: "Внедряйте статический анализ в процесс, а не ищите с его помощью баги".