Tyap-blooper, checked the library Visual C ++ 2013 (update 3)

I was offered to check the libraries included in Visual Studio 2013. I found nothing particularly remarkable. Only a few minor mistakes and shortcomings. You will not make an intriguing article out of this, but I will still describe the shortcomings noted. I hope this will make the libraries a bit better, and will encourage the authors to conduct a more thorough check. I do not have project files for building libraries. Therefore, I checked the files somehow, and a lot could have been missed.

This is the second article about checking libraries in Visual C ++. The results of the previous test can be found in the article: Errors in the Visual C ++ 2012 libraries have been detected .

I can not check the full library. I acted very clumsy. Included in the new project all the files contained in the folders "crt \ src" and "atlmfc \ src". Plus, I made a new test.cpp file, which included all the header files related to the standard library (vector, map, set, and so on).

After that, having conjured a little with the project settings, I achieved that about 80% of files are compiled. I think that's enough. Even if the file is not compiled, it will most likely be checked by PVS-Studio, even if not completely.
')
I think if this article will interest library developers, they will be able to conduct a more thorough analysis. Even if the assembly is carried out in an exotic way, now it will not be a problem. It will be possible to use the compiler launch tracking mechanism.

For verification, I used PVS-Studio version 5.19. The source codes of C / C ++ libraries included in Visual Studio 2013 (update 3) were checked.

Result of checking

We met some shortcomings that we found in the previous version of Visual Studio 2012. For example, the proj () function still looks strange, the ~ single_link_registry () destructor looks dangerous. But it is not interesting to repeat. Let's try to find something new.

Incorrect index check

void _Initialize_order_node(...., size_t _Index, ....) { if (_Index < 0) { throw std::invalid_argument("_Index"); } .... } 

PVS-Studio warning : V547 Expression '_Index <0' is always false. Unsigned type value is never <0. agents.h 8442

The argument is _Index and is unsigned. Therefore, verification does not make sense. An exception will never be generated. I think this is not a mistake, but just an extra code.

Format error

 int _tfpecode; /* float point exception code */ void __cdecl _print_tiddata1 ( _ptiddata ptd ) { .... printf("\t_gmtimebuf = %p\n", ptd->_gmtimebuf); printf("\t_initaddr = %p\n", ptd->_initaddr); printf("\t_initarg = %p\n", ptd->_initarg); printf("\t_pxcptacttab = %p\n", ptd->_pxcptacttab); printf("\t_tpxcptinfoptrs = %p\n", ptd->_tpxcptinfoptrs); printf("\t_tfpecode = %p\n\n", ptd->_tfpecode); .... } 

PVS-Studio warning : V576 Incorrect format. Consider the second printf function. The pointer is expected as an argument. tidprint.c 133

Here we are dealing with the effect of the last line . At the end of the block of the same type of lines an error was made Everywhere should print the value of the pointer. But at the very end, the '_tfpecode' variable is not a pointer, but simply an integer value. It should be written:

 printf("\t_tfpecode = %i\n\n", ptd->_tfpecode); 

Weird repetitive calculations

 unsigned int SchedulerProxy::AdjustAllocationIncrease(....) const { .... unsigned int remainingConcurrency = m_maxConcurrency - m_currentConcurrency; remainingConcurrency = m_maxConcurrency - m_currentConcurrency; .... } 

PVS-Studio warning : V519 The 'remainingConcurrency' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1136, 1137. schedulerproxy.cpp 1137

A variable is assigned the result of the same expression twice. This code is redundant and, most likely, is the result of unsuccessful refactoring.

Suspicion of typo

 double HillClimbing::CalculateThroughputSlope(....) { .... MeasuredHistory * lastHistory = GetHistory(fromSetting); MeasuredHistory * currentHistory = GetHistory(toSetting); .... double varianceOfcurrentHistory = currentHistory->VarianceMean(); double varianceOflastHistory = currentHistory->VarianceMean(); .... } 

PVS-Studio Warning: V656 Variables 'varianceOfcurrentHistory', 'varianceOflastHistory' are initialized. It's probably not an error or un-optimized code. Consider inspecting the 'currentHistory-> VarianceMean ()' expression. Check lines: 412, 413. hillclimbing.cpp 413

Suspiciously, the same value is assigned to the varianceOfcurrentHistory variable and the varianceOflastHistory variable. It was logical to initialize varianceOflastHistory like this:

 double varianceOflastHistory = varianceOfcurrentHistory; 

Moreover, there is also a 'lastHistory' pointer. I will suggest that the code contains a typo. Perhaps the code should be like this:

 double varianceOfcurrentHistory = currentHistory->VarianceMean(); double varianceOflastHistory = lastHistory->VarianceMean(); 

