Перевірка Bitcoin

    Bitcoin, PVS-Studio
Нічого епічного в цій статті не буде. Ми перевірили за допомогою PVS-Studio вихідний код Bitcoin. Знайшли всього пару підозрілих місць. Це не дивно. Думаю, ці вихідні коди не перевіряв тільки ледачий. Але раз перевірили, то вирішив написати маленьку замітку. Так би мовити, «для галочки».
 
Почалося все з того, що ми задумалися провести порівняння PVS-Studio і Clang на відкритих проектах. Завдання велика, складна і думаю зробимо ми її не скоро. Складність представляють наступні моменти:
 
     
  1. Треба знайти проекти, які збираються за допомогою GCC, але при цьому їх можна зібрати за допомогою Clang. Якщо ми будемо перевіряти проекти, які вже орієнтовані на Clang, це буде нечесно. Clang в них природно нічого не знайде, оскільки помилки вже виправлені. А PVS-Studio знайде.
  2.  
  3. аналізатор PVS-Studio доводиться грати на чужому полі під назвою «Linux». Майже немає проектів, які можна одночасно зібрати за допомогою Clang і Visual Studio. Теоретично говориться, що Clang вже добре сумісний з Visual Studio. На практиці це неправда. Не вдається зібрати і перевірити багато проекти. PVS-Studio в свою чергу погано перевіряє проекти в Linux. У результаті доводиться вибирати проекти, з якими однаково вдало можуть працювати обидва інструменту.
  4.  
Одним з проектів, обраних для порівняння став 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 '0 x80 << (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 . Будь ласка, ознайомтеся зі списком.
 
    
Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.