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

Request our prices
New License
License Renewal
--Select currency--
USD
EUR
GBP
RUB
* By clicking this button you agree to our Privacy Policy statement

Free PVS-Studio license for Microsoft MVP specialists
* By clicking this button you agree to our Privacy Policy statement

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

I am interested to try it on the platforms:
* By clicking this button you agree to our Privacy Policy statement

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.

>
>
>
The Little Unicorn That Could

The Little Unicorn That Could

Jun 22 2016
Author:

One of the Microsoft development teams already uses PVS-Studio analyzer in their work. It's great, but it's not enough. That's why I keep demonstrating how static code analysis could benefit developers, using Microsoft projects as examples. We scanned Casablanca project three years ago and found nothing. As a tribute to its high quality, the project was awarded with a "bugless code" medal. As time went by, Casablanca developed and grew. PVS-Studio's capabilities, too, have significantly improved, and now I've finally got the opportunity to write an article about errors found by the analyzer in Casablanca project (C++ REST SDK). These errors are few, but the fact that their number is still big enough for me to make this article, does speak a lot in favor of PVS-Studio's effectiveness.

0406_The_Little_Unicorn_That_Could/image1.png

Casablanca

As I already said in the introduction, we have analyzed Casablanca project before; see the article "A Small Post about Casablanca project" for the analysis results.

Casablanca (C++ REST SDK) is a small project written in Contemporary C++, by which I mean that the project authors heavily use move semantics, lambdas, auto, and so forth. The new features of the C++ language allow programmers to write shorter and safer code. This assertion is supported by the fact that collecting a decent number of bugs from this one is a difficult task, unlike other projects where we easily catch lots of them.

For the analysis results for other Microsoft projects that we have scanned, see the following list of articles: Xamarin.Forms, CNTK, Microsoft Edge, CoreCLR, Windows 8 Driver Samples, Visual C++ 2012 / 2013 library, CoreFX, Roslyn, Microsoft Code Contracts, WPF Samples (coming soon).

So, as we have found, Casablanca is a model of fine, high-quality code. Let's see what issues PVS-Studio analyzer has managed to catch there.

Errors found

Fragment No. 1: typo

There is structure NumericHandValues with two members: low and high. This is how it is declared:

struct NumericHandValues
{
  int low;
  int high;
  int Best() { return (high < 22) ? high : low; }
};

And this is how it is initialized in one of the fragments:

NumericHandValues GetNumericValues()
{
  NumericHandValues res;
  res.low = 0;
  res.low = 0;
  
  ....
}

PVS-Studio diagnostic message: V519 The 'res.low' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 130, 131. BlackJack_Client140 messagetypes.h 131

In this code, the programmer made a mistake and initialized the low member twice, leaving high uninitialized. There's hardly any profound comment to make on this situation; it's just that nobody is safe from typos.

Fragment No. 2: memory release error

void DealerTable::FillShoe(size_t decks)
{
  std::shared_ptr<int> ss(new int[decks * 52]);
  ....
}

PVS-Studio diagnostic message: V554 Incorrect use of shared_ptr. The memory allocated with 'new []' will be cleaned using 'delete'. BlackJack_Server140 table.cpp 471

When destroying an object, a smart pointer of type shared_ptr calls the delete operator by default without brackets []. In this case, however, this behavior leads to an error.

To ensure correct object destruction, the code must be rewritten in the following way:

std::shared_ptr<int> ss(new int[decks * 52],
                        std::default_delete<int[]>());

Fragment No. 3: lost pointer

Static member s_server_api is a smart pointer and is declared in the following way:

std::unique_ptr<http_server>
  http_server_api::s_server_api((http_server*)nullptr);

What doesn't look right is the following function code:

void http_server_api::unregister_server_api()
{
  pplx::extensibility::scoped_critical_section_t lock(s_lock);

  if (http_server_api::has_listener())
  {
    throw http_exception(_XPLATSTR("Server API ..... attached"));
  }

  s_server_api.release();
}

PVS-Studio diagnostic message: V530 The return value of function 'release' is required to be utilized. cpprestsdk140 http_server_api.cpp 64

Note the line "s_server_api.release();". After calling the release function, a smart pointer does not own the object anymore. Therefore, in our example, the pointer to the object is "lost", and the latter will exist until the program terminates.

Again, it looks like we're dealing with a typo in this example: what the programmer must have intended to call is function reset, not release.

Fragment No. 4: wrong enum