And here is exactly a typo

 BOOL CPropertySheet::SetActivePage(CPropertyPage* pPage) { ASSERT_VALID(this); ENSURE_VALID(pPage); ASSERT_KINDOF(CPropertyPage, pPage); int nPage = GetPageIndex(pPage); ASSERT(pPage >= 0); return SetActivePage(nPage); } 

PVS-Studio warning : V503 This is a nonsensical comparison: pointer> = 0. dlgprop.cpp 1206

It is strange to check that the value of the pointer is greater than or equal to zero. This is a typo and actually wanted to check the 'nPage' variable:

 int nPage = GetPageIndex(pPage); ASSERT(nPage >= 0); 

Of course, this is just an ASSERT, and the error will not lead to any negative consequences. But still this is a mistake.

Same action regardless of condition

 void CMFCVisualManager::OnDrawTasksGroupCaption(....) { .... if (pGroup->m_bIsSpecial) { if (!pGroup->m_bIsCollapsed) { CMenuImages::Draw(pDC, CMenuImages::IdArrowUp, rectButton.TopLeft()); } else { CMenuImages::Draw(pDC, CMenuImages::IdArrowDown, rectButton.TopLeft()); } } else { if (!pGroup->m_bIsCollapsed) { CMenuImages::Draw(pDC, CMenuImages::IdArrowUp, rectButton.TopLeft()); } else { CMenuImages::Draw(pDC, CMenuImages::IdArrowDown, rectButton.TopLeft()); } } .... } 

PVS-Studio warning : V523 The 'then' statement is equivalent to the 'else' statement. afxvisualmanager.cpp 2118

Regardless of the condition (pGroup-> m_bIsSpecial), the same actions are performed. This is strange.

Incorrect port number check

 typedef WORD ATL_URL_PORT; ATL_URL_PORT m_nPortNumber; inline BOOL Parse(_In_z_ LPCTSTR lpszUrl) { .... m_nPortNumber = (ATL_URL_PORT) _ttoi(tmpBuf); if (m_nPortNumber < 0) goto error; .... } 

PVS-Studio warning : V547 Expression 'm_nPortNumber <0' is always false. Unsigned type value is never <0. atlutil.h 2773

The variable 'm_nPortNumber' is unsigned WORD.

No virtual destructor

 class CDataSourceControl { .... ~CDataSourceControl(); .... virtual IUnknown* GetCursor(); virtual void BindProp(....); virtual void BindProp(....); .... } CDataSourceControl* m_pDataSourceControl; COleControlSite::~COleControlSite() { .... delete m_pDataSourceControl; .... } 

Warning PVS_Studio: V599, it contains the virtual functions. occsite.cpp 77

The CDataSourceControl class contains virtual methods, but the destructor is not virtual. Is it dangerous. If the class X is inherited from the CDataSourceControl class, it will not be possible to destroy objects of type X using a pointer to the base class.

Unfinished code

 BOOL CMFCWindowsManagerDialog::OnHelpInfo(HELPINFO* pHelpInfo) { pHelpInfo->iCtrlId; CWnd* pParentFrame = AfxGetMainWnd(); pParentFrame->SendMessage(AFX_WM_WINDOW_HELP, 0, (LPARAM) this); return FALSE; } 

Warning PVS_Studio: V607 Ownerless expression 'pHelpInfo-> iCtrlId'. afxwindowsmanagerdialog.cpp 472

What is "pHelpInfo-> iCtrlId;"? What would it mean?

