Проверка Bitcoin

    Bitcoin, PVS-Studio
    Ничего эпического в этой статье не будет. Мы проверили с помощью PVS-Studio исходный код Bitcoin. Нашли всего пару подозрительных мест. Это не удивительно. Думаю, эти исходные коды не проверял только ленивый. Но раз проверили, то решил написать маленькую заметку. Так сказать, «для галочки».

    Началось всё с того, что мы задумались провести сравнение PVS-Studio и Clang на открытых проектах. Задача большая, сложная и думаю сделаем мы её не скоро. Сложность представляют следующие моменты:
    1. Надо найти проекты, которые собираются с помощью GCC, но при этом их можно собрать с помощью Clang. Если мы будем проверять проекты, которые уже ориентированы на Clang, это будет нечестно. Clang в них естественно ничего не найдёт, так как ошибки уж поправлены. А PVS-Studio найдёт.
    2. Анализатору PVS-Studio приходится играть на чужом поле под названием «Linux». Почти нет проектов, которые можно одновременно собрать с помощью Clang и Visual Studio. Теоретически говорится, что Clang уже хорошо совместим с Visual Studio. На практике это неправда. Не удаётся собрать и проверить многие проекты. PVS-Studio в свою очередь плохо проверяет проекты в Linux. В результате приходится выбирать проекты, с которыми одинаково удачно могут работать оба инструмента.

    Одним из проектов, выбранных для сравнения стал Bitcoin. Оба анализатора в нём почти ничего не нашли. Это не удивительно. Думаю, этот проект проверялся уже многими инструментами. Соответственно, этот проект, скорее всего, не войдёт в сравнение. Поэтому, пусть останется хотя бы эта краткая заметка.

    Анализ проекта


    Описывать, что такое Bitcoin не нужно. Исходные коды взяты с помощью:

    git clone https://github.com/bitcoin/bitcoin.git

    Анализ производился с помощью PVS-Studio версии 5.17.

    Странный цикл


    Нашлось только одно место, интересное на мой взгляд. Это какая-то функция, относящаяся к криптографии. Что она делает я не знаю. Возможно, я нашел EPIC FAIL. Сейчас модно находить эпические ошибки, связанные с безопасностью. Однако скорее всего, это мелкая ошибка или даже вовсе, так и было задумано.
    bool CCryptoKeyStore::Unlock(const CKeyingMaterial& vMasterKeyIn)
    {
      {
        LOCK(cs_KeyStore);
        if (!SetCrypted())
          return false;
    
        CryptedKeyMap::const_iterator mi = mapCryptedKeys.begin();
        for (; mi != mapCryptedKeys.end(); ++mi)
        {
          const CPubKey &vchPubKey = (*mi).second.first;
          const std::vector<unsigned char> &vchCryptedSecret =
            (*mi).second.second;
          CKeyingMaterial vchSecret;
          if(!DecryptSecret(vMasterKeyIn, vchCryptedSecret,
                            vchPubKey.GetHash(), vchSecret))
              return false;
          if (vchSecret.size() != 32)
              return false;
          CKey key;
          key.Set(vchSecret.begin(), vchSecret.end(),
                  vchPubKey.IsCompressed());
          if (key.GetPubKey() == vchPubKey)
              break;
          return false;
        }
        vMasterKey = vMasterKeyIn;
      }
      NotifyStatusChanged(this);
      return true;
    }

    Предупреждение PVS-Studio: V612 An unconditional 'return' within a loop. crypter.cpp 169

    Обратите внимание на цикл. В нем должны перебираться какие-то ключи. Но тело цикла выполняется только один раз. В конце цикла расположен оператор «return false;». Цикл также, может быть прерван досрочно с помощью операторов «break;». При этом в теле цикла нет ни одного оператора «continue;».

    Подозрительный сдвиг


    static int64_t set_vch(const std::vector<unsigned char>& vch)
    {
      if (vch.empty())
        return 0;
    
      int64_t result = 0;
      for (size_t i = 0; i != vch.size(); ++i)
          result |= static_cast<int64_t>(vch[i]) << 8*i;
    
      // If the input vector's most significant byte is 0x80,
      // remove it from the result's msb and return a negative.
      if (vch.back() & 0x80)
          return -(result & ~(0x80 << (8 * (vch.size() - 1))));
    
       return result;
    }

    Предупреждение PVS-Studio: V629 Consider inspecting the '0x80 << (8 * (vch.size() — 1))' expression. Bit shifting of the 32-bit value with a subsequent expansion to the 64-bit type. script.h 169

    Функция формирует 64-битное значение. Один сдвиг сделан правильно, а второй возможно нет.

    Правильно:
    static_cast<int64_t>(vch[i]) << 8*i

    В начале переменная расширяется до int64_t и только потом происходит сдвиг.

    Подозрительно:
    0x80 << (8 * (vch.size() - 1))

    Константа 0x80 имеет тип 'int'. Это значит, что при сдвиге может произойти переполнение. Так как функция генерирует 64-битное значение, то я подозреваю наличие ошибки. Подробнее про сдвиги я писал в статье "Не зная брода, не лезь в воду — часть третья".

    Безопасный вариант:
    0x80ull << (8 * (vch.size() - 1))

    Опасные классы


    class CKey {
      ....
      // Copy constructor. This is necessary because of memlocking.
      CKey(const CKey &secret) : fValid(secret.fValid),
                                 fCompressed(secret.fCompressed) {
        LockObject(vch);
        memcpy(vch, secret.vch, sizeof(vch));
      }
      ....
    };

    Предупреждение PVS-Studio: V690 The 'CKey' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. key.h 175

    Как следует из текста комментария, конструктор копирования нужен для осуществления синхронизации. Однако, скопировать объект можно не только с помощью конструктора копирования, но и с помощью operator =. А этот оператор не реализован. Даже если сейчас operator = нигде не используется это всё равно потенциально опасно.

    Аналогично, стоит обратить внимание ещё на несколько классов:
    • V690 The 'Semantic_actions' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. json_spirit_reader_template.h 196
    • V690 The 'CFeeRate' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. core.h 118
    • V690 The 'CTransaction' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. core.h 212
    • V690 The 'CTxMemPoolEntry' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. txmempool.h 27
    • V690 The 'Json_grammer' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. json_spirit_reader_template.h 370
    • V690 The 'Generator' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. json_spirit_writer_template.h 98

    Заключение


    Регулярное использование статических анализаторов может сэкономить массу времени и нервных клеток. Главное, чтобы это было удобно. Например, попробуйте инкрементальный анализ в PVS-Studio. Просто пишешь код и если что, тебя остановят. К хорошему быстро привыкаешь.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. http://www.viva64.com/en/b/0268/.

    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio 352,20
    Ищем ошибки в C, C++ и C# на Windows и Linux
    Поделиться публикацией
    Комментарии 42
    • –42
      в биткоин фаундейшен отправляли статью? что они сказали?
      • +65
        Нет, нет. Что вы. Зачем. Вдруг мы нашли настоящую уязвимость. Пусть её поиспользуют. Думаю, это будет забавно. А так — поправят тихо и всё. Не интересно.
        • +5
          Отправили, отправили, в конце статьи ссылка на FAQ есть
      • 0
        Подскажите доступные статические анализаторы для IDEA (Android Studio) и Xcode (Objective C).
        • –9
          Есть мысль, что они там особо не нужны, так как маленькие проекты очень.
          • +3
            Право, не стоит обобщать.
          • +1
            Cmd+Shift+B в Xcode запускает Clang Analyzer. Вполне неплох.
            • +1
              Скажите, а лично Вы пользуетесь им на регулярной основе?
              • +2
                Да, пользуюсь. Раз в день примерно — достаточно регулярно? Единственная проблема — приходится его отключать для сторонних Cocoa Pods.
                • +1
                  Т.е. проверяете код, который пишите? Какой сценарий использования? Опишите, пожалуйста, как вы им пользуетесь — локально, на билд-сервере и т.п.
                  • 0
                    Проверяю где-то раз в день всё, что написал. На билд-сервере пока не настраивал анализ.
                • +1
                  У меня лично статический анализатор включен в Xcode по-умолчанию, так как вручную я всегда забывал это сделать. Хватает ошибки-опечатки во время сборки. Сама сборка занимает чуть больше времени из-за анализа, но разница слишком маленькая, что бы мне это было критично.
                • 0
                  Он ничего не находит даже в явно плохом коде. И часто выдает ложные срабатывания. Показался достаточно бесполезным. В IDEA, тоже, конечно, свой анализатор встроен, но кто знает, что еще для них написано?
                  • 0
                    Хм, мне он много раз подсказывал подозрительные места. -Werror -Wall -Wpedantic тоже помогают.
                    • 0
                      Какой объем проекта, который компилируете с -Werror -Wall -Wpedantic?
                      • 0
                        Включаю для новых проектов сразу же, старые править — лишь время терять. =)

                        По текущему проекту пока такая статистика:

                        $ cloc .
                             120 text files.
                             120 unique files.
                              10 files ignored.
                        
                        http://cloc.sourceforge.net v 1.60  T=0.51 s (216.2 files/s, 10609.2 lines/s)
                        -------------------------------------------------------------------------------
                        Language                     files          blank        comment           code
                        -------------------------------------------------------------------------------
                        Objective C                     55            869            442           2965
                        C/C++ Header                    55            251            510            360
                        -------------------------------------------------------------------------------
                        SUM:                           110           1120            952           3325
                        -------------------------------------------------------------------------------
                        
                        • 0
                          Это без учёта сторонних библиотек и Cocoa Pods.
              • 0
                А что clang там нашел?
                • 0
                  Ничего интересного. Несколько сообщений в духе «переменная не используется» и т.п.
                • +1
                  «Странный цикл» — это обработка первого элемента в итерируемой коллекции.
                  «Это нормально» (С)
                  • +10
                    и зачем тогда там цикл, если можно было бы просто взять первый элемент как и сейчас?
                    • 0
                      первого элемента может не быть
                      • +15
                        и зачем там for, когда хватило бы if?
                      • +2
                        Вообще похоже что задумано так, что если в коллекции есть хотя бы 1 элемент, то функция возвращает false. Но если это так, то такой стиль программирования, мягко говоря не очень.
                        • 0
                          >Вообще похоже что задумано так, что если в коллекции есть хотя бы 1 элемент, то функция возвращает false

                          Не совсем. Если сработает break, то будет возвращено true.
                          • 0
                            тогда для 1 элемента вместо break следовало сразу ставить return true…
                            • 0
                              Возможно и следовало бы, если бы не

                              1. выполнение vMasterKey = vMasterKeyIn;
                              2. анлок cs_KeyStore при выходе из области видимости
                              3. выполнение NotifyStatusChanged(this);

                              причем именно в этом порядке.
                              А уж только потом return true;
                              • 0
                                Ну во-1х для этого предполагаются try { } finally. Но на сях его нет, это да.

                                тогда, имхо, надо писать так:
                                   CryptedKeyMap::const_iterator mi = mapCryptedKeys.begin();
                                    for (; mi != mapCryptedKeys.end(); mi = mapCryptedKeys.end())
                                    {
                                    }
                                


                                То есть принудительно зафиксировать однократность в заголовке.
                                • +2
                                  >Ну во-1х для этого предполагаются try { } finally

                                  Я ни в коем случае не хочу вызвать холивар, но мое стойкое убеждение, что исключения предназначены для обработки исключительных, нештатных ситуаций, и строить на них логику обработки вполне штатной ситуации — безусловное зло. Нет, доказывать не буду. Нет, доказывать мне обратное тоже не нужно :)

                                  >Но на сях его нет, это да.

                                  Мммм...?

                                  >тогда, имхо, надо писать так:

                                  Там вообще без разницы, что писать. Можно вообще for (; mi != mapCryptedKeys.end(); ) поставить — все равно выражение-инкремент ни разу не вызовется :)
                                  А если хочется явно указать на однократность, то лучше всего это сделает не «говорящая конструкция», которую вы предложили, а не менее говорящий однострочный комментарий вроде «we'll do it only for the 1st key, if there are any» (хотя бы потому, что ваша конструкция тоже может вызвать вопрос «а на хрена оно тут, да еще и такое странное», как и текущий код).
                                  • –2
                                    finally предназначен не для исключительных ситуаций, а для обязательного исполнения завершающего кода _даже в случае исключения_.
                                    То есть try {

                                    } finally {
                                    cleanup_there;
                                    }

                                    > Мммм...?
                                    В C++ НЕТ структуры finally. Есть только try..catch.

                                    > Там вообще без разницы, что писать.

                                    Я говорю УБРАТЬ в конце цикла break. И оставить ТОЛЬКО mi = end().
                                    Хотя… А в си++ будет работать вариант
                                    if (mi!=mapCryptedKeys.end()) {

                                    } while(0);
                                    ? если да — то он имхо лучше всего.
                                    • +3
                                      В C++ есть аналог finally. Называется деструктор. На этом RAII построено.
                                      • –1
                                        То есть для habrahabr.ru/company/pvs-studio/blog/231489/#comment_7822513 надо заводить еще один объект, в котором определять деструктор?
                                        По-моему мы усиливаем спагетти.
                                        • +2
                                          Я не для конкретного случая, а в целом.

                                          В конкретном случае синтаксис конечно усложнился бы, и оно того не стоит.
                                          Хотя с С++11 лямбдами можно сделать более-менее удобно:

                                          foo() {
                                              scope_exit([&](){
                                                  cleanup(); 
                                              });
                                              some_code();
                                          }
                                          

                                          cleanup будет выполнено при выходе из функции в любом случае, независимо от исключений в основном коде.
                                          Код scope_exit занимает всего 5 втрочек (взято из документации буста):
                                          Скрытый текст
                                          struct scope_exit {
                                              scope_exit(std::function<void (void)> f) : f_(f) {}
                                              ~scope_exit(void) { f_(); }
                                          private:
                                              std::function<void (void)> f_;
                                          };
                        • 0
                          С развитием протокола может появиться необходимость обрабатывать больше. Задел на будущее, как мне кажется. На скорость всё равно не влияет.
                        • +4
                          P.S. Оказалось, про этот цикл уже было обсуждение: github.com/bitcoin/bitcoin/issues/4601 ( github.com/bitcoin/bitcoin/pull/4011 ).
                        • +4
                          Исходные коды взяты с помощью:

                          git clone https://github.com/bitcoin/bitcoin.git

                          Неужели так сложно написать id коммита? Плз, всегда когда берёте код из системы контоля версий, пишите id коммита
                          • –3
                            По этому поводу уже говорили многократно — неважна версия (тем более, что никакого ахтунга не обнаружили).
                            • +5
                              Ну и что? Хочется же открыть этот же коммит и убедиться, что там есть этот код, найти его по номерам строк.
                              Да и потом, PVS же отправляет отчёты разрабы? Так с ids коммитов надо отправлять
                          • НЛО прилетело и опубликовало эту надпись здесь

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

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