Bitcoin Check

There will be nothing epic in this article. We checked the source code of Bitcoin using PVS-Studio. Found only a couple of suspicious places. No wonder. I think that only the lazy did not check these source codes. But once checked, I decided to write a small note. So to speak, "for show".

It all started with the thought that we were comparing PVS-Studio and Clang on open source projects. The task is big, difficult, and I think we will not do it soon. The complexity is represented by the following points:

1. We need to find projects that are built using GCC, but you can build them using Clang. If we check projects that are already Clang-oriented, it will be dishonest. Clang naturally will not find anything in them, since the errors have already been fixed. And PVS-Studio will find it.
2. The PVS-Studio analyzer has to play on a foreign field called “Linux”. There are almost no projects that can be built simultaneously with Clang and Visual Studio. Theoretically, Clang is already well compatible with Visual Studio. In practice, this is not true. It is not possible to collect and verify many projects. PVS-Studio, in turn, does not check projects in Linux well. As a result, you have to choose projects with which both tools can work equally well.

One of the projects selected for comparison was Bitcoin. Both analyzers found almost nothing in it. No wonder. I think this project has already been tested by many tools. Accordingly, this project is most likely not to be compared. Therefore, let at least this short note remain.

Project analysis

Describe what Bitcoin is not necessary. Source codes were taken using:

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

analysis was performed using PVS-Studio version 5.17.

Strange cycle

There was only one place that was interesting in my opinion. This is some kind of function related to cryptography. What does she do, I don’t know. Maybe I found EPIC FAIL. Now it’s fashionable to find epic bugs related to security. However, most likely, this is a minor mistake, or even at all, that was intended.

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 &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 Warning: V612 An unconditional 'return' within a loop. crypter.cpp 169

Note the loop. Some keys have to move in it. But the body of the loop is executed only once. At the end of the loop is the “return false;” statement. The cycle can also be interrupted prematurely using the “break;” statements. At the same time, there is not a single “continue;” operator in the loop body.

Suspicious shift

static int64_t set_vch(const std::vector& vch)
{
  if (vch.empty())
    return 0;
  int64_t result = 0;
  for (size_t i = 0; i != vch.size(); ++i)
      result |= static_cast(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 warning: 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

The function generates a 64-bit value. One shift is made correctly, and the second may not.

Right:

static_cast(vch[i]) << 8*i

At the beginning, the variable expands to int64_t and only then does the shift occur.

Suspiciously:

0x80 << (8 * (vch.size() - 1))

The constant 0x80 is of type 'int'. This means that an overflow may occur during the shift. Since the function generates a 64-bit value, I suspect an error. I wrote in more detail about the shifts in the article " Do not know the ford, do not get into the water - part three ."

Safe option:

0x80ull << (8 * (vch.size() - 1))

Dangerous classes

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 warning: V690 The 'CKey' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. key.h 175

As follows from the comment text, the copy constructor is needed for synchronization. However, you can copy an object not only using the copy constructor, but also using operator =. But this operator is not implemented. Even if operator = is not used anywhere now, it is still potentially dangerous.

Similarly, it is worth paying attention to a few more classes:

• 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

Conclusion

Regular use of static analyzers can save a ton of time and nerve cells. The main thing is that it is convenient. For example, try incremental analysis in PVS-Studio. You just write the code and if that happens, they will stop you. You quickly get used to the good .

This article is in English.

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. http://www.viva64.com/en/b/0268/ .