Telegram Open Network (TON) — это платформа, созданная той же командой, которая разработала мессенджер Telegram. Помимо блокчейна, TON предоставляет большой набор услуг. Разработчики недавно сделали код платформы, написанный на 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 Значение false неявно приводится к целочисленному типу. 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 Слева и справа от оператора == одинаковые подвыражения: 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 Условные выражения операторов if, расположенных рядом, идентичны. Проверьте строки: 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 Часть условного выражения всегда ложна: 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 Неопределенное поведение. Проверьте оператора смены ››. Левый операнд ‘-0x100’ отрицательный. bigint.hpp 1925
Выполнение операции побитового сдвига вправо над отрицательным числом является неуказанным поведением: невозможно заранее знать, будет ли знак расширен или дополнен нулями.
Нулевая проверка после нового
CellBuilder* CellBuilder::make_copy() const {
CellBuilder* c = new CellBuilder();
if (!c) { // <=
throw CellWriteError();
}
....
}Диагностическое сообщение PVS-Studio: V668 Проверять указатель c на null нет смысла, так как память была выделена с помощью оператора new. Исключение будет сгенерировано в случае ошибки выделения памяти. 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 Выражение path всегда истинно. 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 Переменная tmp_info назначена, но к моменту завершения функции не используется. 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 Две и более кейс-ветки выполняют одинаковые действия. Строки проверки: 639, 645 встроенные.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 для оператора ‹= с параметром режима, установленным на 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, что означает «Меньше или равно».
Еще одна приятная ошибка из класса ошибок, встречающихся в функциях сравнения.
Разное
#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 Макрос VM_LOG_IMPL является опасным выражением. Параметр маска должен быть заключен в круглые скобки. лог.ч 23
Макрос VM_LOG_IMPL небезопасен. Его второй параметр не заключен в круглые скобки, что может привести к нежелательным побочным эффектам, если в условие будет передано сложное выражение. Но если mask просто константа, этот код будет работать без проблем. Тем не менее, ничто не мешает вам передать что-либо еще в макрос.
Вывод
TON оказался довольно маленьким, поэтому там можно найти несколько ошибок, за которые, безусловно, следует отдать должное команде разработчиков Telegram. Но все время от времени ошибаются, даже эти ребята. Анализаторы кода — мощные инструменты, способные выявлять опасные места в исходном коде на ранних стадиях разработки даже в самых качественных кодовых базах, так что не пренебрегайте ими. Статический анализ не предназначен для запуска время от времени, а должен быть частью процесса разработки: «Внедряйте статический анализ в процесс, а не просто ищите с его помощью ошибки».
