Проверка кода Telegram Open Network анализатором PVS-Studio
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.
Похоже, в этом месте разработчики перепутали возвращаемый статус ошибки. По всей видимости, функция должна возвращать отрицательное значение в случае неудачи, а не 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_
Код 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.
Условие внутри макроса CHECK никогда не выполнится, т.к. его проверка уже была внутри предыдущего if.
Тут есть и другая ошибка: макрос CHECK является небезопасным, т.к. условие внутри него не обернуто в конструкцию do {… } while (0). Это нужно, чтобы избежать коллизий с другими условиями в else. Другими словами, вот такой код будет работать не так, как ожидается:
if (X)
CHECK(condition)
else
foo();Проверка знаковой переменной
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: V560 A part of conditional expression is always false: last == 0×80.
Вторая часть условия никогда не выполнится, т.к. тип char в данном случае имеет знак. При присвоении значения переменной типа int произойдет расширение знака, поэтому диапазон значений переменной все равно останется в пределе [-128, 127], а не в [0, 256].
Стоит отметить, что тип char не всегда будет знаковым — такое поведение зависит от платформы и компилятора. Так что это условие все равно может теоретически выполниться при сборке на другой платформе.
Побитовый сдвиг отрицательного числа
template
bool AnyIntView::export_bits_any(....) const {
....
int mask = (-0x100 >> offs) & 0xff;
....
}
Предупреждение PVS-Studio: V610 Unspecified behavior. Check the shift operator '>>'. The left operand '-0×100' is negative.
Операция побитового сдвига отрицательного числа вправо является неуточнённым поведением: неизвестно, как будет вести себя знак в таком случае — расширится или заполнится нулями?
Проверка на null после new
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.
Тут все понятно из предупреждения анализатора — в случае неудачного выделения памяти будет брошено исключение, а не возвращён нулевой указатель. Проверка не имеет смысла.
Повторная проверка
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.
Этот код был взят в одной из внутренних утилит. Тернарный оператор в данном случае лишний: условие, которое в нем проверяется, уже есть внутри предыдущего if. Скорее всего, тернарный оператор забыли убрать, когда хотели отказаться от стандартных путей (по крайней мере, про них ничего не сказано в сообщении о помощи).
Неиспользуемая переменная
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: V1001 The 'tmp_info' variable is assigned but is not used by the end of the function.
Видимо, в последней строке этой функции планировалось использовать переменную 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));
}
Больше или меньше?
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
Внимательные читатели могли заметить, что в этом коде отсутствует операция <=. И действительно, под номером 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));
....
}
В функции define_builtins можно увидеть вызов compile_cmp_int для <= с параметром mode, равным 6.
Ну и второе — это та же функция 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);
}
Под индексом 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.
Макрос VM_LOG_IMPL является небезопасным. Второй его параметр не окружен круглыми скобками — это может привести к побочным эффектам, если в условие передается сложное выражение. Если же mask — просто константа, то никаких проблем с этим кодом не будет, но ничто не мешает нам передать в макрос что-то еще.
В заключение
Код TON оказался не таким большим, и в нем не так много ошибок. За что, конечно же, стоит отдать должное программистам из команды Telegram. Однако от ошибок не застрахованы даже они. Анализатор кода — мощный инструмент, который способен находить опасные места на ранних этапах даже в качественных кодовых базах, поэтому его использованием не стоит пренебрегать. Статический анализ — это не инструмент для разовых проверок, а часть процесса разработки: «Внедряйте статический анализ в процесс, а не ищите с его помощью баги».
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Larin. Checking Telegram Open Network with PVS-Studio.
