Checking Bitcoin

Jul 29 2014

Author: Andrey Karpov

Project analysis
Conclusion

Don't expect anything epic from this article. We have checked the Bitcoin project's source code with PVS-Studio and found just a couple of suspicious fragments. Which is no wonder: I guess there are few programmers who haven't checked it already. But since we have done our check too, we need to write a small post about it, pro forma, so to say.

It all started when we decided to carry out a comparison of PVS-Studio and Clang based on a collection of open source projects. This is a large and complex task, so I don't expect it to be accomplished soon. What makes it difficult is the following issues:

We need to collect projects usually built by GCC but compilable by Clang too. If we start checking Clang-oriented projects, it won't be fair as Clang will naturally find no bugs there because they have been already fixed with its help. And PVS-Studio will.
The PVS-Studio analyzer has to play on a foreign field known as "Linux". There are almost no projects that can be built both with Clang and Visual Studio. Clang is theoretically claimed to have a good compatibility with Visual Studio, but it doesn't prove true in practice; many projects cannot be built and checked. PVS-Studio, in its turn, is bad at checking projects under Linux. So we have to search for projects both the tools can handle similarly well.

Bitcoin is one of these projects that we chose for our comparison. Both the analyzers have found almost zero bugs in it. And that's no wonder – I guess this project has already been checked by many tools, that's why we will most likely exclude it from the comparison. So, let's have at least this short note remaining of this check.

Project analysis

Bitcoin doesn't need to be introduced. The source codes were downloaded from:

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

Analysis was done by PVS-Studio 5.17.

Strange loop

The analyzer detected only one suspicious fragment that I found worthy. This is some function related to cryptography. I don't know what it does exactly, and maybe I have found a real EPIC FAIL. You see, it's a popular trend nowadays to find epic errors related to security. But this one is most likely a tiny bug or even a correct piece of code written purposely.

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

Notice the loop: it must iterate through some keys. However, the loop body executes only once. There is the "return false;" operator at the end of the loop, and it can also be terminated by the "break;" operator. At the same time, there is not a single "continue;" operator to be found.

Suspicious shift

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's diagnostic message: 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 forms a 64-bit value. One shift is correct, the other is probably not.

The correct line:

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

The variable is first extended to int64_t and only then is shifted.

The suspicious line:

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

The 0x80 constant is of the 'int' type. It means that shifting it may result in an overflow. Since the function generates a 64-bit value, I suspect an error here. To learn more about shifts, see the article "Wade not in unknown waters - part three ".

Fixed code:

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

As the comment suggests, the copy constructor is needed for synch. However, an object can be copied not only by a copy constructor, but the operator = too – and it is missing in this code. Even if the operator = is not used anywhere for now, the code is still potentially dangerous.

Similarly, there are a few other classes that need to be examined:

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

Regularly using static analyzers may help you save a huge amount of time and nerve cells. The main thing about it is that it should be done conveniently. For instance, try the incremental analysis mode in PVS-Studio: you simply keep coding and only if something wrong happens, the tool will interfere. People do tend to get used to good things quickly.

#Cpp