To get a trial key
fill out the form below
Team License (standard version)
Enterprise License (extended version)
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
To get the licence for your open-source project, please fill out this form
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

** This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
Message submitted.

Your message has been sent. We will email you at


If you haven't received our response, please do the following:
check your Spam/Junk folder and click the "Not Spam" button for our message.
This way, you won't miss messages from our team in the future.

>
>
>
Checking Bitcoin

Checking Bitcoin

Jul 29 2014
Author:

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.

0268_Bitcoin/image1.png

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.

Popular related articles
The way static analyzers fight against false positives, and why they do it

Date: Mar 20 2017

Author: Andrey Karpov

In my previous article I wrote that I don't like the approach of evaluating the efficiency of static analyzers with the help of synthetic tests. In that article, I give the example of a code fragment…
The Evil within the Comparison Functions

Date: May 19 2017

Author: Andrey Karpov

Perhaps, readers remember my article titled "Last line effect". It describes a pattern I've once noticed: in most cases programmers make an error in the last line of similar text blocks. Now I want t…
The Last Line Effect

Date: May 31 2014

Author: Andrey Karpov

I have studied many errors caused by the use of the Copy-Paste method, and can assure you that programmers most often tend to make mistakes in the last fragment of a homogeneous code block. I have ne…
The Ultimate Question of Programming, Refactoring, and Everything

Date: Apr 14 2016

Author: Andrey Karpov

Yes, you've guessed correctly - the answer is "42". In this article you will find 42 recommendations about coding in C++ that can help a programmer avoid a lot of errors, save time and effort. The au…
Appreciate Static Code Analysis!

Date: Oct 16 2017

Author: Andrey Karpov

I am really astonished by the capabilities of static code analysis even though I am one of the developers of PVS-Studio analyzer myself. The tool surprised me the other day as it turned out to be sma…
How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers

Date: Oct 22 2018

Author: Andrey Karpov

Just like other static analyzers, PVS-Studio often produces false positives. What you are about to read is a short story where I'll tell you how PVS-Studio proved, just one more time, to be more atte…
Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities

Date: Nov 21 2018

Author: Andrey Karpov

A brief description of technologies used in the PVS-Studio tool, which let us effectively detect a large number of error patterns and potential vulnerabilities. The article describes the implementati…
Static analysis as part of the development process in Unreal Engine

Date: Jun 27 2017

Author: Andrey Karpov

Unreal Engine continues to develop as new code is added and previously written code is changed. What is the inevitable consequence of ongoing development in a project? The emergence of new bugs in th…
Free PVS-Studio for those who develops open source projects

Date: Dec 22 2018

Author: Andrey Karpov

On the New 2019 year's eve, a PVS-Studio team decided to make a nice gift for all contributors of open-source projects hosted on GitHub, GitLab or Bitbucket. They are given free usage of PVS-Studio s…
PVS-Studio ROI

Date: Jan 30 2019

Author: Andrey Karpov

Occasionally, we're asked a question, what monetary value the company will receive from using PVS-Studio. We decided to draw up a response in the form of an article and provide tables, which will sho…

Comments (0)

Next comments

This site is protected by reCAPTCHA and the Google Privacy Policy and Terms of Service apply.
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site.
Learn More →
Accept