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:
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.
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.
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.
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*iThe 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))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:
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.