www.machinelearningmastery.ru

Машинное обучение, нейронные сети, искусственный интеллект
Header decor

Home

Проверка Telegram в открытой сети с помощью PVS-Studio

Дата публикации Oct 3, 2019

Telegram Open Network (TON) - это платформа той же команды, которая разработала мессенджер Telegram. В дополнение к блокчейну, TON предоставляет большой набор услуг. Разработчики недавно сделали код платформы, написанный на C ++, общедоступным и загрузили его на GitHub. Мы решили проверить проект до его официального релиза.

Введение

Telegram Открытая Сетьэто набор различных услуг. Помимо прочего, он предоставляет собственную платежную систему на основе криптовалюты Gram и виртуальную машину под названием TON VM, которая выполняет интеллектуальные контракты. Он также предлагает услугу обмена сообщениями TON Messages. Проект в целом рассматривается как противодействие интернет-цензуре.

Проект построен с использованием CMake, поэтому у меня не было никаких проблем с его сборкой и проверкой. Исходный код написан на C ++ 14 и работает с 210 тыс. LOC:

Поскольку проект небольшой и качественный, в нем не так много ошибок, но с ними все же нужно бороться.

Код возврата

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Условные выражения операторов «если», расположенные рядом друг с другом, идентичны. Проверьте строки: 80, 84. blockdb.cpp 84

Состояние внутриЧЕКмакрос никогда не будет выполнен, так как он уже был проверен предыдущимеслизаявление.

Здесь также присутствует другая ошибка:ЧЕКмакрос небезопасен, так как условие внутри него не заключено вделать { …. } while (0)построить. Такая упаковка необходима, чтобы избежать столкновений с другими условиями вещеветка. Другими словами, следующий код не будет работать должным образом:

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

Вторая часть условия никогда не будет выполнена, потому что типголецподписан в этом случае. При назначении значения переменной типаИНТпроизойдет расширение знака, поэтому его значения будут по-прежнему лежать в диапазоне [-128, 127], а не [0, 256].

Следует отметить, чтоголецне всегда подписан: его поведение зависит от платформы и компилятора. Таким образом, теоретически, данное условие может выполняться при сборке на другой платформе.

Побитовое смещение отрицательного числа

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» на ноль, поскольку память была выделена с помощью оператора «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Выражение «путь» всегда верно. fift-main.cpp 136

Этот фрагмент взят из одной из внутренних утилит проекта. В этом случае троичный оператор избыточен: проверяемое им условие уже проверено предыдущимеслизаявление. Похоже, разработчики забыли удалить этот троичный оператор, когда решили отказаться от использования стандартных путей (по крайней мере, нет упоминаний о них в справочном сообщении).

Неиспользуемая переменная

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Две или более ветвей case выполняют одинаковые действия. Проверьте строки: 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для<=оператор с параметром режима установленным в 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» является опасным выражением. Параметр «маска» должен быть заключен в круглые скобки. log.h 23

VM_LOG_IMPLмакрос небезопасен. Его второй параметр не заключен в круглые скобки, что может привести к нежелательным побочным эффектам, если в условие передается сложное выражение. Но еслимаскироватьэто просто константа, этот код будет работать без проблем вообще. Тем не менее, ничто не мешает вам передавать что-либо еще в макрос.

Вывод

TON оказался довольно маленьким, поэтому там есть несколько ошибок, за которые команда разработчиков Telegram, безусловно, должна отдать должное. Но все время от времени делают ошибки, даже эти парни. Анализаторы кода - это мощные инструменты, способные обнаруживать опасные места в исходном коде на ранних этапах разработки даже в самых качественных кодах, поэтому не пренебрегайте ими. Статический анализ не предназначен для проведения время от времени, но должен быть частью процесса разработки: «Внедрить статический анализ в процесс, а не просто искать с ним ошибки».

Оригинальная статья

Footer decor

© www.machinelearningmastery.ru | Ссылки на оригиналы и авторов сохранены. | map