Bitcoin check

Nothing epic in this article will be. We have checked Bitcoin source code with PVS-Studio. Found only a couple of suspicious places. No wonder. I think these source codes are not checked only lazy. But once checked, I decided to write a small note. So to say, "for show".

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

1. We need to find projects that are built with the help of GCC, but you can build them with Clang. If we check projects that are already focused on Clang, it will not be fair. Clang will naturally find nothing in them, since the errors have already been corrected. And PVS-Studio will find it.
2. The PVS-Studio analyzer has to play in a foreign field called “Linux”. There are almost no projects that can be simultaneously built using Clang and Visual Studio. Theoretically, it says that Clang is already well compatible with Visual Studio. In practice, this is not true. Unable to collect and verify many projects. PVS-Studio, in turn, badly checks projects in Linux. As a result, one has 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 with many tools. Accordingly, this project, most likely, will not be included in the comparison. Therefore, let it remain at least this brief note.

Project analysis

Describe what Bitcoin is not needed. Source codes are 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 interesting in my opinion. This is a function related to cryptography. What she does I do not know. Perhaps I found an EPIC FAIL. Now it is fashionable to find epic errors related to security. However, most likely, this is a minor mistake or even completely, and it was conceived.

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

Pay attention to the cycle. Some keys must be moved in it. But the loop body is executed only once. At the end of the loop is the operator "return false;". The cycle can also be interrupted prematurely with the help of “break;” operators. At the same time, there is no “continue;” operator in the loop body.

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 warning: V629 Consider inspecting the '0x80 << (8 * (vch.size () - 1))' expression. Bit shifting of the 32-bit type. script.h 169

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

Right:

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

At the beginning, the variable expands to int64_t and only then a shift occurs.

Suspiciously:

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

The constant 0x80 is of type 'int'. This means that an overflow may occur during shear. Since the function generates a 64-bit value, I suspect an error. I wrote more about the shifts in the article " Without knowing the ford, do not climb 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 text of the comment, the copy constructor is needed for synchronization. However, you can copy an object not only using the copy constructor, but also using operator =. And this operator is not implemented. Even if operator = now is not used anywhere, it is still potentially dangerous.

Similarly, you should pay attention to several other classes:

• V690 The 'Semantic_actions' class implements the '=' operator, but there is no 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 there is no copy constructor. It is dangerous to use such a class. core.h 212
• V690 The 'CTxMemPoolEntry' class implements a copy constructor It is dangerous to use such a class. txmempool.h 27
• V690 The 'Json_grammer' class implements the '=' operator, but there is no copy constructor. It is dangerous to use such a class. json_spirit_reader_template.h 370
• V690 The 'Generator' class implements the '=' operator, but there is no 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 lot of time and nerve cells. The main thing is that it is convenient. For example, try incremental analysis in PVS-Studio. Just write the code and if anything, you will be stopped. 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/ .