There are two enumerations, BJHandState and BJHandResult, which are declared in the following way:

enum BJHandState {
  HR_Empty, HR_BlackJack, HR_Active, HR_Held, HR_Busted
};
enum BJHandResult {
  HR_None, HR_PlayerBlackJack, HR_PlayerWin,
  HR_ComputerWin, HR_Push
};

And this is a code fragment from function PayUp:

void DealerTable::PayUp(size_t idx)
{
  ....
  if ( player.Hand.insurance > 0 &&
       Players[0].Hand.state == HR_PlayerBlackJack )
  {
    player.Balance += player.Hand.insurance*3;
  }
  ....
}

PVS-Studio diagnostic message: V556 The values of different enum types are compared. Types: BJHandState, BJHandResult. BlackJack_Server140 table.cpp 336

The state variable is of type BJHandState, which means that the programmer mixed up the enumerations. The code was more likely meant to look like this:

if ( player.Hand.insurance > 0 &&
     Players[0].Hand.state == HR_BlackJack )

The funny thing is that this error doesn't affect the program execution in any way for now. Fortunately, the constants HR_BlackJack and HR_PlayerBlackJack currently refer to the same value, 1. The reason is that both constants occupy the same position in the corresponding enumerations. However, it may change as the project develops, resulting in a strange, obscure error.

Fragment No. 5: strange break

web::json::value AsJSON() const 
{
  ....
  int idx = 0;
  for (auto iter = cards.begin(); iter != cards.end();)
  {
    jCards[idx++] = iter->AsJSON();
    break;
  }
  ....
}

PVS-Studio diagnostic message: V612 An unconditional 'break' within a loop. BlackJack_Client140 messagetypes.h 213

The break statement looks very suspicious, as the loop can iterate only once at most. I can't tell for sure what exactly this code is meant to do, but it doesn't look right in its present form.

Miscellaneous

Besides the issues that we have already discussed and could call errors, the analyzer found a few fragments of untidy code - for example the ones where iterators are post-incremented.

inline web::json::value
TablesAsJSON(...., std::shared_ptr<BJTable>> &tables)
{
  web::json::value result = web::json::value::array();

  size_t idx = 0;
  for (auto tbl = tables.begin(); tbl != tables.end(); tbl++)
  {
    result[idx++] = tbl->second->AsJSON();
  }
  return result;
}

PVS-Studio diagnostic message: V803 Decreased performance. In case 'tbl' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. BlackJack_Client140 messagetypes.h 356

It's not an error, of course, but it is considered a good style to use a pre-increment instead: ++tbl. If you feel unsure about this, please see the following articles:

There are 10 more examples of post-incremented iterators found in the library's code, but I don't think we need to discuss them here.

Another example of untidy code:

struct _acquire_protector
{
  _acquire_protector(....);
  ~_acquire_protector();
  size_t   m_size;
private:
  _acquire_protector& operator=(const _acquire_protector&);
  uint8_t* m_ptr;
  concurrency::streams::streambuf<uint8_t>& m_buffer;
};

PVS-Studio diagnostic message: V690 The '=' operator is declared as private in the '_acquire_protector' class, but the default copy constructor will still be generated by compiler. It is dangerous to use such a class. cpprestsdk140.uwp.staticlib fileio_winrt.cpp 825

As you can see, the programmer prohibited the use of the copy operator. However, the object can still be copied using the copy constructor, which the compiler creates by default.

Conclusion

PVS-Studio analyzer has at last detected something to find fault with. The errors are few, but they are still errors. It means that using static analysis regularly, not occasionally, like I did for this article, could help prevent lots of bugs at the earliest stage. Fixing errors right after writing the code is better than during the testing or debugging phase or, worst of all, when these errors are reported by end users.

References

Popular related articles
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…
Characteristics of PVS-Studio Analyzer by the Example of EFL Core Libraries, 10-15% of False Positives

Date: Jul 31 2017

Author: Andrey Karpov

After I wrote quite a big article about the analysis of the Tizen OS code, I received a large number of questions concerning the percentage of false positives and the density of errors (how many erro…
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…
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…
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…
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…
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…
PVS-Studio for Java

Date: Jan 17 2019

Author: Andrey Karpov

In the seventh version of the PVS-Studio static analyzer, we added support of the Java language. It's time for a brief story of how we've started making support of the Java language, how far we've co…
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…
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…

Comments (0)

Next comments
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