Не ждите от этой статьи ничего эпического. Мы проверили исходный код проекта Биткойн с помощью PVS-Studio и обнаружили всего пару подозрительных фрагментов. И это неудивительно: я думаю, мало кто из программистов еще не проверил это. Но так как мы тоже сделали свою проверку, то нужно написать об этом небольшой пост, так сказать для проформы.

Все началось с того, что мы решили провести сравнение PVS-Studio и Clang на основе коллекции проектов с открытым исходным кодом. Это большая и сложная задача, поэтому я не ожидаю, что она будет выполнена в ближайшее время. Что усложняет, так это следующие проблемы:

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

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

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

Биткойн в представлении не нуждается. Исходные коды были загружены с:

git-клон 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 Безусловный «возврат» в цикле. crypter.cpp 169

Обратите внимание на цикл: он должен перебирать некоторые ключи. Однако тело цикла выполняется только один раз. Есть «возврат ложного»; оператор в конце цикла, а также его можно прервать с помощью «перерыва»; оператор. При этом нет ни одного «продолжить»; Оператор нужно найти.

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

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 Попробуйте проверить выражение ‘0x80 ‹‹ (8 * (vch.size() — 1))’. Битовый сдвиг 32-битного значения с последующим расширением до 64-битного типа. скрипт.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 В классе CKey реализован конструктор копирования, но отсутствует оператор =. Опасно использовать такой класс. ключ.ч 175

Как следует из комментария, конструктор копирования необходим для синхронизации. Однако копировать объект можно не только конструктором копирования, но и оператором = — а в этом коде он отсутствует. Даже если оператор = пока нигде не используется, код все равно потенциально опасен.

Точно так же есть несколько других классов, которые необходимо изучить:

  • V690 Класс «Semantic_actions» реализует оператор «=», но не имеет конструктора копирования. Опасно использовать такой класс. json_spirit_reader_template.h 196
  • V690 Класс «CFeeRate» реализует конструктор копирования, но не имеет оператора «=». Опасно использовать такой класс. ядро.ч 118
  • V690 Класс «CTransaction» реализует оператор «=», но не имеет конструктора копирования. Опасно использовать такой класс. ядро.ч 212
  • V690 Класс «CTxMemPoolEntry» реализует конструктор копирования, но не имеет оператора «=». Опасно использовать такой класс. txmempool.h 27
  • V690 Класс «Json_grammer» реализует оператор «=», но не имеет конструктора копирования. Опасно использовать такой класс. json_spirit_reader_template.h 370
  • V690 Класс «Генератор» реализует оператор «=», но не имеет конструктора копирования. Опасно использовать такой класс. json_spirit_writer_template.h 98

Вывод

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