Suspicious double initialization

 CMFCStatusBar::CMFCStatusBar() { m_hFont = NULL; // setup correct margins m_cxRightBorder = m_cxDefaultGap; //<<-- m_cxSizeBox = 0; m_cxLeftBorder = 4; m_cyTopBorder = 2; m_cyBottomBorder = 0; m_cxRightBorder = 0; //<<-- .... } 

PVS-Studio warning : V519 The m_cxRightBorder variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 74, 80. afxstatusbar.cpp 80

At the beginning, the value of another variable is written into the 'm_cxRightBorder' variable. And then, that value is suddenly overwritten by zero.

Suspicious status check

 #define S_OK ((HRESULT)0L) #define E_NOINTERFACE _HRESULT_TYPEDEF_(0x80004002L) HRESULT GetDocument(IHTMLDocument2** ppDoc) const { const T* pT = static_cast<const T*>(this); return pT->GetDHtmlDocument(ppDoc) ? S_OK : E_NOINTERFACE; } HRESULT GetEvent(IHTMLEventObj **ppEventObj) const { .... if (GetDocument(&sphtmlDoc)) .... } 

PVS-Studio warning : V545 Such conditional expression of 'if' operator is incorrect for the HRESULT type value 'GetDocument (& sphtmlDoc)'. The SUCCEEDED or FAILED macro should be used instead. afxhtml.h 593

The design of the code may not correspond to its logic of operation. It seems that if the condition 'GetDocument (...)' is true, then it was possible to “get” the document. But, in fact, it's not like that. The GetDocument () function returns a value of type HRESULT. With this type, the opposite is true. For example, the status S_OK is encoded as 0, and the status E_NOINTERFACE as 0x80004002L. To check the values ​​of type HRESULT, you should use special macros: SUCCEEDED, FAILED.

I do not know if there is an error here or not, but this code is confusing and should be checked.

Wrong argument for macro MAKE_HRESULT

 #define MAKE_HRESULT(sev,fac,code) \ ((HRESULT) \ (((unsigned long)(sev)<<31) | \ ((unsigned long)(fac)<<16) | \ ((unsigned long)(code))) ) ATLINLINE ATLAPI AtlSetErrorInfo(....) { .... hRes = MAKE_HRESULT(3, FACILITY_ITF, nID); .... } 

PVS-Studio warning : V673 The '(unsigned long) (3) << 31' expression evaluates the value of the '32' bits. atlcom.h 6650

Everything will work correctly, but there is an error. I will explain.

The function should generate error information in a variable of type HRESULT. To do this, use the MAKE_HRESULT macro. But it is used incorrectly. The programmer considered that the first parameter 'severity' could lie in the aisles from 0 to 3. Apparently, he confused this with the method of generating error codes used when working with the GetLastError () / SetLastError () functions.

The macro MAKE_HRESULT can only accept 0 (success) or 1 (failure) as the first argument. This issue was discussed in more detail on the CodeGuru website: Warning! MAKE_HRESULT macro doesn't work.

Since the number 3 is used as the first actual argument, an overflow occurs. The number 3 "will turn" into 1. Due to this randomness, the error will not affect the program operation.

ASSERTs whose conditions are always true

There are quite a few situations where the condition for ASSERT looks like this: (X> = 0). In this case, the variable X is declared as an unsigned integer type. It turns out that the condition is always true.

In some cases, the presence of such an ASSERT is reasonable. Suddenly, in the process of refactoring, the variable will become sign, and the algorithm is not ready to work with negative numbers. However, the existence of some of them is probably meaningless. They need to be removed from the code or replaced with another useful check. That's why I decided to mention them in the article.

Consider an example:

 DWORD m_oversubscribeCount; void ExternalContextBase::Oversubscribe(....) { if (beginOversubscription) { ASSERT(m_oversubscribeCount >= 0); ++m_oversubscribeCount; } .... } 

PVS-Studio warning : V547 Expression 'm_oversubscribeCount> = 0' is always true. Unsigned type value is always> = 0. externalcontextbase.cpp 204

The rest of the warnings are just a list:

• V547 Expression 'm_oversubscribeCount> = 0' is always true. Unsigned type value is always> = 0. internalcontextbase.cpp 506
• V547 Expression 'pGlobalNode-> m_idleCores> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 3764
• V547 Expression 'pGlobalNode-> m_availableCores> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 3769
• V547 Expression 'pReceivingProxyData-> m_allocation> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4100
• V547 Expression 'pReceivingProxyData-> m_allocation> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4360
• V547 Expression 'exclusiveCoresAvailable> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4657
• V547 Expression 'coresNeeded> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4657
• V547 Expression 'previousGlobal> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4781
• V547 Expression 'currentGlobal> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4782
• V547 Expression 'm_minConcurrency> = 0' is always true. Unsigned type value is always> = 0. Schedulerproxy.cpp 63
• V547 Expression 'm_minimumHardwareThreads> = 0' is always true. Unsigned type value is always> = 0. Schedulerproxy.cpp 125
• V547 Expression 'm_oversubscribeCount> = 0' is always true. Unsigned type value is always> = 0. umsthreadinternalcontext.cpp 308
• V547 Expression 'j> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 1922
• V547 Expression 'pMaxNode-> m_availableCores> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 2542
• V547 Expression 'previousLocal> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4793
• V547 Expression 'currentLocal> = 0' is always true. Unsigned type value is always> = 0. resourcemanager.cpp 4794
• V547 Expression is always true. Unsigned type value is always> = 0. schedulerpolicybase.cpp 285
• V547 Expression 'value> = 0' is always true. Unsigned type value is always> = 0. Schedulerpolicybase.cpp 345

Unnecessary type conversions

There were several explicit type conversions that are not only redundant, but can also spoil the values.

Consider the first example:

 size_t __cdecl strnlen(const char *str, size_t maxsize); size_t __cdecl _mbstrnlen_l(const char *s, size_t sizeInBytes, _locale_t plocinfo) { .... if ( _loc_update.GetLocaleT()->locinfo->mb_cur_max == 1 ) /* handle single byte character sets */ return (int)strnlen(s, sizeInBytes); .... } 

PVS-Studio warning : V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'strnlen (s, sizeInBytes)'. _mbslen_s.c 67

The strnlen () function returns a value of type 'size_t'. Then it is suddenly explicitly cast to the 'int' type. After that, the value is implicitly re-expanded to the size_t type.

This code contains a potential 64-bit error. If suddenly someone wants to calculate the number of characters in a very long string using the _mbstrnlen_l () function in a 64-bit program, then he will get an incorrect result.

It seems to me that this obvious cast remains in the code by accident and should simply be deleted.

Consider the second example:

 WINBASEAPI SIZE_T WINAPI GlobalSize (_In_ HGLOBAL hMem); inline void __cdecl memcpy_s( _Out_writes_bytes_to_(_S1max,_N) void *_S1, _In_ size_t _S1max, _In_reads_bytes_(_N) const void *_S2, _In_ size_t _N); AFX_STATIC HGLOBAL AFXAPI _AfxCopyGlobalMemory(....) { ULONG_PTR nSize = ::GlobalSize(hSource); .... Checked::memcpy_s(lpDest, (ULONG)::GlobalSize(hDest), lpSource, (ULONG)nSize); .... } 

PVS-Studio warning: V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'nSize'. olemisc.cpp 684.

The GlobalSize () function returns the type SIZE_T. The arguments to the memcpy_s () function are also of type size_t.

So why then did an explicit type cast "(ULONG) :: GlobalSize (hDest)" be made?

If we work with a buffer of more than 4Gb, then the memcpy_s () function will copy only part of the array.

There are some more suspicious type casts:

• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen (* vp ++)'. cenvarg.c 236
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: ':: GlobalSize (m_hGlobalMemory)'. fileshrd.cpp 48
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen (lpsz)'. dumpcont.cpp 31
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wcslen (lpsz)'. dumpcont.cpp 82
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: '(cElems * sizeof (CLSID))'. ctlcore.cpp 1975
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'wParam'. afxtoolbarslistcheckbox.cpp 94
• V220 Suspicious sequence of types of castings: memsize -> 32-bit integer -> memsize. The value being casted: 'nChars * sizeof (TCHAR)'. statreg.h 270

Use before check

 CMFCPopupMenu* CMFCCustomizeButton::CreatePopupMenu() { .... if (m_pWndParentToolbar->IsLocked()) { pMenu->GetMenuBar()->m_pRelatedToolbar = m_pWndParentToolbar; } pMenu->m_bRightAlign = m_bMenuRightAlign && (m_pWndParentToolbar->GetExStyle() & WS_EX_LAYOUTRTL) == 0; BOOL bIsLocked = (m_pWndParentToolbar == NULL || m_pWndParentToolbar->IsLocked()); .... } 

PVS-Studio warning : V595 The m_pWndParentToolbar 'pointer was used against nullptr. Check lines: 192, 199. afxcustomizebutton.cpp 192

At the beginning, the pointer 'm_pWndParentToolbar' is dereferenced in the expression 'm_pWndParentToolbar-> IsLocked ()'. And then it is checked for equality to zero: 'm_pWndParentToolbar == NULL'.

Such code is dangerous and, I think, it is not necessary to explain why.

Another such case:

 void COleControlSite::BindDefaultProperty(....) { .... if (pDSCWnd != NULL) { .... m_pDSCSite = pDSCWnd->m_pCtrlSite; .... m_pDSCSite->m_pDataSourceControl->BindProp(this, TRUE); if (m_pDSCSite != NULL) m_pDSCSite->m_pDataSourceControl->BindColumns(); } .... } 

PVS-Studio warning: V595 The m_pDSCSite 'pointer was used against nullptr. Check lines: 1528, 1529. occsite.cpp 1528

Superfluous variables

Unnecessary variables are not an error. But superfluous, they are superfluous, and it is worth getting rid of them. Example:

 int GetImageCount() const { CRect rectImage(m_Params.m_rectImage); if (m_Bitmap.GetCount() == 1) { HBITMAP hBmp = m_Bitmap.GetImageWell(); BITMAP bmp; if (::GetObject(hBmp, sizeof(BITMAP), &bmp) == sizeof(BITMAP)) { return bmp.bmHeight / m_Params.m_rectImage.Height(); } return 0; } return m_Bitmap.GetCount(); } 

PVS-Studio warning : V808 'rectImage' object of 'CRect' type created but did not use. afxcontrolrenderer.h 89

The rectangle 'rectImage' is created, but not used later. An extra line in the program and a few extra bars when running the Debug version.

I give a list of found unnecessary variables: vs2003_V808.txt

Little things

Quite a lot of warnings can be attributed not to errors, but rather to an unsuccessful programming style. It seems to me that the source code of Visual C ++ libraries should serve as a role model for other programmers. Therefore, do not teach them the bad things.

I will list some pieces that can be improved.

Dangerous comparisons with TRUE

 _PHNDLR __cdecl signal(int signum, _PHNDLR sigact) { .... if ( SetConsoleCtrlHandler(ctrlevent_capture, TRUE) == TRUE ) .... } 

PVS-Studio warning : V676 BOOL type with TRUE. winsig.c 255

Everywhere, including MSDN, it says that it is not good to compare something with TRUE. The function can return any value other than 0, and this will be considered true. TRUE, this is 1. It is always correct to compare: Foo ()! = FALSE.

Similar checks can be found here:

• V676 BOOL type with TRUE. event.cpp 448
• V676 BOOL type with TRUE. Correct expression is: 'retVal! = FALSE'. resourcemanager.cpp 1437
• V676 BOOL type with TRUE. Correct expression is: 'retVal! = FALSE'. resourcemanager.cpp 5027

Increment

 void _To_array( ::Concurrency::details::_Dynamic_array<_EType>& _Array) { _LockHolder _Lock(_M_lock); _M_iteratorCount++; for(_LinkRegistry::iterator _Link = _M_links.begin(); *_Link != NULL; _Link++) { _Array._Push_back(*_Link); } } 

PVS-Studio warning: V803 Decreased performance. In case of "_Link" is an iterator Replace iterator ++ with ++ iterator. agents.h 1713

A trifle, of course, but everywhere it is recommended to use ++ iterator. If it is possible here, then it is better to use the prefix operator to demonstrate a good style for teaching others.

Note. Notes on this topic:

• Is it practical to use the prefix increment operator ++ it for iterators instead of postfix increment it ++ .
• Pre vs. post increment operator - benchmark .

If the authors of the libraries find it worthwhile to work with an increment, then here is a list of places for this: vs2003_V803.txt .

Improper alert level recovery

 #pragma warning (disable : 4311) SetClassLongPtr(m_hWnd, GCLP_HBRBACKGROUND, PtrToLong(reinterpret_cast<void*>( ::GetSysColorBrush(COLOR_BTNFACE)))); #pragma warning (default : 4311) 

Warning V665 Possibly, the usage of #pragma warning (default: X) 'is incorrect in this context. The '#pragma warning (push / pop)' should be used instead. Check lines: 165, 167. afxbasepane.cpp 167

The correct way to return a previous warning state is to use "#pragma warning (push [, n])" and "#pragma warning (pop)".

The remaining places: vs2003_V665.txt .

Check (this == NULL)

Classics of the genre:

 _AFXWIN_INLINE CWnd::operator HWND() const { return this == NULL ? NULL : m_hWnd; } 

PVS-Studio warning : V704 'this == 0' expression should be avoided, because it can never be NULL. afxwin2.inl 19

Unfortunately, this is a very frequent pattern. Especially in MFC. But it is worth slowly weaning people to use such constructions and set a good example.

For those readers who do not yet know why this is bad, I suggest to get acquainted with the documentation for diagnostics V704 . Everything is described there in sufficient detail.

I understand that there is no way to fix operator HWND (). Backward compatibility is more important. But suddenly, somewhere it can be done without serious consequences. The list of such checks: vs2003_V704.txt

Conclusion

As you can see it turned out quite a big article. But, in fact, nothing significant was found. The code of Visual C ++ libraries is uniquely high-quality and debugged.

I would be glad if this article will help in the future to make the library Visual C ++ a little better. I note once again that the check was incomplete. The developers of Visual C ++ libraries will be able to do this better, since they have scripts / projects for building libraries. If there are any difficulties, I am ready to help - write to us in support.

PS For those who missed the post " Let's play the game, " I suggest to check their attentiveness and pass the proposed test . At the same time I want to answer the question why the test for a while. It is the trolling of those people who claim that the errors that PVS-Studio detects are in their eyes in 15 seconds.

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. A Slipshod Check of the Visual C ++ 2013 Library (update 3) .