Unicorn that could

One of the Microsoft development teams is already using the PVS-Studio analyzer in its work. This is good, but not enough. Therefore, I continue to demonstrate the benefits of static code analysis using Microsoft projects. Three years ago we checked the project Casablanca and could not find anything in it. For this project was awarded the medal "bezbazhny code." Time passed, the project developed and grew. In turn, the PVS-Studio analyzer has significantly advanced in its code analysis capabilities. And finally, I can write an article about errors that the analyzer detects in the Casablanca project (C ++ REST SDK). There are few errors, but the fact that now there are enough of them for writing an article speaks about the effectiveness of PVS-Studio.

Casablanca

As I noted in the annotation, we have already checked the draft Casablanca. You can read about it in the article " A small note about the project Casablanca ".



Casablanca (C ++ REST SDK) is a small project written in modern C ++. Speaking of the modern C ++ language, I mean that the semantics of move (move semantics), lambda-functions, auto and so on are actively used in the code. New features in C ++ allow you to write shorter and more reliable code. This is confirmed by the fact that finding errors in this project is not an easy task. Although usually we do it extremely easily.



For those interested in what other Microsoft projects we checked, I provide a list of articles devoted to these checks: Xamarin.Forms , CNTK , Microsoft Edge , CoreCLR , Windows 8 Driver Samples , Visual C ++ 2012/2013 , CoreFX , Roslyn , Microsoft Code Contracts , and An article about WPF Samples verification is coming soon.

')

So, the Casablanca project is a sample of good, high-quality code. Let's see what can still be found in it with the help of the PVS-Studio static code analyzer.

What did you find bad

Fragment N1: typo



There is a NumericHandValues structure containing two members: low and high . Here is the declaration of this structure:

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

And now let's see how this structure is initialized in one place:

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

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



As you can see, accidentally two times initialized the member low , but at the same time they forgot to initialize high . Thoughtful commentary is hard to write here. Just no one is immune from typos.



Fragment N2: Memory Free Error

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

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



By default, a smart pointer such as shared_ptr to invoke an object will invoke the delete operator without square brackets [] . In this case, it is wrong.



The correct code should be as follows:

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

Fragment N3: lost pointer



The static member s_server_api is a smart pointer and is defined as follows:

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

Suspicion is caused by 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 warning: V530. cpprestsdk140 http_server_api.cpp 64



Note the "s_server_api.release ();". After calling the release function, the smart pointer no longer owns the object. The pointer to the object is “lost” and the object will exist until the end of the program’s life.



Most likely, we again encountered a typo. I think they wanted to call the reset function, not the release at all.



Fragment N4: wrong enum



There are two BJHandState and BJHandResult enums in the project , declared as follows:

 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 now let's look at the code fragment from the PayUp function:

 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 warning: V556 The values ​​of different enum types are compared. Types: BJHandState, BJHandResult. BlackJack_Server140 table.cpp 336



The state variable is of type BJHandState . This means that the programmer is confused in the enumerations. Apparently, the code should look like this:

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

The funny thing is that this error actually does not affect the work of the program. Thanks to a happy coincidence, the HR_BlackJack and HR_PlayerBlackJack constants currently have the same value, equal to 1. The fact is that both of these constants in the enumeration are in the same position in the list. However, in the development process of the program this may change, and then a strange, incomprehensible mistake will arise.



Fragment N5: 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 warning: V612 An unconditional 'break' within a loop. BlackJack_Client140 messagetypes.h 213



The break statement in this code looks extremely suspicious. A loop can perform a maximum of one iteration. It's hard for me to say how this code should behave, but, most likely, something is wrong with it now.

Other stuff

In addition to the code snippets discussed above and claiming to be errors, the analyzer also found some inaccurate places. For example, use postincrement for iterators.

 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 warning: V803 Decreased performance. In case of a tbl is an iterator Replace iterator ++ with ++ iterator. BlackJack_Client140 messagetypes.h 356



This, of course, is not a mistake. However, pre-increment is considered a good style: ++ tbl . For those who doubt that this makes sense, I refer to the following two articles:

1. Is it practical to use the prefix increment operator ++ it for iterators instead of postfix increment it ++. http://www.viva64.com/ru/b/0093/
2. Pre vs. post increment operator - benchmark. http://silviuardelean.ro/2011/04/20/pre-vs-post-increment-operator/

In the library code there are 10 more places where iterator postincrement is used in cycles, but I do not see the point in citing them in the article.



Consider another example when the analyzer points to an inaccurate 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 warning: V690 The '=' operator will still be generated by the compiler. It is dangerous to use such a class. cpprestsdk140.uwp.staticlib fileio_winrt.cpp 825



As you can see, the programmer has banned the use of the copy operator. However, the object can still be copied using the copy constructor created by the default compiler.

Conclusion

Finally, the PVS-Studio analyzer was able to find fault with something. There were few errors, but still they are. And this means that if you use static analysis not once, as I have done now, but regularly, then you can prevent a lot of errors at an early stage. It is better to correct the errors immediately after writing the code, rather than in the process of testing, debugging, and even more so after one of the users reports a defect.

Additional links

1. The title of the article is a reference to the fairy tale "The Engine that Smog ".
2. Link where you can download the PVS-Studio analyzer and try to check one of your projects in C, C ++ or C #: http://www.viva64.com/en/pvs-studio-download/

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. The Little Unicorn That Could .