Проверка кода Telegram Open Network анализатором PVS-Studio

    Picture 1

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

    Введение


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

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

    Picture 5

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

    Код возврата


    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

    Операция побитового сдвига отрицательного числа вправо является неуточнённым поведением: неизвестно, как будет вести себя знак в таком случае — расширится или заполнится нулями?

    Проверка на 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. 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. Однако от ошибок не застрахованы даже они. Анализатор кода — мощный инструмент, который способен находить опасные места на ранних этапах даже в качественных кодовых базах, поэтому его использованием не стоит пренебрегать. Статический анализ — это не инструмент для разовых проверок, а часть процесса разработки: "Внедряйте статический анализ в процесс, а не ищите с его помощью баги".



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Larin. Checking Telegram Open Network with PVS-Studio.
    PVS-Studio
    461,50
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 35

      +17
      PVS Studio делает самую полезную рекламу на хабре. Думаю что можно смело подавать заявку на конкурс Телеграма и получать свой приз! Разработчики как настоящие джентльмены теперь обязаны жениться приобрести подписку или дать один из призов.

      Если кто интересуется, вот информация.

      Here’s how to submit your code for the blockchain contest (https://t.me/contest/102) depending on the goal at hand:

      1. Submit an archive containing your source code, a howto manual and a build script (if necessary) to @jobs_bot (choose Blockchain Contest).

      Or:

      2. Submit a pull request with the TON VM/compiler improvement on GitHub. Describe the issue in the comment to your pull request. In addition, submit the link to the pull request and an archive as described in 1. demonstrating the new features suggested in the pull requests to @jobs_bot.

      Or:

      3. Submit a pull request on GitHub. Describe the issue in the comment to the pull request. Submit the link to the pull request to @jobs_bot.
        +3
        Так там есть уже хоть один пруф, что TON имеет отношение к Telegram вообще?
          +7
          В телеграм от телеграм пришла рассылка с конкурсами, где, кроме всего прочего, предлагается конкурс на разработку смартконтрактов для этой платформы. Так что теперь нет ни одного сомнения, что он имеет отношение к телеге.
            +1
            А, ну вот и комментарий выше тоже о нём. Интересно, чего же они шифровались так до этого тогда? ;-) Там целую конспирологию успели развести.
              0
              Неплохая пиар стратегия, почему бы и нет. Развели конспирологию, долго и много обсуждали и все это — без больших затрат со стороны телеграмма.
                0
                Если у Apple до презентации нового айфона спросить про уже утёкшую информацию о нём, там точно так же ответят «достоверная информация только в официальных каналах, всё остальное мы не комментируем».
              0
              Кошелек с грамами готовятся звести: telegram.org/tos/wallet
              0
              Тут все понятно из предупреждения анализатора — в случае неудачного выделения памяти будет брошено исключение, а не возвращён нулевой указатель. Проверка не имеет смысла.
              А если поддержка исключений в принципе отключена в опциях компилятора, что произойдет? Или слинковано с nothrownew.obj для некоторых особо интересных компиляторов.
                +2

                gcc -fno-exceptions насколько я понимаю будет вызывать abort. Чтобы вместо исключения new возвращал nullptr надо вызывать new(std::nothrow).

                  0

                  А если у класса переопределен оператор new? Вызван же будет он, а вдруг он делает malloc?

                  +3
                  Тогда отключите эту диагностику и используйте анализатор дальше. Однако, подобный вариант чреват разнообразнейшими проблемами. Допустим, ваш код знает про особый new, но что делать с используемыми библиотеками, которые могут быть не готовы к такой шутке?
                    0

                    Во встроенном По, например, ексепшенов нет по умолчанию. Да и вообще они там редко применяются.

                      0
                      Там и динамическая аллокация обычно противопоказана.
                        0

                        Да, но она поддерживается компилятором по умолчанию. Т. е. Heap и оператор new никто не отменял. А исключения нет, они для Embedded C++ включаются отдельно, так же как и RTTI, например.

                          0
                          Тем не менее кучка (даже не куча) там маленькая, фрагментация очень быстро может убить работоспособность динамической аллокации. Если уж и аллоцируем что-то, то только при старте на всё память поделили — и больше ни удаления, ни новых аллокаций быть не должно. Только в этом случае есть возможность на что-то более-менее вменяемое рассчитывать.
                            0

                            Согласен.

                  –8
                  «За что, конечно же, стоит отдать должное программистам из команды Telegram. Однако от ошибок не застрахованы даже они.»
                  Как вариант это не случайные ошибки, а намеренно оставленные телегой для последующей эксплуатации. Трудно поверить в то, что телега нанимает быдлокодеров и не проверяет свой код анализаторами.
                    +1
                    Вы видимо никогда не работали в крупных, считающихся крутыми, технологических компаниях. Бардак есть всегда и везде, идеально вряд ли вообще хоть кто-то делает. Особенно на старте проекта когда он еще не оброс бюрократией и процессы могут быстро меняться.
                      +1
                      Это типичные ошибки крупного проекта, совершаемые по недосмотру и/или в запаре/дедлайне, а не быдловство, как вам кажется. Вы не программист, если таких ошибок никогда не совершали. И да, шапочку из фольги не забудьте надеть когда будете в Телеграм выходить, лишний тернарный оператор украдет ваши данные, видите, там вначале стоит единица, а это не иначе как часть номера вашей банковской карты, гнусные разрабы и сюда свои ручонки запихнули!
                      0
                      Извиняюсь, я могу ошибаться — я не разрабатываю на плюсах и сях, но
                      «Похоже, в этом месте разработчики перепутали возвращаемый статус ошибки. По всей видимости, функция должна возвращать отрицательное значение в случае неудачи, а не true/false. По крайней мере, именно возврат значения -1 можно наблюдать в коде ниже.» — весь код не приведен, но судя по объвлению переменной r, функция возвращает тип int, а не bool. Объясните пожалуйста в чем я не прав? Там же обычное true of false? r: -1
                        +1

                        В самом начале примера:


                            if ((shard.shard & 1) || cs.size_ext() != 0x20000) {
                              return false;                                     // <= Вот тут
                            }
                        +2
                        Может они комментарии специально вычищают перед тем, как выложить исходники? Этот их элитаризм уже начинает влиять на культуру разработки. Пишешь документацию и заботишься, чтобы тебя поняли? Ну значит ты глупый и никчемный, звезды так не делают.

                        Призеры чемпионата мира, очень быстро ставшие миллионерами. Может оно и правда круто, а вот публикация MTProto была как серия плевков, и еще эта попытка своего, особенного шифрования. Джобс кумир для Дурова, а сам Дуров теперь кумир для немалого числа разработчиков. И почему мне не нравится такой заносчивый стиль?
                          +1

                          Интересно, ругнётся ли PVS Studio на сравнение переменной типа float или double с самой собой? Это может использоваться для проверки, равна ли переменная NAN. Да и оператор "==" может быть перегружен.
                          В любом случае полезно предупредить об этом разработчика, а он пусть уже анализирует, правильный это код, или нет.

                            +4
                            Нет. Не будет ругаться. Это слишком часто встречается в коде. Это указано в документации на V501. Идея, что лучше ругаться на всё подряд, а программист уж разберётся, к сожалению, контрпродуктивна.
                              +3

                              Спасибо за ссылки. Почитал, с доводами согласен.

                            0

                            Нет ли в планах сделать инспекцию, которая ругается на магические числа типа 2, 3, 4, 5, 6?

                              0
                              В группе 64-х битных диагностик есть поиск 4, 8, может ещё каких-то чисел, кратных размеру указателя. А что особенного в других приведённых числах?
                                +1

                                В коде нельзя так использовать голые числа. Нужно завести константы со смысловыми названиями. В данном случае, должно быть что вроде:


                                const int LEQ = 6;

                                или enum.


                                Магические числа, как минимум, ухудшают читаемость кода, а зачастую приводят к ошибкам. В IDEA такие проверки имеются.

                                  +1
                                  В приблизительных математических расчётах вещи типа 2, 6, 24, 120 — нормальное явление.
                                    0
                                    Понятно что есть случаи когда имеет смысл такую проверку выключать. Но мне кажется это случаи более редкие чем те, где было бы неплохо иметь ее из коробки.
                                      0

                                      Если у числа есть размерность (метры, вольты, киловаттчасы), то вместо него в расчетах должен быть символ.

                                  +2
                                  Нет ли в планах сделать инспекцию, которая ругается на магические числа типа 2, 3, 4, 5, 6?
                                  Нет.

                                  Подобная инспекция относится к классу крайне шумных. Прошу прочитать эту заметку, чтобы понимать нашу позицию, что много предупреждений, не значит хорошо.

                                  Мы можем сделать подобную диагностику только в том случае, если она будет входить в один из поддерживаемых стандартов, таких как MISRA. Но кстати, даже в MISRA подобного нет. Есть поиск восьмеричных констант, беззнаковых без суффикса U и так далее, но не просто чисел.

                                  Ещё вариант, это реализация подобной диагностики в разделе «Customers Specific Requests» по запросу клиента. Они по умолчанию выключены. Становитесь нашим клиентом и сделаем подобную диагностику. А бесплатно такую ужасную диагностику мы делать не будем :).
                                    0

                                    С удовольствием бы стал вашим клиентом, даже без этой диагностики. К сожалению, пока не могу себе этого позволить.

                                      0
                                      Как я понял в PVS Studio начиная с версии 6.27 (2018 года) можно включить полноценную проверку на соответствие кода стандарту MISRA C и MISRA C++? https://www.viva64.com/ru/b/0596/
                                        +2
                                        На данный момент есть следующий список поддерживаемых правил и он постоянно пополняется.

                                  Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                                  Самое читаемое