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.
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.
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.
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.
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.